Properly update anchored regions in dialogs and other top-layer elements - #46
Properly update anchored regions in dialogs and other top-layer elements#46m-akinc wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses anchored regions failing to re-position when scrolling occurs inside a <dialog>, by detecting an ancestor dialog in the composed tree and (intendedly) routing auto-update event listeners to the appropriate scroll container.
Changes:
- Extend
composedParent()traversal to optionally follow slot assignment, and add a new helper to find the closest ancestor dialog. - Update
AnchoredRegionto capture an ancestor dialog on connect and use it as the event target for auto-update listeners. - Add a change file for a patch release of
@ni/fast-foundation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/web-components/fast-foundation/src/utilities/composed-parent.ts | Adds optional slot-aware composed-parent traversal and a new dialog-ancestor helper. |
| packages/web-components/fast-foundation/src/anchored-region/anchored-region.ts | Switches auto-update listener targeting to dialog vs window based on ancestry. |
| change/@ni-fast-foundation-7b80b38c-f554-4f52-87cd-81b3a9c205cc.json | Records a patch change entry for the fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
m-akinc
changed the title
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
Some general thoughts:
- dialog containing elements (dialog / drawer / other) can be nested, dont' think we should assume the most direct parent is the intended target. It may be a dialog element made visible and not open, just being used as a container
- Think what we care about is that only when we are open do we want to find the nearest open top layer element. That can be dialog
el.matches(':modal')or an arbitrary element with popover apiel.matches(':popover-open')(edit: I missed some too, see following comment about thetop-layer-observerlibrary). But I have not thought this through and if we want to make the assumption that while the select is open the app will not switch what is in the top-layer (though that may be reasonable). - The algorithm changes the scroll event and resize event target from window to the dialog, but elements don't fire resize events. Seems like it should still be registered on window. Maybe to be robust the scroll event on dialog is in addition to window instead of replacing?
There was a problem hiding this comment.
Maybe the top-layer-observer library referenced here would be useful: whatwg/html#9075 (comment)
or overkill, not sure yet
There was a problem hiding this comment.
dialog containing elements (dialog / drawer / other) can be nested, dont' think we should assume the most direct parent is the intended target. It may be a dialog element made visible and not open, just being used as a container
Assuming these events bubble all the way up, we probably want the furthest dialog (or whatever top-layer thing) ancestor. The code was already attaching listeners unconditionally, but I guess we could attach/detach the listener(s) when hidden is toggled.
elements don't fire resize events. Seems like it should still be registered on window. Maybe to be robust the scroll event on dialog is in addition to window instead of replacing?
Yeah, the copilot review caught that, too. I changed it so that we always register for the resize event on the window, but conditionally target an ancestor dialog for the scroll events. I don't think there's a reason to listen on both.
There was a problem hiding this comment.
Talked offline, some things discussed:
- Want to test for each of the known top-layer behaviors as mentioned by that library (dialog, popover api, fullscreen, but not customizable select for now since it is not cross-browser)
- Lets start with an evaluation that minimizes assumptions. Can we use the library to observe the top-layer state and respond correctly
- If that complexity is high a simplifying assumption may be to assume while the anchored region is open that top-layers are not changing. This is not a good assumption for every potential usage of an anchored region. Could image someone using a toast or a rich tooltip for a multistep wizard / configuration box that opens additional pop-ups etc.
- Want to avoid coupling to implementation specific assumptions, i.e. coupling to specific attributes, etc.
There was a problem hiding this comment.
I believe I've addressed all the feedback now.
- The code is generalized to handle modal dialogs, popovers, and fullscreen elements.
- Using the
top-layer-observerlibrary to detect when elements move in/out of the top layer and respond by moving the scroll listener, as needed.
There was a problem hiding this comment.
I've done a basic analysis of the top-level-observer and the select-events transitive dep and it does seem pretty heavy weight for our use case.
top-layer-observer is tracking all focus events and doing recursive searches of the shadowroots and registering event listeners on each. I'd be very concerned with potential memory leaks on something like the table that has lots of nested shadow root trees.
select-events seems even heavier registering mutationobservers recursively on all shadowroots. That's a ton of book keeping as well.
I think we should fallback to the assumption we discussed offline that we are checking parent elements for the different types of top-layers and finding the closest open one to register scroll listeners on.
There was a problem hiding this comment.
Talked offline and discussed the two alternate implementation strategies that are the alternate to top-layer-observer:
- (less ideal, effectively rejected) in the current state we can't rely on connect/disconnect corresponding to when the anchored-region should be active. The select and combobox have the anchored region always connected and just change visibility. So we would need to track all parents potential to enter and exit the top-layer. This would require entering all parent shadow roots to observer for toggle events and likely other events for the different popover layer types
- (ideal but requires anchored region api changes) we update the anchored region to have an active/inactive state that components use for when the anchored region should be enabled. Then rely on the assumption that while the anchored region is active we can find the current active top-layer parent if any and that while the anchored region is active the current top-layer parents don't change. It is an assumption that a parent won't enter / exit the top layer while the anchored region is open. Not a perfectly robust assumption but may be good enough. This relies on component usage of anchored region using a new api on anchored region to configure active / inactive states.
- also discussed having components instead use anchored region as expected by the current api where they connect / disconnect in template with a
whendirective, etc. but seems very likely to cause issues as the child slot elements will connect/disconnect and the slotted options are required for state of the parent elements. PRobably much easier / less risky to expose a new active/inactive api on anchored region
- also discussed having components instead use anchored region as expected by the current api where they connect / disconnect in template with a
- (rejected) instead of using detection for what is currently active in the top-layer we assume / configure the anchored region to what is assumed possible top-layer elements to be (certain elements by tagname, etc). Undesired as it couples to specific component implementations and is not robust for an imminent future for more elements participating in popover apis in angular material, etc. Prefer a more robust / general top-layer detection strategy.
…-region-in-dialog
m-akinc
changed the title
| function isTopLayerRoot(element: HTMLElement): boolean { | ||
| return element.matches(':popover-open') | ||
| || element.matches(':modal') | ||
| || document.fullscreenElement === element; | ||
| } |
| private startAutoUpdateEventListeners = (): void => { | ||
| window.addEventListener(eventResize, this.update, { passive: true }); | ||
| window.addEventListener(eventScroll, this.update, { | ||
| passive: true, | ||
| capture: true, | ||
| }); | ||
| this.addScrollListener(); | ||
| this.topLayerObserver ??= new TopLayerObserver(this.handleTopLayerChange); | ||
| this.topLayerObserver.observe(); | ||
| if (this.resizeDetector !== null && this.viewportElement !== null) { |
| // The "top layer" does not exist in Node.js, and the top-layer-observer module crashes when loaded in Node.js. | ||
| let topLayerObserverConstructor: (new (callback: TopLayerCallback) => TopLayerObserver) | undefined; | ||
| if (typeof document !== "undefined") { | ||
| void import("top-layer-observer").then(m => topLayerObserverConstructor = m.TopLayerObserver); |
There was a problem hiding this comment.
Just to capture as a comment, likely something we should document somewhere. Nimble and transitive libraries have gone through significant effort to have a static dependency graph and should not rely on dynamic imports. Applications can for performance optimization, etc. but it should not be forced on applications at the library level. As can be seen by the related rollup config changes, it forces downstream build configuration behavior and gates certain performance characteristics (blocks pages on additional network requests) that applications should instead have control over.
|
Moving back to draft for now as need some research / design proposals to continue |
Pull Request
📖 Description
Anchored regions in modal dialogs do not update their position when the containing scrollable area is scrolled. Anchored region adds an event listener for the scroll event to the
window, but when the anchored region is in a modal dialog, the event listener needs to be on the dialog instead.Added logic to handle popover and fullscreen elements, in addition to modal dialogs.
🎫 Issues
ni/nimble#3002