Skip to content

GAUD-10113 - Handle disconnected dialog more gracefully #7045

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.

Sign up for GitHub

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

Open
svanherk wants to merge 1 commit into
base: main
Choose a base branch
from
+6 −0
Open

GAUD-10113 - Handle disconnected dialog more gracefully #7045

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions components/dialog/dialog-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ export const DialogMixin = superclass => class extends superclass {
disconnectedCallback() {
super.disconnectedCallback();
window.removeEventListener('d2l-mvc-dialog-open', this._handleMvcDialogOpen);

// remove handlers, reset state and allow body scrolling if dialog is removed from the DOM before fully closing
this._removeHandlers();
Copy link
Copy Markdown
Contributor Author

@svanherk svanherk Jun 4, 2026

Choose a reason for hiding this comment

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

This could arguably just be window.removeEventListener('resize', this._updateSize);, but I figured it might be better to call the same function as _handleClose in case more window-level ones were added someday

Copy link
Copy Markdown
Member

@dlockhart dlockhart Jun 5, 2026

Choose a reason for hiding this comment

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

I wonder if it would be worth putting what you've added here into something like a #doCloseCleanup() that gets called from here and _handleClose? Just in case we add more to _handleClose it'll at least force the conversation about whether that belongs in the central place or not.

this._state = null;
this.opened = false;
Comment on lines +113 to +114
Copy link
Copy Markdown
Contributor Author

@svanherk svanherk Jun 4, 2026

Choose a reason for hiding this comment

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

I'm not really sure if these are needed - I can't see a case where you've navigated away and the same component is later somehow re-connected. But maybe in a non-navigation case where a dialog was removed forcefully but later added back?

Copy link
Copy Markdown
Contributor

@dbatiste dbatiste Jun 5, 2026

Choose a reason for hiding this comment

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

Yeah idk... it's kinda of a weird case. In theory, someone could remove the dialog from the DOM in the open state, and then reconnect it, and I'd usually expect the component to be in the same state as when it was disconnected. But in practice, I'm not really expecting anyone to do that.

Copy link
Copy Markdown
Member

@dlockhart dlockhart Jun 5, 2026

Choose a reason for hiding this comment

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

Messing with opened without dispatching d2l-dialog-close worries me a little since consumers often sync their own reactive state _isSomeDialogOpened tied to <d2l-dialog ?opened="${this._isSomeDialogOpened}">.

Copy link
Copy Markdown
Contributor

@dbatiste dbatiste Jun 5, 2026

Choose a reason for hiding this comment

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

... I think that general behaviour of whether to maintain state depends on the property. I can probably think of case that would go either way on that. In this case though, it's probably fine to put the dialog into the closed state when it's disconnected.

Copy link
Copy Markdown
Contributor

@dbatiste dbatiste Jun 5, 2026

Choose a reason for hiding this comment

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

Messing with opened without dispatching d2l-dialog-close worries me a little since consumers often sync their own reactive state _isSomeDialogOpened tied to <d2l-dialog ?opened="${this._isSomeDialogOpened}">.

I think if opened changes, the _handleClose method would get called, which dispatches the d2l-dialog-close event that they use to sync their state. So it's probably ok, but I share the concern.

Copy link
Copy Markdown
Contributor Author

@svanherk svanherk Jun 5, 2026

Choose a reason for hiding this comment

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

Yeah, the main reason I thought to set opened to false was because setting it back to true is what causes _addHandlers to run. So if we're removing the handlers, and then the dialog gets reconnected as is, it's gonna be a bit broken. But I'm not confident that's not still the case with what I have here anyways

if (this._useNative) allowBodyScroll(this);
Copy link
Copy Markdown
Contributor Author

@svanherk svanherk Jun 4, 2026

Choose a reason for hiding this comment

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

This matches what d2l-backdrop does for the non-native dialog case.

Copy link
Copy Markdown
Contributor

@dbatiste dbatiste Jun 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The _handleClose calls this. Maybe we should just be setting this.opened = false; here and letting the rest of the dialog workflow do the cleanup/reset.

Copy link
Copy Markdown
Contributor Author

@svanherk svanherk Jun 5, 2026

Choose a reason for hiding this comment

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

So the issue is that the animationEnd event never happens when navigating away, so the actual close (_handleClose) doesn't happen. this.opened is actually already false by the time we get to disconnectedCallback in the case I'm testing, so maybe another reason to not bother setting it.

}

async updated(changedProperties) {
Expand Down