-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Hide the sidebar when collapsed to prevent browser search to find text from it #2725
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.
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
Conversation
44dfae5
to
9c1d6db
Compare
Thanks to the GUI tests, I uncovered the case where the window size is too small and therefore the sidebar should not be displayed by default. =D |
Anyway, it's ready for review. |
Thank you very much! This seems to introduce a few regressions:
Would you be able to look into those issues? |
On safari, sadly no. But on firefox, definitely. :) |
9c1d6db
to
f4eacc3
Compare
Fixed. I also extended the GUI test to ensure it doesn't happen again. |
When showing the sidebar, Safari was causing the sidebar to snap into place without animating. This is apparently some well-known issue where it doesn't like adding new elements (or changing display) and toggling an animated transition in the same event loop.
Thanks, I think I have fixed the issue that was happening on Safari. |
// Workaround for Safari skipping the animation when changing | ||
// `display` and a transform in the same event loop. This forces a | ||
// reflow after updating the display. | ||
sidebar.offsetHeight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very confused about this fix considering it does nothing. Dark magic.
Fixes #2721.
For the GUI test, I need to add a new command to ensure the text is not searchable when collapsed. I opened an issue for that in browser-ui-test: GuillaumeGomez/browser-UI-test#659.