-
Notifications
You must be signed in to change notification settings - Fork 1
Properly update anchored regions in dialogs and other top-layer elements #46
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
base: archives/fast-element-1
Are you sure you want to change the base?
Changes from all commits
a2bce98
3b7f747
2d8946a
dd0a941
f243483
d919431
bb8caed
7582e43
2d91fb7
e156fbb
9fc3e0b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "patch", | ||
| "comment": "Properly update anchored regions in dialogs", | ||
| "packageName": "@ni/fast-foundation", | ||
| "email": "7282195+m-akinc@users.noreply.github.com", | ||
| "dependentChangeType": "patch" | ||
| } | ||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,21 @@ | ||
| import { attr, DOM, observable } from "@ni/fast-element"; | ||
| import { Direction, eventResize, eventScroll } from "@ni/fast-web-utilities"; | ||
| import type { TopLayerCallback, TopLayerObserver } from "top-layer-observer"; | ||
| import { FoundationElement } from "../foundation-element/foundation-element.js"; | ||
| import { topLayerRootAncestor } from "../utilities/composed-parent.js"; | ||
| import { getDirection } from "../utilities/direction.js"; | ||
| import { IntersectionService } from "../utilities/intersection-service.js"; | ||
| import type { | ||
| ResizeObserverClassDefinition, | ||
| ResizeObserverEntry, | ||
| } from "../utilities/resize-observer.js"; | ||
|
|
||
| // 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
|
|
||
| /** | ||
| * Defines the base behavior of an anchored region on a particular axis | ||
| * | ||
|
|
@@ -404,13 +412,17 @@ export class AnchoredRegion extends FoundationElement { | |
| // justify a layout update that affects the dom (prevents repeated sub-pixel corrections) | ||
| private updateThreshold: number = 0.5; | ||
|
|
||
| private scrollListenerTarget: EventTarget = window; | ||
| private topLayerObserver: TopLayerObserver | null = null; | ||
|
|
||
| private static intersectionService: IntersectionService = new IntersectionService(); | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| connectedCallback() { | ||
| super.connectedCallback(); | ||
| this.scrollListenerTarget = topLayerRootAncestor(this) ?? window; | ||
| if (this.autoUpdateMode === "auto") { | ||
| this.startAutoUpdateEventListeners(); | ||
| } | ||
|
|
@@ -1315,10 +1327,11 @@ export class AnchoredRegion extends FoundationElement { | |
| */ | ||
| private startAutoUpdateEventListeners = (): void => { | ||
| window.addEventListener(eventResize, this.update, { passive: true }); | ||
| window.addEventListener(eventScroll, this.update, { | ||
| passive: true, | ||
| capture: true, | ||
| }); | ||
| this.addScrollListener(); | ||
| if (topLayerObserverConstructor !== undefined) { | ||
| this.topLayerObserver ??= new topLayerObserverConstructor(this.handleTopLayerChange); | ||
| this.topLayerObserver.observe(); | ||
| } | ||
| if (this.resizeDetector !== null && this.viewportElement !== null) { | ||
|
Comment on lines
1328
to
1335
|
||
| this.resizeDetector.observe(this.viewportElement); | ||
| } | ||
|
|
@@ -1328,10 +1341,33 @@ export class AnchoredRegion extends FoundationElement { | |
| * stops event listeners that can trigger auto updating | ||
| */ | ||
| private stopAutoUpdateEventListeners = (): void => { | ||
| this.topLayerObserver?.disconnect(); | ||
| window.removeEventListener(eventResize, this.update); | ||
| window.removeEventListener(eventScroll, this.update); | ||
| this.removeScrollListener(); | ||
| if (this.resizeDetector !== null && this.viewportElement !== null) { | ||
| this.resizeDetector.unobserve(this.viewportElement); | ||
| } | ||
| }; | ||
|
|
||
| private handleTopLayerChange = (): void => { | ||
| const newValue = topLayerRootAncestor(this) ?? window; | ||
| if (newValue !== this.scrollListenerTarget) { | ||
| this.removeScrollListener(); | ||
| this.scrollListenerTarget = newValue; | ||
| this.addScrollListener(); | ||
| } | ||
| }; | ||
|
|
||
| private addScrollListener = (): void => { | ||
| this.scrollListenerTarget.addEventListener(eventScroll, this.update, { | ||
| passive: true, | ||
| capture: true, | ||
| }); | ||
| }; | ||
|
|
||
| private removeScrollListener = (): void => { | ||
| this.scrollListenerTarget.removeEventListener(eventScroll, this.update, { | ||
| capture: true, | ||
| }); | ||
| }; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general thoughts:
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the
top-layer-observerlibrary referenced here would be useful: whatwg/html#9075 (comment)or overkill, not sure yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
hiddenis toggled.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked offline, some things discussed:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I've addressed all the feedback now.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked offline and discussed the two alternate implementation strategies that are the alternate to top-layer-observer:
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