lion icon indicating copy to clipboard operation
lion copied to clipboard

chore: use dialog element for top layer functionality of all overlays

Open tlouisse opened this issue 2 years ago • 5 comments

Working POC that uses dialog element for top layer functionality of all overlays.

tlouisse avatar May 25 '22 07:05 tlouisse

🦋 Changeset detected

Latest commit: 4dc2aebbcbc0007de4b407ea4ea71cab9aff753a

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

changeset-bot[bot] avatar May 25 '22 07:05 changeset-bot[bot]

Hey @tlouisse, I've been keeping an eye on this draft and thanks for picking it up. Am I right in thinking this will allow an overlay to be positioned relative to the invoker, but still be mounted to the root node (to avoid overflow issues etc)?

stefanpearson avatar Jul 11 '22 12:07 stefanpearson

Hey @tlouisse, I've been keeping an eye on this draft and thanks for picking it up. Am I right in thinking this will allow an overlay to be positioned relative to the invoker, but still be mounted to the root node (to avoid overflow issues etc)?

Hi @stefanpearson,

You're right about the overflow issues being avoided for overlays positioned relative to their invoker. Since we use the dialog under the hood, they can stay in the same dom position (no need to move to the body (a.k.a. root node) anymore), since modal dialogs will paint to the top layer. (for non modal dialogs we will pull of some css tricks to still make them escape out of their parent as we will show in the demos).

Using the dialog in context (not moving dom nodes around) brings all kinds of benefits:

  • a11y works as expected. It's possible to have id references from content to outside or vice versa
  • css works as expected. css variables and parts rely on dom inheritance and will always work now
  • better performance (connectedCallback runs once)

tlouisse avatar Jul 13 '22 11:07 tlouisse

I've looked into the PoC and love that it keeps the content right where it was put. Vue for instance is sensitive to its rendered elements getting relocated especially when it tries to do DOM-patching. Our team is now considering how to approach the overlays. How do you see this improvement landing in Lion?

I'm happy to help in bringing this PoC into a release. Eg. trying out versions, or help out with simpler tasks (got some rookie level webcomponent / Lion overlay system background).

riovir avatar Jul 18 '22 14:07 riovir

I've looked into the PoC and love that it keeps the content right where it was put. Vue for instance is sensitive to its rendered elements getting relocated especially when it tries to do DOM-patching. Our team is now considering how to approach the overlays. How do you see this improvement landing in Lion?

I'm happy to help in bringing this PoC into a release. Eg. trying out versions, or help out with simpler tasks (got some rookie level webcomponent / Lion overlay system background).

Thanks for checking it out @riovir! For now I would need to do this in my spare time, which I don't have that plenty of atm :) So help would be really welcome.

What needs to be done, I think:

  • a11y manual testing => are screen reader outputs the same before and after the change? I think the original tests were done by @erikkroes, maybe he can give you some hints on what to look for specifically. We strive to test at least the three most popular sr/browser combinations:
    • Jaws + Edge
    • NVDA + Firefox
    • VoiceOver + Safari
  • I think there are some failing tests left that need to be fixed
  • rebase with master
  • Maybe @daKmoR wants to have a check whether everything in OverlayManager is still needed and works correct?
  • We use the <dialog> element in shadow dom, but the accessible role (role=dialog/tooltip/listbox etc.) should be in light dom. Therefore, we put role=none (accessible role is already on an element below), but now we can still leverage the 'top layer capabilities'. Although this has no effect on a11y, axe test tooling will complain that you should not change semantics of native elements. We need to add some docs explaining that it's all ok: we have a good reason to do so (until the browser supports popup)

Then, for non modal dialogs, there will be some challenges with z-index. It would be nice to demo those in the edge cases section.

tlouisse avatar Jul 19 '22 07:07 tlouisse

LGTM - nice job 💪

daKmoR avatar Dec 09 '22 14:12 daKmoR

@riovir @stefanpearson: we finally put in the effort to wrap this up.

Feel free to try out in our new release of @lion/ui version >= 0.0.10 🎉

tlouisse avatar Dec 09 '22 15:12 tlouisse

Hi! Can't wait! 😁

riovir avatar Dec 09 '22 18:12 riovir

@tlouisse I took the dialog for a spin. It works well in the LionDialog! Unfortunately, when testing out the LionSelectRich the component seems to be broken in the 0.0.11 version. The symptoms point to click and enter events causing an unintended echo effect:

Seemingly two events emitted without animations, causing the select to immediately close on open. When dropdown animations are in place I notice the component repeatedly open and close itself.

I've included a reproduction recipe in a minimalized Vue app here: https://gitlab.com/riovir/vue-app-scaffolding/-/tree/lion-ui-11-select-rich-reprod

It includes an example of how I normally use Lion components (wrap them into Vue components, define them under a globally scoped name) and a barebone using the webcomponents directly.

I'm unsure if the problem is tied to the overlays or comes from the component.

Once the necessary issues are addressed are there plans, to publish them in the separated packages or it's best to start migrating our design system to the @lion/ui?

riovir avatar Dec 16 '22 13:12 riovir

@riovir we just released @lion/[email protected] that addresses one issue in the select-rich... I'm however not sure if it's the one you encounter 🤔 I think there are some more issues 😅

In the long run the idea is to keep only the @lion/ui package with multiple entrypoints as it has the exact same runtime implications but is much easier to consume and maintain.

It's currently in alpha for testing - but early next year we will release a beta version for public consumption and deprecate all @lion/* packages except @lion/ui and @lion/ajax

daKmoR avatar Dec 16 '22 13:12 daKmoR

The v0.0.12 release indeed solved my problem with the LionSelectRich. :)

riovir avatar Dec 20 '22 08:12 riovir