Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toolbar box-shadow mask disappears when keyboard is up #2760

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Toolbar box-shadow mask disappears when keyboard is up #2760

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7d2bc22
Add one line change for visible box-shadow on safari by setting overf…
Jan 7, 2025
ccfe321
Add fix to prevent absolute position showing horizontal scroll bar.
Jan 8, 2025
a66f0a1
Fix modal component issue due to css changes
Jan 8, 2025
b453223
Fix lint errors.
Jan 8, 2025
4cdb704
Remove console logs from lint
Jan 8, 2025
8714d48
Add change to base button
Jan 22, 2025
3b6fc63
Adjustment
Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

</head>
<body>
<div style="position: relative; overflow: hidden;">
<div id="root"></div>

<script>
Expand All @@ -45,5 +46,6 @@
document.body.setAttribute('data-env', navigator.webdriver ? 'test' : '%MODE%')
</script>
<script type="module" src="/src/index.tsx"></script>
</div>
</body>
</html>
13 changes: 12 additions & 1 deletion src/components/modals/ModalComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,18 @@ class ModalComponent extends React.Component<ModalProps> {
const modalClasses = modalRecipe({ id, center })

return (
<div ref={this.ref} style={style} className={cx(modalClasses.root, css({ ...(top ? { top: 55 } : null) }))}>
<div
ref={this.ref}
style={style}
className={cx(
modalClasses.root,
css({
overflow: 'visible',
position: 'relative',
...(top ? { top: 55 } : null),
}),
)}
>
{!this.props.preventCloseOnEscape && !hideClose && (
<a
className={css({
Expand Down
2 changes: 0 additions & 2 deletions src/e2e/puppeteer-environment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ const PuppeteerEnvironment: Environment = {
.catch((err: Error) => {
// using `console.log` here to avoid errors or logs being swallowed by vitest
// all of `console.error`, `console.warn` and `console.info` don't show up in the terminal
// eslint-disable-next-line no-console
console.log('Could not connect to browserless.')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid irrelevant changes such as these.

throw err
})

Expand Down
2 changes: 0 additions & 2 deletions src/e2e/puppeteer/setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,6 @@ const setup = async ({
break
case 'info':
case 'log':
// eslint-disable-next-line no-console
console[messageType](text)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid irrelevant changes such as these.

break
// ConsoleMessage 'warning needs to be converted to native console 'warn'
case 'warn':
Expand Down
2 changes: 0 additions & 2 deletions src/hooks/usePositionFixed.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ const initEventHandler = once(() => {
/** Emulates position fixed on mobile Safari with positon absolute. Returns { position, overflowX, top } in absolute mode. */
const usePositionFixed = (): {
position: 'fixed' | 'absolute'
overflowX?: 'hidden' | 'visible'
top: string
} => {
const position = positionFixedStore.useState()
Expand All @@ -37,7 +36,6 @@ const usePositionFixed = (): {

return {
position: position ?? 'fixed',
overflowX: position === 'absolute' ? 'hidden' : 'visible',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain this change? I'm a bit worried there will be unintended side effects from removing this attribute in the several places that use usePositionFixed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is needed to be removed and was discussed with @raineorshine in the ticket 1785. The reason that overflowX needed to be set when we switch to position absolute, was because there were no 'relative bounds' surrounding the app. When we switch to absolute, it moves part of the page off screen horizontally, thus giving the app a horizontal scroll bar that we don't need.

You can see the videos above attached to this pr, showing the horizontal scroll bar appear. This was a patchwork fix in the past and it needed to be reworked, because everytime the overflowX was set to hidden while the position was absolute, the boxShadow would disappear.

In order to resolve the issue of the boxShadow disappearing we can only find a way to remove this overflowX change (regarding the horizontal scrollbar), and to resolve this we need to add a relative bounds and overflow setting around all the apps which display in the root.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great explanation, thanks.

/* spacing.safeAreaTop applies for rounded screens */
top: position === 'absolute' ? `${scrollTop}px` : token('spacing.safeAreaTop'),
}
Expand Down
Loading