Last Comment Bug 1322939 - Implement proper modal <dialog>
: Implement proper modal <dialog>
Status: ASSIGNED
[do not land patches until https://gi...
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal with 4 votes (vote)
: ---
Assigned To: Tim Nguyen :ntim
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 1200896 ::backdrop 1236828
Blocks: dialog-element
  Show dependency treegraph
 
Reported: 2016-12-11 14:05 PST by Tim Nguyen :ntim
Modified: 2017-02-25 08:15 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
Bug 1322939 - Introduce pending dialog stack. (59 bytes, text/x-review-board-request)
2017-01-13 17:51 PST, Tim Nguyen :ntim
bugs: review-
Details | Review
Bug 1322939 - Pop/Push dialog to pending dialog stack when needed. (59 bytes, text/x-review-board-request)
2017-01-13 17:51 PST, Tim Nguyen :ntim
bugs: review-
Details | Review
Bug 1322939 - Introduce :-moz-modal-dialog pseudo-selector. (59 bytes, text/x-review-board-request)
2017-01-13 17:51 PST, Tim Nguyen :ntim
bugs: review+
xidorn+moz: review+
Details | Review
Bug 1322939 - Render pending dialog stack into top layer. (59 bytes, text/x-review-board-request)
2017-01-13 17:51 PST, Tim Nguyen :ntim
no flags Details | Review
Bug 1322939 - Implement inert subtrees. (59 bytes, text/x-review-board-request)
2017-01-13 17:51 PST, Tim Nguyen :ntim
no flags Details | Review

Description User image Tim Nguyen :ntim 2016-12-11 14:05:29 PST
Modal dialogs should have their ::backdrop element appearing.
Comment 1 User image Xidorn Quan [:xidorn] (UTC+10) 2016-12-11 14:53:08 PST
We may need to fix one or both of bug 1195213 and bug 1200896 to have modal <dialog>.
Comment 2 User image Tim Nguyen :ntim 2017-01-13 14:03:54 PST
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?
Comment 3 User image Xidorn Quan [:xidorn] (UTC+10) 2017-01-13 16:40:37 PST
I failed to find an existing spec issue, so I filed a new one: https://github.com/whatwg/html/issues/2268
Comment 4 User image Tim Nguyen :ntim 2017-01-13 17:51:29 PST Comment hidden (mozreview-request)
Comment 5 User image Tim Nguyen :ntim 2017-01-13 17:51:29 PST Comment hidden (mozreview-request)
Comment 6 User image Tim Nguyen :ntim 2017-01-13 17:51:29 PST Comment hidden (mozreview-request)
Comment 7 User image Tim Nguyen :ntim 2017-01-13 17:51:29 PST Comment hidden (mozreview-request)
Comment 8 User image Tim Nguyen :ntim 2017-01-13 17:51:29 PST Comment hidden (mozreview-request)
Comment 9 User image Tim Nguyen :ntim 2017-01-13 17:53:40 PST
I've pushed the patches on mozreview for feedback.
Comment 10 User image Tim Nguyen :ntim 2017-01-13 17:55:10 PST
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 11 User image Tim Nguyen :ntim 2017-01-13 18:21:20 PST Comment hidden (mozreview-request)
Comment 12 User image Xidorn Quan [:xidorn] (UTC+10) 2017-01-14 23:22:14 PST
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 13 User image Xidorn Quan [:xidorn] (UTC+10) 2017-01-14 23:25:35 PST
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 14 User image Olli Pettay [:smaug] 2017-01-18 03:12:01 PST
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 15 User image Xidorn Quan [:xidorn] (UTC+10) 2017-01-18 03:13:32 PST
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 User image Olli Pettay [:smaug] 2017-01-18 03:27:32 PST
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
Comment 17 User image Xidorn Quan [:xidorn] (UTC+10) 2017-01-18 03:29:53 PST
Oh, hmmm, I think we need them as well.
Comment 18 User image Olli Pettay [:smaug] 2017-01-18 03:31:54 PST
Note, the spec has only
dialog::backdrop {
  background: rgba(0,0,0,0.1);
}
Comment 19 User image Xidorn Quan [:xidorn] (UTC+10) 2017-01-18 03:37:29 PST
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 User image Olli Pettay [:smaug] 2017-01-18 12:38:54 PST
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
Comment 21 User image Olli Pettay [:smaug] 2017-01-18 12:45:09 PST
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 User image Olli Pettay [:smaug] 2017-01-18 12:51:39 PST
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()
Comment 23 User image Xidorn Quan [:xidorn] (UTC+10) 2017-01-20 20:01:38 PST
(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 User image aboxhall 2017-02-15 20:51:15 PST
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/
Comment 25 User image Tim Nguyen :ntim 2017-02-25 08:15:40 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.