Closed Bug 1322939 Opened 7 years ago Closed 3 years ago

Implement proper modal <dialog>

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: ntim, Assigned: sefeng)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [do not land patches until https://github.com/whatwg/html/issues/2268 is solved])

Attachments

(2 files, 3 obsolete files)

Bug 1322939 - Pop/Push dialog to pending dialog stack when needed.
59 bytes, text/x-review-board-request
smaug
: review-
Details
Bug 1322939 - Introduce :-moz-modal-dialog pseudo-selector.
59 bytes, text/x-review-board-request
smaug
: review+
xidorn
: review+
Details 
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>.
Depends on: 1195213
Depends on: 1200896
No longer depends on: 1195213
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)
Whiteboard: [do not land patches until https://github.com/whatwg/html/issues/2268 is solved]
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.
Attachment #8826801 - Flags: review?(bugs)
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 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 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 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
Attachment #8826797 - Flags: review?(bugs) → review-
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()
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.
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.
Priority: -- → P3
Assignee: ntim.bugs → nobody
Status: ASSIGNED → NEW
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.
Attachment #8826797 - Attachment is obsolete: true
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.
Attachment #8826800 - Attachment is obsolete: true
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.
Attachment #8826801 - Attachment is obsolete: true
Depends on: 1580603

The minimum required to fix this would be:

  • start pushing modal dialogs into the top layer (similarly to attachment 8826798 [details], but with the top layer instead of the pending dialog stack, needing bug 1580603 to be fixed first)
  • introduce a :-moz-modal-dialog selector with the relevant UA styles (as attachment 8826799 [details] does, though code probably changed a lot now)
  • fixing UnsetFullscreenElement to not remove the element from the top layer when the dialog is modal (using the event state from the previous part). Not sure what SetFullscreenElement should do in case the element is a modal dialog.

Some essential parts that could be done separately (since this is behind a pref anyway):
bug 1200896 and bug 921504 (edit: the last one is not part of the dialog spec).

Assignee: nobody → sefeng
Depends on: 1634547
Depends on: 1637307
Depends on: 1637310
Type: defect → enhancement
No longer depends on: 1200896
Depends on: 1200896
Depends on: 1681633

Most of this bug has been solved in dependent bugs.

I think bugs regarding dialogs can be tracked on the dialog-element meta.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: