Toolbar box-shadow mask disappears when keyboard is up #2760
Conversation
…lowX to visible when set as absolute position.
|
Thanks for the submission! It would be nice if it was this easy, though I have my doubts. We know it was added for a reason. I think we should track down the commit message that describes the original use of Other behavior to test on mobile Safari with keyboard open and closed:
|
|
@raineorshine Alright, I finally found a solution to this issue after a few hours of work. I managed to solve the problem with overflow by adding a relative bounds div wrap everything within the body tag, with an overflow: hidden property. Everything seems to be working so far and I haven't found any issues with this solution so far. If you guys can find any regressions of bugs introduced by this fix, please let me know. Simulator.Screen.Recording.-.iPhone.15.Pro.-.2025-01-08.at.13.16.02.mp4 |
|
@trevinhofmann I think things are working with this fix, but the puppeteer tests are all messed up. |
There was a problem hiding this comment.
Testing locally, this proposal does seem to resolve the toolbar box-shadow issue. It also appears to introduce regressions.
the puppeteer tests are all messed up
Yes, there are several failing puppeteer tests. They are (mostly?) related to the Home icon being shifted down. This appears to be an unintended side effect and will need to be resolved.
Example:
| @@ -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.') | |||
There was a problem hiding this comment.
Please avoid irrelevant changes such as these.
| @@ -50,8 +50,6 @@ const setup = async ({ | |||
| break | |||
| case 'info': | |||
| case 'log': | |||
| // eslint-disable-next-line no-console | |||
| console[messageType](text) | |||
There was a problem hiding this comment.
Please avoid irrelevant changes such as these.
| @@ -37,7 +36,6 @@ const usePositionFixed = (): { | |||
|
|
|||
| return { | |||
| position: position ?? 'fixed', | |||
| overflowX: position === 'absolute' ? 'hidden' : 'visible', | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Great explanation, thanks.
This comment was marked as resolved.
This comment was marked as resolved.
|
@raineorshine and @trevinhofmann, I think I am confused about what regressions this change has introduced. I can address them as soon as I know what they are. The horizontal scroll bar has been fixed after adding the relative div surrounding the app root. However, since I added the div, the app's stability has improved and the scrolling doesn't 'jump arround' like it used too and the menu, toolbar, and overall app screen has been shifted slightly causing the puppeteer tests to fail. |
|
Great! If the horizontal scroll bar issue is fixed, then it should just be @trevinhofmann's comments above that require addressing:
I will test at the end to ensure there are no other regressions! |
|
@raineorshine I am not familiar with puppeteer, where should I update the snapshots with the new version in the code? I have been searching for it. |
|
I don't believe you should update any snapshots in this case. If a snapshot is failing, it means a visual regression was introduced. You can see in the diff that the home icon got shifted down. The code should be fixed so that the puppeteer tests pass and match the original snapshots. You can find more information here: https://github.com/cybersemics/em/wiki/Testing#5-visual-snapshot-tests |
|
Then, I cannot put this change in. Because this change adds a relative div box around the app to stabilize changes between position:absolute. The entire app's screenshot is shifted slightly. Technically, this is a 'regression' because of the change compared to the reference screenshot. |
|
Maybe you could find places to adjust the CSS to compensate for the layout shifts? We ideally want to preserve the existing layout while fixing the box-shadow issue. If the |
|
So this change is necessary because there originally was no 'relative bounds' within the app when anything was changed to position:absolute. This caused issues with another pull request of mine that will be also fixed by this pr. Because it didn't have any relative bounds, the IOS status bar would overlap with the apps. I originally put in a fix for this and was planning to push in another one. However, this pr fixes that issue. Adding this relative bounds prevents position:absolute from extending the app horizontally. In the current app, we just hid the extra horizontal panels by setting OverflowX: hidden. But this is not really a good fix compared to preventing the app from extending hozontially when an element's 'position' changes. Long story short, this is a fix to a root problem of the overflowX. If we don't want to push this in, I have not found another way to remove overflowX. and remove the horizontal app additions with a position change. |
|
I had hoped that, knowing that the puppeteer tests where not reliable for this fix, we could still explore this pr and push in the change that would fix a lingering issue. But I wanted to be very careful about it, so I asked for help in reviewing it. |
|
Yes, overall this seems like a good change, since allowing the app to extend horizontally is undesirable. Regardless of how correct this change is, it causes a visual regression which impacts the user experience. Generally we want to patch regressions in the same PR. If we merged this now and then fixed the positioning in a separate PR, we would have a "bad" commit in the commit history, which can interfere with |
|
@raineorshine Do you know any advice on how to handle updating the patch regressions then? I don't know how to make any more progress on this pr. |
|
I'm not sure what you mean by updating the patch regressions. This looks to be a CSS issue. Are you able to reproduce it in Chrome? |
|
@raineorshine No, this issue is caused by what I mentioned earlier in this thread.
This div is newly added as a bounds around the entire app in order to fix the problem with The only problem is, I can't move this CSS styling to any other places without causing other styling changes within the app. Its best as it is right now. However, because of this new div surrounding everything, the puppeteer tests are failing. So here comes my dilemma about what I had been asking about. I can either discard this fix and this pr completely, because the fix is relative niche as it only occurs on safari. Or we can update the saved screenshots of what the CSS used to look like to match these on coming changes. |
|
And I don't know how to update those saved screenshots with puppeteer. I am stuck. |
|
I think you might be missing the point here. Updating the screenshots is like turning off the fire alarm off when there is a fire. The puppeteer tests are trying to tell us something. In this case, it's not a fire, but a flaw in the construction. It's like the contractor neglected to put a vent in the bathroom. But in order to put in the vent, we need to move the sink and re-paint. I can reproduce the regression very easily in Chrome. It's clearly a CSS issue. There are some other styles that might need to be adjusted or elements moved to accommodate the new relative container. Is this something you're interested in doing? If not, that's totally okay. It's up to you whether you want to continue on this. The success criteria is resolution of the box-shadow issue without any regressions. I can confidently say that it is possible. If you're not up for it, I totally understand. |
|
@raineorshine I do realize the point of the tests. However, I might be confused about how to update new CSS changes if the puppeteer service prevents differences of a certain amount. There are 6 tests that are failing due to less than 1% difference in pixels. I may not be able to handle this final push to fix these changes due to my lack of understanding on this aspect. I understand that puppeteer is comparing differences and warning about it, but it shouldn't be like traditional tests like unit tests where it tests the logic. You are right that I may not be up for it, because I have been thinking about this for a bit too long to continue to admit that I can finish it according you the code review comments I was given. |
|
If these were imperceptible differences, I would totally understand that. But the visual regression is quite clear. It is a CSS issue that can be troubleshooted like any CSS issue. If you start looking at the home icon and the nav bar, I bet you would start to get some clues. It's totally up to you though. I've appreciated your help up to this point! Current Behavior
Expected Behavior
|
|
Oh, if this is the issue it's not a difficult fix. Currently, it exists but you need to scroll down the page to see it. 
|
Yes, this is what the puppeteer snapshot tests show us. You should be able to see it in the diff on your machine when you run
Yes, exactly. You can test on |
Fix for issue: 1785
Need to keep an eye on these changes to make sure no regression is introduced, but otherwise this seems to be a solid fix for keeping the box shadow on safari browser.