Implement proper modal <dialog>

ASSIGNED
Assigned to

Status

()

Core
DOM: Core & HTML
ASSIGNED
7 months ago
29 days ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [do not land patches until https://github.com/whatwg/html/issues/2268 is solved])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

7 months ago
Modal dialogs should have their ::backdrop element appearing.
Depends on: 1236828, 1064843
We may need to fix one or both of bug 1195213 and bug 1200896 to have modal <dialog>.
Depends on: 1195213
(Assignee)

Updated

6 months ago
Depends on: 1200896
(Assignee)

Updated

6 months ago
No longer depends on: 1195213
Keywords: dev-doc-needed
(Assignee)

Comment 2

6 months ago
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?
Flags: needinfo?(xidorn+moz)
I failed to find an existing spec issue, so I filed a new one: https://github.com/whatwg/html/issues/2268
Flags: needinfo?(xidorn+moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Whiteboard: [do not land patches until https://github.com/whatwg/html/issues/2268 is solved]
(Assignee)

Comment 9

6 months ago
I've pushed the patches on mozreview for feedback.
(Assignee)

Comment 10

6 months ago
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 hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8826801 - Flags: review?(bugs)

Comment 12

5 months ago
mozreview-review
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.
Attachment #8826799 - Flags: review?(xidorn+moz) → review+

Comment 13

5 months ago
mozreview-review
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.
Attachment #8826800 - Flags: review?(xidorn+moz)

Comment 14

5 months ago
mozreview-review
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?
Attachment #8826799 - Flags: review?(bugs) → review+

Comment 15

5 months ago
mozreview-review-reply
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.

Comment 16

5 months ago
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.

Comment 18

5 months ago
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 20

5 months ago
mozreview-review
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
Attachment #8826797 - Flags: review?(bugs) → review-

Comment 21

5 months ago
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 22

5 months ago
mozreview-review
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()
Attachment #8826798 - Flags: review?(bugs) → review-
(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.

Comment 24

4 months ago
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/
(Assignee)

Comment 25

4 months ago
(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.
You need to log in before you can comment on or make changes to this bug.