fix(overlay): default listenerHost to target in overlay-trigger-directive#5873
fix(overlay): default listenerHost to target in overlay-trigger-directive#5873TarunAdobe merged 10 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: bd63ae7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 78 packages
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 |
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
TarunAdobe
force-pushed
the
ttomar/trigger-directive
branch
from
b089a7b to
678b431
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| // Now click the cached trigger to open the overlay. | ||
| const opened = oneEvent(cachedTrigger, 'sp-opened'); | ||
| cachedTrigger.click(); | ||
| await opened; |
There was a problem hiding this comment.
can we add some assertions after this to confirm the popover is rendered?
There was a problem hiding this comment.
still missing the assertions for popover being open
There was a problem hiding this comment.
@TarunAdobe Either you check for the overlay opened or check if the sp-opened is dispatched or not. Either of them will suffice.
There was a problem hiding this comment.
I unresolved the thread because I still don't see the assertion here 😅
| this.target = part.element as HTMLElement; | ||
| newTarget = true; | ||
| } | ||
| this.listenerHost = this.target; |
There was a problem hiding this comment.
are we sure this is the correct default? im trying to understand the override but the strategies and controller complexity is making my brain warp. can we chat about this in our team meeting thursday?
There was a problem hiding this comment.
The type error occurs because this.listenerHost is undefined when reconnected() → init() runs before the overlay is ready.
This happens because the parent class sets this.listenerHost = this.target in update(), child class overrides update() but only sets this.listenerHost inside the async handleOverlayReady callback. If reconnection happens before the overlay exists → this.listenerHost is undefined and it will error out.
this.listenerHost = this.target is not architecturally correct coz the slottable-request event is dispatched from the overlay element with bubbles: false.
Attaching the listener to this.target (the trigger button) means it will never receive the event since it doesn't bubble. This breaks the lazy content loading mechanism entirely.
We can override the reconnected() in the child class to guard against premature initialization, its cleaner since it's specific to the child's async overlay setup pattern.
override reconnected(): void {
// Only call init() if the overlay is ready
if (this.listenerHost) {
this.init();
}
}There was a problem hiding this comment.
good job finding a solution @Rajdeepc !!! I updated the change.
Rajdeepc
dismissed
their stale review
Revisiting coz i see the events don't bubble from parent 'slottable-requestand the listener needs to be on the overlay. there are two ways to do this, we can wait for the overlay to exist before attaching the listener or guard against callinginit()` method.
TarunAdobe
force-pushed
the
ttomar/trigger-directive
branch
from
786ae5d to
0477afd
Compare
|
@TarunAdobe can you include the manual testing steps in the PR description? |
|
When testing overlay from this link: https://swcpreviews.z13.web.core.windows.net/pr-5873/docs/storybook/?path=/story/overlay-directive--default The overlay does not open and im receiving this in the console: |
📚 Branch Preview Links🔍 First Generation Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
WE TOTALLY DIDN'T KNOW BT THIS IS A PROBLEM IN MAIN TOO!! |
|
@caseyisonit the error you saw in the story would be fixed in a separate pr #5896 |
I updated the Managed overlay trigger story implementation so that it now caches the overlay trigger instead of replacing it and thus you can verify this fix from that story (check description again). |
TarunAdobe
force-pushed
the
ttomar/trigger-directive
branch
from
4b22517 to
1c8d9c0
Compare
|
@lehelen19 Could you help validate this in Hz before we move forward with the rollout? It would also be helpful if you could share your specific use case. We can then add it to our Storybook dev instance to reproduce the scenario and run additional tests on our side. This issue appears to be highly use-case specific, so any additional context will help us ensure full coverage. |
|
The bug was triggering errors logged in Splunk, so I don't have a specific use case to share |
TarunAdobe
force-pushed
the
ttomar/trigger-directive
branch
from
d4cfab8 to
bbe64a9
Compare
Thanks for the context. @lehelen19 I want to do a soft test here before we roll out this fix. Can you confirm if this fix works for you or not? |
I don't have a specific test case for this issue. You might be able to remove a component with the overlay directive and then reconnect it to see if you are encountering the error in the console? We should be able to see the related Splunk error disappear once we get the fix in main |
TarunAdobe
force-pushed
the
ttomar/trigger-directive
branch
from
e83e1df to
dae7ed3
Compare
| // Now click the cached trigger to open the overlay. | ||
| const opened = oneEvent(cachedTrigger, 'sp-opened'); | ||
| cachedTrigger.click(); | ||
| await opened; |
There was a problem hiding this comment.
@TarunAdobe Either you check for the overlay opened or check if the sp-opened is dispatched or not. Either of them will suffice.
I have updated the story with a better usecase that is closer to the product requirement. Please check the latest update.
This is a bit nuanced and I don't fully agree if we need to add anything to the documentation as this is a certain pattern that is being used in a certain app. My expectation as a user is that overlay-directive would work for most / all common patterns and we don't all possible usescases of our components either way. |
|
I still see some weirdness Uploading Screen Recording 2026-01-08 at 11.39.19.mov… |
rubencarvalho
left a comment
rubencarvalho
left a comment
There was a problem hiding this comment.
When disabling the quick actions with the menu open, re-enabling it will not make it work again.
TarunAdobe
force-pushed
the
ttomar/trigger-directive
branch
3 times, most recently
from
e878817 to
31101aa
Compare
| // Clear the overlay reference in the strategy so a new one will be created | ||
| // on the next open, which will trigger handleOverlayReady again. | ||
| if (this.strategy) { | ||
| (this.strategy as unknown as { _overlay: undefined })._overlay = |
There was a problem hiding this comment.
Could we consider properly typing _overlay as optional to avoid the cast?
Change private _overlay!: AbstractOverlay; to private _overlay?: AbstractOverlay and update the getter.
And if we really want to be idiomatic, we should add a public clearOverlay() method:
public clearOverlay(): void {
if (this._overlay) {
this._overlay.removeController(this);
}
this._overlay = undefined;
}and then here we instead just call this.strategy?.clearOverlay();. It seems to me like a cleaner approach because it keeps the cleanup logic encapsulated in the controller (where it can also call removeController()).
There was a problem hiding this comment.
lol i like this approach over my comment, we were reviewing at the same time
| override reconnected(): void { | ||
| // If overlay was never created (user never opened it), there's nothing to reconnect. | ||
| // The overlay and listenerHost are only set when handleOverlayReady fires, | ||
| // which happens on first open. Without this guard, init() would fail | ||
| // trying to add a listener to undefined listenerHost. | ||
| // | ||
| // If overlay WAS created before disconnect, we cleared the references in disconnected(), | ||
| // so this.overlay will be undefined and we'll skip init(). A fresh overlay will be | ||
| // created on the next open via handleOverlayReady. |
There was a problem hiding this comment.
Could we maybe can modify this / add a sentence to call out the intention of the empty reconnected override?
IDK, something along the lines of
// Intentionally empty: don't call init() since the overlay reference was cleared in disconnected() and will be recreated on next open.| // Now click the cached trigger to open the overlay. | ||
| const opened = oneEvent(cachedTrigger, 'sp-opened'); | ||
| cachedTrigger.click(); | ||
| await opened; |
There was a problem hiding this comment.
I unresolved the thread because I still don't see the assertion here 😅
caseyisonit
left a comment
caseyisonit
left a comment
There was a problem hiding this comment.
looking so good, works great, but there's a private property override that needs to be adjusted to the public property. i tried to explain what i think a working solution might be but might take some tinkering.
| directive, which preserves the DOM and component state when | ||
| toggled off instead of destroying it. This tests that the | ||
| overlay trigger properly handles reconnection without | ||
| errors. |
There was a problem hiding this comment.
Thank you for this update to this story! its a great example now!
| // Clear the overlay reference in the strategy so a new one will be created | ||
| // on the next open, which will trigger handleOverlayReady again. | ||
| if (this.strategy) { | ||
| (this.strategy as unknown as { _overlay: undefined })._overlay = |
There was a problem hiding this comment.
This is overriding a private property in the interaction controller that is typed as AbstractController, missing undefined thus the need of the typescript.
There is a public overlay property that we can tap into and modify the first return in the setter to _overlay= undefined and add undefined as an accepted type value to the private property. i believe the getter might also need to accept undefined as well.
|
|
||
| private prepareContentRelativeDescription(): void { | ||
| if (!this.overlay) return; | ||
| const overlay = this.overlay; // Capture for closure |
There was a problem hiding this comment.
yus the linter cried otherwise i had missed it
|
@caseyisonit If you get some time today can you please review this. We would like to get this out for PsWeb and give them a alpha release to test |
TarunAdobe
force-pushed
the
ttomar/trigger-directive
branch
from
1e23669 to
3c6c8f8
Compare
caseyisonit
left a comment
caseyisonit
left a comment
There was a problem hiding this comment.
this is working as intended, i did notice a flicker of css styles when clicking the trigger to open (not close) but its negligible so approving!
5 participants
Description
Fixes OverlayTriggerDirective behavior when used with Lit's cache() directive. The directive now properly cleans up overlay state on disconnect and allows fresh overlay creation on reconnect, preventing stale state and runtime errors.
Motivation and context
When using Lit's cache() directive with overlay trigger elements, the directive's lifecycle behaves differently than with standard rendering:
Without
cache():reconnected()never runs on restorationWith
cache():disconnected()andreconnected()lifecycle hooks firereconnected()attempts to reinitialize by callinginit()When a cached trigger element was disconnected and reconnected, the overlay instance remained in a stale state. On reconnect, the overlay reference persisted but:
The overlay DOM element was orphaned or in an inconsistent state
The handleOverlayReady callback wouldn't fire again for the stale overlay
Attempting to reuse the old overlay caused errors and prevented the overlay from opening
Solution:
On disconnected():
Close the overlay to clean up any open state
Remove the overlay element from the DOM
Clear the overlay reference in the strategy by setting _overlay = undefined
On reconnected():
Do nothing - the overlay reference was cleared, so the next click will create a fresh overlay
The handleOverlayReady callback will fire again with the new overlay instance
Everything works as if it's the first time opening
This approach ensures that cached trigger elements always work correctly through multiple hide/show cycles. The strategy's persistent click listeners remain active (as intended), but the overlay is freshly created each time it's needed after a disconnect.
Related issue(s)
Manual Testing Steps
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeatures