Skip to content
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

Unset is_modal flag when modal dialog is removed from the document #6216

Closed
sefeng211 opened this issue Dec 10, 2020 · 5 comments
Closed

Unset is_modal flag when modal dialog is removed from the document #6216

sefeng211 opened this issue Dec 10, 2020 · 5 comments
Labels
topic: dialog The <dialog> element.

Comments

@sefeng211
Copy link
Contributor

#5936 introduced a side effect that made the modal dialog to keep its modal status when it's being removed from the DOM by using the is modal flag.

This introduces an inconsistency between top layer and modal dialog, as dialog can now have modal status but not in top layer.

This feels weird because we now have two concepts. We can't use the modal status of the dialog to figure out if it's in top layer. In Gecko, now we need to introduce a new internal selector to reflect whether we are in top layer, because we can't use modal status anymore.

How about we also unset it when the element is being removed from the document?

cc @emilio @bfgeek

@josepharhar
Copy link
Contributor

I have a patch for chrome ready, as per the filed chrome bug.
I'll wait for the WPT to land before merging it.

sefeng211 added a commit to sefeng211/wpt that referenced this issue Mar 11, 2021
sefeng211 added a commit to sefeng211/wpt that referenced this issue Mar 12, 2021
@domenic
Copy link
Member

domenic commented Mar 17, 2021

I'm now wondering why we have the "is modal" flag at all, instead of just using "in the top layer" to define the CSS rendering?

I guess because fullscreen also puts the dialog in the top layer, and we don't want fullscreen dialogs to render that way? But does that mean Gecko needs to have a separate flag anyway?

@sefeng211
Copy link
Contributor Author

Yeah, fullscreen is a case, where dialog is in top layer but not modal. And that's right, we don't just have one flag to indicate both top layer and modal status, however, the thing we are trying to fix here is that modal status should always imply top layer. Currently, there's a discrepancy such that dialog can have modal status without being in top layer. Am I clear....?

@domenic
Copy link
Member

domenic commented Mar 17, 2021

Yep, that makes sense. And indeed, unifying would be nice. I'll go poke the PR thread...

domenic pushed a commit to web-platform-tests/wpt that referenced this issue Mar 17, 2021
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 20, 2021
… removed and re-added to the document, a=testonly

Automatic update from web-platform-tests
Dialog should not still be centered when removed and re-added to the document

Follows whatwg/html#6216.
--

wpt-commits: 93b2b92f9fcd41d47fae2def765c23d1d47e6984
wpt-pr: 27968
@josepharhar
Copy link
Contributor

The chromium patch has been merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: dialog The <dialog> element.
Development

No branches or pull requests

4 participants