Modal dialogs should have their ::backdrop element appearing.

We may need to fix one or both of bug 1195213 and bug 1200896 to have modal <dialog>.

Xidorn, so the spec defines both the top layer and the pending dialog stack. Should gecko define both concepts as well or is that currently an issue in the spec?

I failed to find an existing spec issue, so I filed a new one: https://github.com/whatwg/html/issues/2268

I've pushed the patches on mozreview for feedback.

I expect the patches will likely need to change when https://github.com/whatwg/html/issues/2268 is solved. But I'm posting the patches here to get early feedback on the general approach.

Comment on attachment 8826799 [details] Bug 1322939 - Introduce :-moz-modal-dialog pseudo-selector. https://reviewboard.mozilla.org/r/104674/#review105458 Looks fine to me. It seems this patch is unlikely to be affected by the spec issue.

Comment on attachment 8826800 [details] Bug 1322939 - Render pending dialog stack into top layer. https://reviewboard.mozilla.org/r/104676/#review105460 This part seems to be something which would be significantly affected by the spec issue, so cancelling r? for now.

Comment on attachment 8826799 [details] Bug 1322939 - Introduce :-moz-modal-dialog pseudo-selector. https://reviewboard.mozilla.org/r/104674/#review106252 ::: layout/style/res/html.css:833 (Diff revision 1) > > +dialog:-moz-modal-dialog { > + -moz-top-layer: top !important; > +} > + > +dialog::backdrop { blink seems to have a bit different rule for ::backdrop. Does this give similar behavior, or do we need a spec change or what?

Comment on attachment 8826799 [details] Bug 1322939 - Introduce :-moz-modal-dialog pseudo-selector. https://reviewboard.mozilla.org/r/104674/#review106252 > blink seems to have a bit different rule for ::backdrop. > > Does this give similar behavior, or do we need a spec change or what? What do you mean by having a different rule? `::backdrop` pseudo-element would not take effect if the element is not in the top layer.

blink has dialog::backdrop { position: fixed; top: 0; right: 0; bottom: 0; left: 0; background: rgba(0,0,0,0.1) } https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/html.css?q=html.css&sq=package:chromium&dr&l=1090

Oh, hmmm, I think we need them as well.

Note, the spec has only dialog::backdrop { background: rgba(0,0,0,0.1); }

Fullscreen API spec defines the remaining for all ::backdrop... and so does our ua.css. So yeah, the current patch should be fine, then.

Comment on attachment 8826797 [details] Bug 1322939 - Introduce pending dialog stack. https://reviewboard.mozilla.org/r/104670/#review106422 ::: dom/base/nsDocument.cpp:10993 (Diff revision 1) > > +// Pending dialog stack > +bool > +nsDocument::PendingDialogStackPush(Element* aElement) > +{ > + NS_ASSERTION(aElement, "Must pass non-null to PendingDialogStackPush()"); please don't use NS_ASSERTION in new code. Either MOZ_ASSERT or NS_WARN_IF_FALSE ::: dom/base/nsDocument.cpp:11010 (Diff revision 1) > + if (mPendingDialogStack.IsEmpty()) { > + return nullptr; > + } > + uint32_t last = mPendingDialogStack.Length() - 1; > + nsCOMPtr<Element> element(do_QueryReferent(mPendingDialogStack[last])); > + NS_ASSERTION(element, "Should have pending dialog!"); Why here we assume do_QueryReferent returns non-null, but in GetPendingDialogStack we just bypass the entry if it is null. ::: dom/base/nsDocument.cpp:11011 (Diff revision 1) > + return nullptr; > + } > + uint32_t last = mPendingDialogStack.Length() - 1; > + nsCOMPtr<Element> element(do_QueryReferent(mPendingDialogStack[last])); > + NS_ASSERTION(element, "Should have pending dialog!"); > + NS_ASSERTION(element->IsInUncomposedDoc(), "Pending dialog should be in doc"); Why UncomposedDoc() ? Spec allows use of elements in shadow dom. Use *ComposedDoc() ::: dom/base/nsDocument.cpp:11012 (Diff revision 1) > + } > + uint32_t last = mPendingDialogStack.Length() - 1; > + nsCOMPtr<Element> element(do_QueryReferent(mPendingDialogStack[last])); > + NS_ASSERTION(element, "Should have pending dialog!"); > + NS_ASSERTION(element->IsInUncomposedDoc(), "Pending dialog should be in doc"); > + NS_ASSERTION(element->OwnerDoc() == this, "Pending dialog should be in this doc"); also here, use something else than NS_ASSERTION ::: dom/base/nsDocument.cpp:11012 (Diff revision 1) > + } > + uint32_t last = mPendingDialogStack.Length() - 1; > + nsCOMPtr<Element> element(do_QueryReferent(mPendingDialogStack[last])); > + NS_ASSERTION(element, "Should have pending dialog!"); > + NS_ASSERTION(element->IsInUncomposedDoc(), "Pending dialog should be in doc"); > + NS_ASSERTION(element->OwnerDoc() == this, "Pending dialog should be in this doc"); So there is actually some code somewhere ensuring that if dialog from document A is moved to document B, it is first removed from document A's dialog stack? ::: dom/base/nsDocument.cpp:11017 (Diff revision 1) > + NS_ASSERTION(element->OwnerDoc() == this, "Pending dialog should be in this doc"); > + return element; > +} > + > +void > +nsDocument::RemoveDialogFromPendingStack(Element* aDialog) { nit, { goes to its own line after method ::: dom/base/nsDocument.cpp:11020 (Diff revision 1) > + > +void > +nsDocument::RemoveDialogFromPendingStack(Element* aDialog) { > + NS_ASSERTION(dialog, "Should have pending dialog!"); > + NS_ASSERTION(dialog->IsInUncomposedDoc(), "Pending dialog should be in doc"); > + NS_ASSERTION(dialog->OwnerDoc() == this, "Pending dialog should be in this doc"); NS_ASSERTION usage again ::: dom/base/nsDocument.cpp:11041 (Diff revision 1) > + for (const nsWeakPtr& ptr : mPendingDialogStack) { > + if (nsCOMPtr<Element> elem = do_QueryReferent(ptr)) { > + elements.AppendElement(elem); > + } > + } > + return elements; Could you possibly return Move(elements) or something to avoid extra copy of the array's contents

