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(overlays): no hiding of nested overlays having hideOnEsc configured #2398

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

tlouisse
Copy link
Member

@tlouisse tlouisse commented Oct 31, 2024

closes #2198

Copy link

changeset-bot bot commented Oct 31, 2024

🦋 Changeset detected

Latest commit: c798ce5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lion/ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tlouisse tlouisse force-pushed the fix/esc-on-nested-overlay branch 3 times, most recently from 576d1d8 to 90a69c7 Compare October 31, 2024 16:17
gerjanvangeest
gerjanvangeest previously approved these changes Nov 4, 2024
@tlouisse tlouisse force-pushed the fix/esc-on-nested-overlay branch from 90a69c7 to 9d6ba25 Compare November 4, 2024 09:16
@gerjanvangeest gerjanvangeest dismissed their stale review November 4, 2024 10:39

If the child overlay has hidesOnOutsideEsc set to true, does not mean the parent overlay should close.

Comment on lines 1149 to 1165
if (ev.key !== 'Escape' || (ev._hasAlreadyHandledEscape && !this.hidesOnOutsideEsc)) return;

this.hide();

const shouldNotCloseParents = this.hidesOnEsc;
if (shouldNotCloseParents) {
// We could do ev.stopPropagation() here, but we don't want to hide info for
// the outside world about user interactions. Instead, we piggyback
// on the event so our parent overlay can read it.
// @ts-expect-error
// eslint-disable-next-line no-param-reassign
ev._hasAlreadyHandledEscape = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

My general idea about JS event is that an event is just thrown with details no more than its type and the target then more details should be investigated from the target by the event handler.

Do you think it's better if we let this handler investigate the target to decide if it has been already handled?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can make it cleaner by storing these data in a Weakmap instead of on the event. The thing here is that it passes two nested overlays and the parent one should know about the child one, so just checking the target is not a solution

Copy link
Member Author

@tlouisse tlouisse Nov 4, 2024

Choose a reason for hiding this comment

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

See https://jsfiddle.net/rmsc6u73/ to see what we're trying to achieve

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the example! Great to see how the dialog element simplify things.

How do you think about making the OverlayManger know about parent-child relationship of overlays and handle what should happen when the ESC key was pressed?

I think nested popups often comes with different way to display which can benefit from the central manager:

  • We might want to have control on the background of the popups (ex: making the half-opaque background to stack to make the background darker or just keep the background opacity)
  • We might want to put the child popup so that it looks neatly stacked above the parent, especially on mobiles
  • In the previous company, the designer decided to put the child popup contents in the parent popup like the iOS navigation menu (https://developer.apple.com/documentation/uikit/uinavigationcontroller)

Anyway, I'm not insisting, just a suggestion.

Copy link
Member Author

@tlouisse tlouisse Nov 5, 2024

Choose a reason for hiding this comment

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

I really like the scenarios you describe. It would be great to create them in a demo (as a lion extension example) and see what's needed to build them :)

With all the recent platform developments (especially wrt popover) we have a great opportunity to make things more lightweight (I think we still need the manager and controller, see https://hidde.blog/popover-accessibility, https://adrianroselli.com/2023/05/brief-note-on-popovers-with-dialogs.html, etc.).
Let's refine the future of the system together and have a brainstorm session about how we can make it better with modern technology.

Currently, I have a draft doc that explains the origins (and potential future) of the overlay system. I'm also thinking about ways to make the manager opt-in. Since we're using dialog inside today we can leverage the document order (whereas before we moved contents to the body and needed the OverlayManager to keep track of their ordering). Having this dom order intact, we have a lot of opportunities css-wise and by querying the dom.

@tlouisse tlouisse force-pushed the fix/esc-on-nested-overlay branch from d1f6685 to ade4034 Compare November 4, 2024 22:11
@tlouisse tlouisse force-pushed the fix/esc-on-nested-overlay branch from ade4034 to 9ac290e Compare November 4, 2024 22:16
@tlouisse tlouisse merged commit 03a9b33 into master Nov 5, 2024
7 checks passed
@tlouisse tlouisse deleted the fix/esc-on-nested-overlay branch November 5, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants