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.

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

fix: set display none on overlay part to fix Safari 17.4 issue #7246

Merged
merged 4 commits into from Mar 21, 2024

Conversation

web-padawan
Copy link
Member

@web-padawan web-padawan commented Mar 20, 2024

Description

Fixes #7245

Type of change

  • Bugfix

Before

safari-broken.mp4

After

safari-working.mp4

Note

Test changes are due to the fact that previously getFocusableElements() was sometimes called with closed overlay.
As the helper used internally only checked for the element to be hidden directly, it returned the focusable elements.

Now when the CSS is updated to set display: none on the overlay part explicitly, those tests started to fail.

Also, some tests have been adjusted to use proper timings (so that the overlay doesn't get detached too early).

@web-padawan
Copy link
Member Author

Field highlighter test is failing for vaadin-select which is apparently related to closing state. Taking a look.

Copy link
Member

@jouni jouni left a comment

Choose a reason for hiding this comment

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

Without some animation on the host, closing is immediate. So this fix is not what we want. We only check the host element for the animation property and delay the DOM removal until that animation has finished.

The actually visible animation is on the overlay part, not on the host. The "dummy" animation is there only to provide the same animation duration as the animation on the overlay part. If the duration of the dummy animation is shorter than the animation on the overlay part, then the animation will be cut short.

If the dummy animation is longer, then the overlay part might reappear for a brief period after its animation is finished, but that depends on the animation-fill-mode property value.

@web-padawan
Copy link
Member Author

Updated to set display: none !important on [part='overlay'] which seems to resolve the issue.
For some reason, blinking overlay was visible when no longer attached to the document body 🤷‍♂️

@web-padawan web-padawan changed the title fix: do not use dummy animation on close to fix Safari 17.4 fix: set display none on overlay part to fix Safari 17.4 issue Mar 20, 2024
@web-padawan
Copy link
Member Author

Updated failing overlay focus-trap tests to first open the overlay and then get focusable elements. Also fixed a dialog test.

Copy link

sonarcloud bot commented Mar 21, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@web-padawan web-padawan merged commit 08bf848 into main Mar 21, 2024
9 checks passed
@web-padawan web-padawan deleted the fix/dummy-animation-safari branch March 21, 2024 11:13
@vaadin-bot
Copy link
Collaborator

Hi @web-padawan and @web-padawan, when i performed cherry-pick to this commit to 23.4, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 08bf848
error: could not apply 08bf848... fix: set display none on overlay part to fix Safari 17.4 issue (#7246)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.4.0.alpha17 and is also targeting the upcoming stable 24.4.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component overlay opens multiple times in Safari
4 participants