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: pointer-events when content has CSS animations #173

Merged
merged 3 commits into from
Mar 5, 2020

Conversation

Haprog
Copy link
Contributor

@Haprog Haprog commented Mar 3, 2020

@Haprog
Copy link
Contributor Author

Haprog commented Mar 4, 2020

Fails in Safari and iOS Safari. Needs a bit more work. (Edit: fixed)

Haprog added 2 commits March 4, 2020 12:45
Makes modal state and focus restoration behaviour synchronous (without depending on possible closing/opening animation being finished).
Comment on lines +582 to +584
if (event && event.target !== this) {
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is most of the fix. This makes overlay ignore any unrelated animationend events that might be triggered by anything inside the dialog (e.g. if using <vaadin-text-area> inside the dialog which triggers a CSS animation on init).

if (!this.modeless) {
this._enterModalState();
}
this.removeAttribute('opening');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the opening attribute was moved to be the very last thing in finishOpening() so we can use it (e.g. in MutationObserver) to detect when the opening has been completely finished (including possible animations).

Other stuff (the modal state update) was moved out of this method to be run earlier/synchronously as it doesn't need to be tied to the completion of the possible animation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar changes to finishClosing() below.

Copy link

Choose a reason for hiding this comment

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

In my opinion, this refactor is important, in the future, we can use the opening/closing to check something because we know the status state: opening/ opened or closing/closed.

This fix addressed the bug and refactored code better. Thank you for the great fix.

Comment on lines 718 to 722
// Disable pointer events in other attached overlays
OverlayElement.__attachedInstances.forEach(el => {
if (el !== this && !el.hasAttribute('opening') && !el.hasAttribute('closing')) {
if (el !== this) {
el.shadowRoot.querySelector('[part="overlay"]').style.pointerEvents = 'none';
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was simplified a bit as this works like this now with the updated logic above. Now pointer-events: none might also be applied to closing or opening overlays here but that should be fine. Closing dialogs will remove it themselves after the closing animation finishes (in finishClosing()).

Comment on lines +16 to +31
<dom-module id="overlay-test-styles" theme-for="vaadin-overlay">
<template>
<style>
:host([animate][opening]),
:host([animate][closing]) {
animation: 50ms overlay-dummy-animation;
}

@keyframes overlay-dummy-animation {
to {
opacity: 1 !important; /* stylelint-disable-line keyframe-declaration-no-important */
}
}
</style>
</template>
</dom-module>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an animation similar to what vaadin-dialog applies on the dialog overlay (where the issue was found using vaadin-text-area in vaadin-dialog).

Copy link
Member

@tomivirkki tomivirkki left a comment

Choose a reason for hiding this comment

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

Seems to work great 👍 Also, the old test suite passes with the changes.

@Haprog Haprog merged commit 1e9e4c0 into master Mar 5, 2020
@Haprog Haprog deleted the fix/158-ui-locked branch March 5, 2020 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants