GAUD-10113 - Handle disconnected dialog more gracefully#7045
Conversation
…from the DOM before its close animation and functionality have completed
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
| this._removeHandlers(); | ||
| this._state = null; | ||
| this.opened = false; | ||
| if (this._useNative) allowBodyScroll(this); |
There was a problem hiding this comment.
This matches what d2l-backdrop does for the non-native dialog case.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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}">.
There was a problem hiding this comment.
... 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
dlockhart
left a comment
dlockhart
left a comment
There was a problem hiding this comment.
Is this unit testable or is the timing too tricky?
| 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(); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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}">.
3 participants
Right now, dialog doesn't do a great job of cleaning up after itself if it's ripped from the DOM before its close animation happens (which can happen if a dialog actions triggers a navigation).
This came up in the context of native dialogs not re-enabling scroll on the
body, but I also found that it leaves aresizeevent listener on thewindowas well.