If I read the spec right, the same dialog should be added twice to the stack in case dialog.showModal(); dialog.removeAttribute("show"); dialog.showModal();

Comment on attachment 8826798 [details] Bug 1322939 - Pop/Push dialog to pending dialog stack when needed. https://reviewboard.mozilla.org/r/104672/#review106432 We're still missing the following part of the spec "If at any time a dialog element is removed from a Document, then if that dialog is in that Document's pending dialog stack, the following steps must be run:" ::: dom/html/HTMLDialogElement.cpp:63 (Diff revision 1) > } > ErrorResult ignored; > SetOpen(false, ignored); > ignored.SuppressException(); > + > + if (mIsModal) { Why do we need mIsModal? The spec doesn't have such check. The spec just says "If subject is in its Document's pending dialog stack, then run these substeps:" ::: dom/html/HTMLDialogElement.cpp:64 (Diff revision 1) > ErrorResult ignored; > SetOpen(false, ignored); > ignored.SuppressException(); > + > + if (mIsModal) { > + nsDocument* doc = static_cast<nsDocument*>(GetUncomposedDoc()); GetComposedDoc() ::: dom/html/HTMLDialogElement.cpp:98 (Diff revision 1) > + mIsModal = true; > + > SetOpen(true, aError); > aError.SuppressException(); > + > + nsDocument* doc = static_cast<nsDocument*>(GetUncomposedDoc()); GetComposedDoc()

(In reply to Tim Nguyen :ntim from comment #10) > I expect the patches will likely need to change when > https://github.com/whatwg/html/issues/2268 is solved. > > But I'm posting the patches here to get early feedback on the general > approach. It seems the editor domenic agrees with this change, so I think we can move forward with that proposal.

Would you consider implementing the `inert` attribute as part of this change? https://github.com/WICG/inert#spec I am in the process of implementing this for Chrome: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/N--HhuYFJQI/bmwhhHDZCQAJ https://codereview.chromium.org/2088453002/

(In reply to aboxhall from comment #24) > Would you consider implementing the `inert` attribute as part of this > change? > > https://github.com/WICG/inert#spec > > I am in the process of implementing this for Chrome: > https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/N--HhuYFJQI/ > bmwhhHDZCQAJ > https://codereview.chromium.org/2088453002/ Sure, it should be easy to do so.

(In reply to Tim Nguyen :ntim from comment #25) > (In reply to aboxhall from comment #24) > > Would you consider implementing the `inert` attribute as part of this > > change? > > > > https://github.com/WICG/inert#spec > > > > I am in the process of implementing this for Chrome: > > https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/N--HhuYFJQI/ > > bmwhhHDZCQAJ > > https://codereview.chromium.org/2088453002/ > > Sure, it should be easy to do so. This is bug 921504. I've cc'ed you there.

Assuming P3 due to lack of recent activity. Feel free to correct me.

The spec issue (https://github.com/whatwg/html/issues/2268) which was preventing patches being landed appears to have been resolved

Comment on attachment 8826797 [details] Bug 1322939 - Introduce pending dialog stack. There's no more pending dialog stack in the spec, it's now the top layer.

Comment on attachment 8826800 [details] Bug 1322939 - Render pending dialog stack into top layer. This is unnecessary since there's only a top layer now.

Comment on attachment 8826801 [details] Bug 1322939 - Implement inert subtrees. The CSS implementation is wrong. Would probably need some layout code to do this properly.