Last Comment Bug 1322938 - Basic implementation of HTMLDialogElement
: Basic implementation of HTMLDialogElement
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: Unspecified Unspecified
: -- normal with 2 votes (vote)
: mozilla53
Assigned To: Tim Nguyen :ntim
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 1328321
Blocks: dialog-element
  Show dependency treegraph
 
Reported: 2016-12-11 14:00 PST by Tim Nguyen :ntim
Modified: 2017-01-16 23:26 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed

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

Attachments
Bug 1322938 - Basic implementation of HTMLDialogElement. (58 bytes, text/x-review-board-request)
2016-12-12 02:58 PST, Tim Nguyen :ntim
bugs: review+
masayuki: review+
Details | Review
Bug 1322938 - Emit close event when HTMLDialogElement.prototype.close() is called. (58 bytes, text/x-review-board-request)
2016-12-12 02:58 PST, Tim Nguyen :ntim
bugs: review+
Details | Review
Bug 1322938 - Update <dialog> element Web Platform Tests expected results. (58 bytes, text/x-review-board-request)
2016-12-12 02:58 PST, Tim Nguyen :ntim
bugs: review+
Details | Review
Bug 1322938 - Put <dialog> element behind preference. (58 bytes, text/x-review-board-request)
2016-12-20 12:00 PST, Tim Nguyen :ntim
bugs: review+
Details | Review
Bug 1322938 - Test whether dom.dialog_element.enabled works properly. (59 bytes, text/x-review-board-request)
2016-12-20 17:28 PST, Tim Nguyen :ntim
bugs: review+
Details | Review
Parser changes (102.13 KB, patch)
2016-12-22 01:40 PST, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
htmlparser repo patch (34.92 KB, patch)
2016-12-22 01:44 PST, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
Bug 1322938 - Make the HTML tree builder aware of <dialog>. (59 bytes, text/x-review-board-request)
2016-12-22 12:20 PST, Tim Nguyen :ntim
wchen: review+
Details | Review

Description User image Tim Nguyen :ntim 2016-12-11 14:00:59 PST
This bug is about implementing a basic implementation of HTMLDialogElement.

This doesn't include:
- Anchoring
- Modal dialogs
Comment 1 User image Tim Nguyen :ntim 2016-12-11 16:14:50 PST
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #0)
> This bug is about implementing a basic implementation of HTMLDialogElement.
> 
> This doesn't include:
> - Anchoring
Anchoring is getting removed from the spec (https://github.com/whatwg/html/pull/2158 ), and Chrome doesn't implement it. So this isn't going to be implemented.

> - Modal dialogs
are covered by bug 1322939
Comment 2 User image Tim Nguyen :ntim 2016-12-11 16:39:25 PST

PR to web-platform-tests to remove anchoring:
https://github.com/w3c/web-platform-tests/pull/4312
Comment 3 User image Tim Nguyen :ntim 2016-12-12 02:58:42 PST Comment hidden (mozreview-request)
Comment 4 User image Tim Nguyen :ntim 2016-12-12 02:58:42 PST Comment hidden (mozreview-request)
Comment 5 User image Tim Nguyen :ntim 2016-12-12 02:58:42 PST Comment hidden (mozreview-request)
Comment 6 User image Tim Nguyen :ntim 2016-12-12 03:00:49 PST Comment hidden (mozreview-request)
Comment 7 User image :Ehsan Akhgari 2016-12-13 10:52:24 PST
Tim, can you please clarify which parts of part 1 you'd like me to review?  Thank you!

Also, have you sent an intent to implement (or ship) email for this feature?
Comment 8 User image Tim Nguyen :ntim 2016-12-13 11:18:40 PST
(In reply to :Ehsan Akhgari from comment #7)
> Tim, can you please clarify which parts of part 1 you'd like me to review? 
> Thank you!
I've requested review for the editor bits, but I wouldn't mind some comments on the other bits as well.

> Also, have you sent an intent to implement (or ship) email for this feature?
Yes, but it's still awaiting approval from the dev-platform list moderators.
Comment 9 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-12-14 07:01:49 PST
Comment on attachment 8817970 [details]
Bug 1322938 - Emit close event when HTMLDialogElement.prototype.close() is called.

https://reviewboard.mozilla.org/r/98150/#review98850

Worth to test what other browsers do with close event, but at least the spec doesn't want this behavior.

::: dom/html/HTMLDialogElement.h:54
(Diff revision 1)
>  protected:
>    virtual ~HTMLDialogElement();
>    JSObject* WrapNode(JSContext* aCx,
>                       JS::Handle<JSObject*> aGivenProto) override;
> +
> +  RefPtr<AsyncEventDispatcher> mEventDispatcher;

Why do we need this member variable?

::: dom/html/HTMLDialogElement.cpp:32
(Diff revision 1)
>      SetReturnValue(aReturnValue.Value());
>    }
>    ErrorResult ignored;
>    SetOpen(false, ignored);
>    ignored.SuppressException();
> +

Not about this patch, but looks like the implementation is still missing "pending dialog stack" concept.

::: dom/html/HTMLDialogElement.cpp:34
(Diff revision 1)
>    ErrorResult ignored;
>    SetOpen(false, ignored);
>    ignored.SuppressException();
> +
> +  if (mEventDispatcher) {
> +    mEventDispatcher->Cancel();

Nothing in the spec says dispatching could be cancelled. So as far as I see, mEventDispatcher->Cancel() call here should be removed
Comment 10 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-12-14 07:17:03 PST
Comment on attachment 8817971 [details]
Bug 1322938 - Update <dialog> element Web Platform Tests expected results.

https://reviewboard.mozilla.org/r/98152/#review98854

FWIW, this is a case where MozReview fails badly. It has effectively dataloss issues when a patch is removing files.
Comment 11 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-12-14 07:20:11 PST
Comment on attachment 8817971 [details]
Bug 1322938 - Update <dialog> element Web Platform Tests expected results.

https://reviewboard.mozilla.org/r/98152/#review98856

removal of dialog-close.html.ini and the patch for close event hints that there aren't enough tests for close event yet.
Could you please add some to ensure the right behavior when dialog is opened and closed several times and there should be right number of close events.
But that is about some other patch. This one should be fine.
Comment 12 User image :Ehsan Akhgari 2016-12-14 09:08:59 PST
Comment on attachment 8817969 [details]
Bug 1322938 - Basic implementation of HTMLDialogElement.

I don't review editor code any more.  Editor reviews go to Masayuki now.
Comment 13 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-12-14 11:23:03 PST
Comment on attachment 8817969 [details]
Bug 1322938 - Basic implementation of HTMLDialogElement.

https://reviewboard.mozilla.org/r/98148/#review98876

Don't you need to update also parser/html/* stuff? You may want to ping hsivonen.

::: dom/html/HTMLDialogElement.cpp:22
(Diff revision 1)
> +}
> +
> +NS_IMPL_ELEMENT_CLONE(HTMLDialogElement)
> +
> +void
> +HTMLDialogElement::Close(const mozilla::dom::Optional<nsAString>& aReturnValue) {

Coding style is that { after method's param list should go to its own line.
Same thing everywhere in this patch.

::: dom/html/HTMLDialogElement.cpp:33
(Diff revision 1)
> +  ignored.SuppressException();
> +}
> +
> +void
> +HTMLDialogElement::Show() {
> +  ErrorResult ignored;

Show() should return early if open is already set

::: dom/html/HTMLDialogElement.cpp:40
(Diff revision 1)
> +  ignored.SuppressException();
> +}
> +
> +void
> +HTMLDialogElement::ShowModal(ErrorResult& aError) {
> +  if (!IsInUncomposedDoc() || Open()) {

FYI, I filed https://bugs.chromium.org/p/chromium/issues/detail?id=674217 , but it is possible that the spec will get changed, in which case using IsInComposed() doc would be the right thing.

::: dom/html/HTMLDialogElement.cpp:46
(Diff revision 1)
> +   aError.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> +   return;
> +  }
> +
> +  SetOpen(true, aError);
> +  aError.SuppressException();

Ok, so this is still missing several step from the spec. I assume those will be implemented in separate patches or bugs.

::: dom/webidl/HTMLDialogElement.webidl:14
(Diff revision 1)
> + * © Copyright 2004-2011 Apple Computer, Inc., Mozilla Foundation, and
> + * Opera Software ASA. You are granted a license to use, reproduce
> + * and create derivative works of this document.
> + */
> +
> +interface HTMLDialogElement : HTMLElement {

We want this initially behind a pref since the implementation will probably need some work before shipping.
Comment 14 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-12-14 11:23:54 PST
Comment on attachment 8817971 [details]
Bug 1322938 - Update <dialog> element Web Platform Tests expected results.

https://reviewboard.mozilla.org/r/98152/#review98924

Actually, if we put the element behind a pref, we should use the pref in the .inis for now I think.
Comment 15 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-12-14 11:25:05 PST
Comment on attachment 8817969 [details]
Bug 1322938 - Basic implementation of HTMLDialogElement.

https://reviewboard.mozilla.org/r/98148/#review98926
Comment 16 User image Tim Nguyen :ntim 2016-12-20 12:00:18 PST Comment hidden (mozreview-request)
Comment 17 User image Tim Nguyen :ntim 2016-12-20 12:00:18 PST Comment hidden (mozreview-request)
Comment 18 User image Tim Nguyen :ntim 2016-12-20 12:00:18 PST Comment hidden (mozreview-request)
Comment 19 User image Tim Nguyen :ntim 2016-12-20 12:00:18 PST Comment hidden (mozreview-request)
Comment 20 User image Tim Nguyen :ntim 2016-12-20 12:06:30 PST Comment hidden (mozreview-request)
Comment 21 User image Tim Nguyen :ntim 2016-12-20 12:10:32 PST
Comment on attachment 8817969 [details]
Bug 1322938 - Basic implementation of HTMLDialogElement.

https://reviewboard.mozilla.org/r/98148/#review98876

Done

> Coding style is that { after method's param list should go to its own line.
> Same thing everywhere in this patch.

Fixed

> Show() should return early if open is already set

Fixed

> Ok, so this is still missing several step from the spec. I assume those will be implemented in separate patches or bugs.

Yes, bug 1322939 will take care of those
Comment 22 User image Tim Nguyen :ntim 2016-12-20 12:12:10 PST
Comment on attachment 8817970 [details]
Bug 1322938 - Emit close event when HTMLDialogElement.prototype.close() is called.

https://reviewboard.mozilla.org/r/98150/#review98850

> Why do we need this member variable?

It's for AsyncEventRunning

> Not about this patch, but looks like the implementation is still missing "pending dialog stack" concept.

Yep, the implementation of modal dialogs will take care of it
Comment 23 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-12-20 12:53:54 PST
Comment on attachment 8817969 [details]
Bug 1322938 - Basic implementation of HTMLDialogElement.

https://reviewboard.mozilla.org/r/98148/#review100340

Those fixed, but all the parsing handling needs to be reviewed by hsivonen. I didn't really look at it.
(I see there is a followup patch to put things behind a pref)

::: dom/html/HTMLDialogElement.h:35
(Diff revision 2)
> +  void SetOpen(bool aOpen, ErrorResult& aError)
> +  {
> +    SetHTMLBoolAttr(nsGkAtoms::open, aOpen, aError);
> +  }
> +
> +  void GetReturnValue(nsAString& aReturnValue) {

{ to its own line here and elsewhere with methods


(the only special case is when a method is super short so that both {} and the content fit into one line)

::: dom/html/HTMLDialogElement.cpp:49
(Diff revision 2)
> +}
> +
> +void
> +HTMLDialogElement::ShowModal(ErrorResult& aError)
> +{
> +  if (!IsInUncomposedDoc() || Open()) {

The spec has changed, and the check should be now IsInComposedDoc(), not IsInUncomposedDoc()

::: dom/webidl/HTMLDialogElement.webidl:14
(Diff revision 2)
> + * © Copyright 2004-2011 Apple Computer, Inc., Mozilla Foundation, and
> + * Opera Software ASA. You are granted a license to use, reproduce
> + * and create derivative works of this document.
> + */
> +
> +interface HTMLDialogElement : HTMLElement {

So I think we want this behind a pref initially (possibly enabled on Nightly, but disabled elsewhere). That means also document.createElement("dialog") should be controlled by the pref.

::: layout/style/res/ua.css:337
(Diff revision 2)
> +
> +dialog {
> +  position: absolute;
> +  left: 0px;
> +  right: 0px;
> +  color: -moz-dialogText;

Why -moz-dialogText? The spec uses just black.

::: layout/style/res/ua.css:344
(Diff revision 2)
> +  border-width: initial;
> +  border-style: solid;
> +  border-color: initial;
> +  border-image: initial;
> +  padding: 1em;
> +  background: -moz-dialog;

The spec uses white for background. Why different value here? Do other browsers not follow the spec here? If so, should the spec be changed?

At least blink seems to follow the spec here
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/html.css?q=html.css&sq=package:chromium&dr&l=1090
Comment 24 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-12-20 12:56:55 PST
Comment on attachment 8817970 [details]
Bug 1322938 - Emit close event when HTMLDialogElement.prototype.close() is called.

https://reviewboard.mozilla.org/r/98150/#review100354

::: dom/html/HTMLDialogElement.h:54
(Diff revision 2)
>  protected:
>    virtual ~HTMLDialogElement();
>    JSObject* WrapNode(JSContext* aCx,
>                       JS::Handle<JSObject*> aGivenProto) override;
> +
> +  RefPtr<AsyncEventDispatcher> mEventDispatcher;

You don't need this for anything.

::: dom/html/HTMLDialogElement.cpp:33
(Diff revision 2)
>      SetReturnValue(aReturnValue.Value());
>    }
>    ErrorResult ignored;
>    SetOpen(false, ignored);
>    ignored.SuppressException();
> +  mEventDispatcher =

there is no need to assign AsyncEventDispatcher to a local member variable.

::: dom/html/HTMLDialogElement.cpp:62
(Diff revision 2)
>    SetOpen(true, aError);
>    aError.SuppressException();
>  }
>  
> +void
> +HTMLDialogElement::AsyncEventRunning(AsyncEventDispatcher* aEvent)

I don't see need for this method
Comment 25 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-12-20 13:00:18 PST
Comment on attachment 8817971 [details]
Bug 1322938 - Update <dialog> element Web Platform Tests expected results.

https://reviewboard.mozilla.org/r/98152/#review100356

Can we really remove those couple of .ini files if we put this stuff behind a pref?
I thought wpt tests require prefs to be set in .inis.

That answered, r+
Comment 26 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-12-20 13:03:43 PST
Comment on attachment 8820380 [details]
Bug 1322938 - Put <dialog> element behind preference.

https://reviewboard.mozilla.org/r/99884/#review100360

Ah, you add the .inis back here.

One thing, could you possibly add a small mochitest that when the pref is false, not parser nor document.createElement can create dialog elements (but HTMLUnknownElements)
We've had issues before where we accidentally exposed element creation. So, an additional patch?
Comment 27 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-12-20 13:07:18 PST
Comment on attachment 8817969 [details]
Bug 1322938 - Basic implementation of HTMLDialogElement.

https://reviewboard.mozilla.org/r/98148/#review100364

::: dom/html/HTMLDialogElement.h:27
(Diff revision 2)
> +  }
> +
> +  NS_IMPL_FROMCONTENT_HTML_WITH_TAG(HTMLDialogElement, dialog)
> +
> +  virtual nsresult Clone(mozilla::dom::NodeInfo* aNodeInfo, nsINode** aResult) const override;
> +  nsString mReturnValue;

Could you put mReturnValue at the end of the class declaration, not somewhere between.

::: dom/html/HTMLDialogElement.h:48
(Diff revision 2)
> +  void Show();
> +  void ShowModal(ErrorResult& aError);
> +
> +protected:
> +  virtual ~HTMLDialogElement();
> +  JSObject* WrapNode(JSContext* aCx,

Why WrapNode is protected?
Comment 28 User image Tim Nguyen :ntim 2016-12-20 16:07:08 PST
(In reply to Olli Pettay [:smaug] from comment #27)
> Comment on attachment 8817969 [details]
> Bug 1322938 - Basic implementation of HTMLDialogElement.
> 
> https://reviewboard.mozilla.org/r/98148/#review100364
> 
> ::: dom/html/HTMLDialogElement.h:27
> (Diff revision 2)
> > +  }
> > +
> > +  NS_IMPL_FROMCONTENT_HTML_WITH_TAG(HTMLDialogElement, dialog)
> > +
> > +  virtual nsresult Clone(mozilla::dom::NodeInfo* aNodeInfo, nsINode** aResult) const override;
> > +  nsString mReturnValue;
> 
> Could you put mReturnValue at the end of the class declaration, not
> somewhere between.
Will do.

> ::: dom/html/HTMLDialogElement.h:48
> (Diff revision 2)
> > +  void Show();
> > +  void ShowModal(ErrorResult& aError);
> > +
> > +protected:
> > +  virtual ~HTMLDialogElement();
> > +  JSObject* WrapNode(JSContext* aCx,
> 
> Why WrapNode is protected?
They all look protected in other interfaces: https://dxr.mozilla.org/mozilla-central/search?q=WrapNode+path%3Adom%2Fhtml&redirect=false

I think it makes sense because we don't want to expose it to other code, and this overrides WrapNode from a derived class, so protected makes sense.
Comment 29 User image Tim Nguyen :ntim 2016-12-20 16:23:54 PST
(In reply to Olli Pettay [:smaug] from comment #23)
> Comment on attachment 8817969 [details]
> Bug 1322938 - Basic implementation of HTMLDialogElement.
> 
> https://reviewboard.mozilla.org/r/98148/#review100340
> 
> Those fixed, but all the parsing handling needs to be reviewed by hsivonen.
> I didn't really look at it.
> (I see there is a followup patch to put things behind a pref)
> 
> ::: dom/html/HTMLDialogElement.h:35
> (Diff revision 2)
> > +  void SetOpen(bool aOpen, ErrorResult& aError)
> > +  {
> > +    SetHTMLBoolAttr(nsGkAtoms::open, aOpen, aError);
> > +  }
> > +
> > +  void GetReturnValue(nsAString& aReturnValue) {
> 
> { to its own line here and elsewhere with methods
> 
> 
> (the only special case is when a method is super short so that both {} and
> the content fit into one line)
Fixed.

> ::: dom/html/HTMLDialogElement.cpp:49
> (Diff revision 2)
> > +}
> > +
> > +void
> > +HTMLDialogElement::ShowModal(ErrorResult& aError)
> > +{
> > +  if (!IsInUncomposedDoc() || Open()) {
> 
> The spec has changed, and the check should be now IsInComposedDoc(), not
> IsInUncomposedDoc()
> 
Fixed.

> ::: layout/style/res/ua.css:337
> (Diff revision 2)
> > +
> > +dialog {
> > +  position: absolute;
> > +  left: 0px;
> > +  right: 0px;
> > +  color: -moz-dialogText;
> 
> Why -moz-dialogText? The spec uses just black.
>
> ::: layout/style/res/ua.css:344
> (Diff revision 2)
> > +  border-width: initial;
> > +  border-style: solid;
> > +  border-color: initial;
> > +  border-image: initial;
> > +  padding: 1em;
> > +  background: -moz-dialog;
> 
> The spec uses white for background. Why different value here? Do other
> browsers not follow the spec here? If so, should the spec be changed?
> 
> At least blink seems to follow the spec here
> https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/html.
> css?q=html.css&sq=package:chromium&dr&l=1090
-moz-dialog and -moz-dialogText are accessibility-friendly colors (they work well with high-contrast themes). But I don't have a strong opinion. I'll follow what Blink does so we don't get different colors across browsers.
Comment 30 User image Tim Nguyen :ntim 2016-12-20 17:28:07 PST Comment hidden (mozreview-request)
Comment 31 User image Tim Nguyen :ntim 2016-12-20 17:28:07 PST Comment hidden (mozreview-request)
Comment 32 User image Tim Nguyen :ntim 2016-12-20 17:28:07 PST Comment hidden (mozreview-request)
Comment 33 User image Tim Nguyen :ntim 2016-12-20 17:28:07 PST Comment hidden (mozreview-request)
Comment 34 User image Tim Nguyen :ntim 2016-12-20 17:28:07 PST Comment hidden (mozreview-request)
Comment 35 User image Tim Nguyen :ntim 2016-12-20 17:48:44 PST Comment hidden (mozreview-request)
Comment 36 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-12-21 02:49:56 PST
Comment on attachment 8817970 [details]
Bug 1322938 - Emit close event when HTMLDialogElement.prototype.close() is called.

https://reviewboard.mozilla.org/r/98150/#review100584
Comment 37 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2016-12-21 02:51:07 PST
Comment on attachment 8820495 [details]
Bug 1322938 - Test whether dom.dialog_element.enabled works properly.

https://reviewboard.mozilla.org/r/100002/#review100586

Thanks, and this can be changed or removed later when we enable the pref by default.
Comment 38 User image Tim Nguyen :ntim 2016-12-21 03:37:33 PST
Olli, thanks for the reviews!

Masayuki, can you review the editor bits please? 
Henri, can you review the parser bits please?

Thank you.
Comment 39 User image Henri Sivonen (:hsivonen) 2016-12-22 01:34:39 PST
Comment on attachment 8817969 [details]
Bug 1322938 - Basic implementation of HTMLDialogElement.

https://reviewboard.mozilla.org/r/98148/#review100992

::: parser/html/nsHtml5AtomList.h:870
(Diff revision 3)
>  HTML5_ATOM(circle, "circle")
>  HTML5_ATOM(center, "center")
>  HTML5_ATOM(canvas, "canvas")
>  HTML5_ATOM(divide, "divide")
>  HTML5_ATOM(degree, "degree")
> +HTML5_ATOM(dialog, "dialog")

This is a generated file. Please don't edit it manually. This patch is missing the HTML tree builder changes. To save you the trouble of two levels of code generation, I'll attach a patch with the required changes. Those will supersede this change to nsHtml5AtomList.h.

Marking r- in order to make sure this change is removed from this changeset. Otherwise, assuming you import the patch I'll attach shortly and get r+ from wchen for it, the legacy parser change looks OK. (I'm not sure if editor still cares or if that's dead code.)
Comment 40 User image Henri Sivonen (:hsivonen) 2016-12-22 01:40:31 PST
Created attachment 8821073 [details] [diff] [review]
Parser changes

This patch contains the m-c changes to the HTML parser. I don't know what would happen if I pushed it to Review Board, so I suggest you push it as part of your queue and request review from wchen for my changes.

The relevant spec changes is from 2012 (https://github.com/whatwg/html/commit/2fb24fcf3f916236e8767e2cb72b23e5c75b77e9). I've managed to miss it at the time. It turns out that html5lib doesn't have a unit test for it. Filed https://github.com/html5lib/html5lib-tests/issues/85 . Once a unit test is added to the html5lib-tests repo, we should sync our m-c copy of the html5lib-tests/tree-construction/.
Comment 41 User image Henri Sivonen (:hsivonen) 2016-12-22 01:44:27 PST
Created attachment 8821074 [details] [diff] [review]
htmlparser repo patch

Once attachment  8821073 [details] [diff] [review] lands in m-c, this corresponding patch should be pushed to https://hg.mozilla.org/projects/htmlparser/
Comment 42 User image Masayuki Nakano [:masayuki] 2016-12-22 03:58:25 PST
Comment on attachment 8817969 [details]
Bug 1322938 - Basic implementation of HTMLDialogElement.

https://reviewboard.mozilla.org/r/98148/#review101010

Otherwise, look ok to me (under editor/).

::: editor/libeditor/HTMLEditUtils.cpp:641
(Diff revision 3)
>         GROUP_OPTIONS | GROUP_INLINE_ELEMENT),
>    ELEM(dd, true, false, GROUP_DL_CONTENT, GROUP_FLOW_ELEMENT),
>    ELEM(del, true, true, GROUP_PHRASE | GROUP_BLOCK, GROUP_FLOW_ELEMENT),
>    ELEM(details, true, true, GROUP_BLOCK, GROUP_FLOW_ELEMENT),
>    ELEM(dfn, true, true, GROUP_PHRASE, GROUP_INLINE_ELEMENT),
> +  ELEM(dialog, true, false, GROUP_BLOCK, GROUP_FLOW_ELEMENT),

Is this really not possible to contain <dialog> element itself?

https://html.spec.whatwg.org/multipage/forms.html#the-dialog-element
The contents is "Flow content".

https://html.spec.whatwg.org/multipage/dom.html#flow-content-2
Flow Content includes <dialog>.

Am I missing something?
Comment 43 User image Tim Nguyen :ntim 2016-12-22 12:10:27 PST
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #42)
> Comment on attachment 8817969 [details]
> Bug 1322938 - Basic implementation of HTMLDialogElement.
> 
> https://reviewboard.mozilla.org/r/98148/#review101010
> 
> Otherwise, look ok to me (under editor/).
> 
> ::: editor/libeditor/HTMLEditUtils.cpp:641
> (Diff revision 3)
> >         GROUP_OPTIONS | GROUP_INLINE_ELEMENT),
> >    ELEM(dd, true, false, GROUP_DL_CONTENT, GROUP_FLOW_ELEMENT),
> >    ELEM(del, true, true, GROUP_PHRASE | GROUP_BLOCK, GROUP_FLOW_ELEMENT),
> >    ELEM(details, true, true, GROUP_BLOCK, GROUP_FLOW_ELEMENT),
> >    ELEM(dfn, true, true, GROUP_PHRASE, GROUP_INLINE_ELEMENT),
> > +  ELEM(dialog, true, false, GROUP_BLOCK, GROUP_FLOW_ELEMENT),
> 
> Is this really not possible to contain <dialog> element itself?
> 
> https://html.spec.whatwg.org/multipage/forms.html#the-dialog-element
> The contents is "Flow content".
> 
> https://html.spec.whatwg.org/multipage/dom.html#flow-content-2
> Flow Content includes <dialog>.
> 
> Am I missing something?

Good catch, thanks! Will fix it.

(In reply to Henri Sivonen (:hsivonen) (not doing reviews or reading bugmail until 2017-01-02) from comment #39)
> Comment on attachment 8817969 [details]
> Bug 1322938 - Basic implementation of HTMLDialogElement.
> 
> https://reviewboard.mozilla.org/r/98148/#review100992
> 
> ::: parser/html/nsHtml5AtomList.h:870
> (Diff revision 3)
> >  HTML5_ATOM(circle, "circle")
> >  HTML5_ATOM(center, "center")
> >  HTML5_ATOM(canvas, "canvas")
> >  HTML5_ATOM(divide, "divide")
> >  HTML5_ATOM(degree, "degree")
> > +HTML5_ATOM(dialog, "dialog")
> 
> This is a generated file. Please don't edit it manually. This patch is
> missing the HTML tree builder changes. To save you the trouble of two levels
> of code generation, I'll attach a patch with the required changes. Those
> will supersede this change to nsHtml5AtomList.h.
Thanks! Will publish on mozreview.
Comment 44 User image Tim Nguyen :ntim 2016-12-22 12:20:06 PST Comment hidden (mozreview-request)
Comment 45 User image Tim Nguyen :ntim 2016-12-22 12:20:06 PST Comment hidden (mozreview-request)
Comment 46 User image Tim Nguyen :ntim 2016-12-22 12:20:06 PST Comment hidden (mozreview-request)
Comment 47 User image Tim Nguyen :ntim 2016-12-22 12:20:06 PST Comment hidden (mozreview-request)
Comment 48 User image Tim Nguyen :ntim 2016-12-22 12:20:06 PST Comment hidden (mozreview-request)
Comment 49 User image Tim Nguyen :ntim 2016-12-22 12:20:06 PST Comment hidden (mozreview-request)
Comment 50 User image Tim Nguyen :ntim 2016-12-22 12:23:29 PST Comment hidden (mozreview-request)
Comment 51 User image Tim Nguyen :ntim 2016-12-22 12:23:29 PST Comment hidden (mozreview-request)
Comment 52 User image Tim Nguyen :ntim 2016-12-22 12:23:29 PST Comment hidden (mozreview-request)
Comment 53 User image Tim Nguyen :ntim 2016-12-22 12:23:29 PST Comment hidden (mozreview-request)
Comment 54 User image Tim Nguyen :ntim 2016-12-22 12:23:29 PST Comment hidden (mozreview-request)
Comment 55 User image Tim Nguyen :ntim 2016-12-22 12:23:29 PST Comment hidden (mozreview-request)
Comment 56 User image William Chen [:wchen] 2016-12-22 16:43:28 PST
Comment on attachment 8821270 [details]
Bug 1322938 - Make the HTML tree builder aware of <dialog>.

https://reviewboard.mozilla.org/r/100574/#review101156
Comment 58 User image Tim Nguyen :ntim 2016-12-22 18:44:21 PST Comment hidden (mozreview-request)
Comment 59 User image Tim Nguyen :ntim 2016-12-22 18:44:21 PST Comment hidden (mozreview-request)
Comment 60 User image Tim Nguyen :ntim 2016-12-22 18:44:21 PST Comment hidden (mozreview-request)
Comment 61 User image Tim Nguyen :ntim 2016-12-22 18:44:21 PST Comment hidden (mozreview-request)
Comment 62 User image Tim Nguyen :ntim 2016-12-22 18:44:21 PST Comment hidden (mozreview-request)
Comment 63 User image Tim Nguyen :ntim 2016-12-22 18:44:21 PST Comment hidden (mozreview-request)
Comment 64 User image Tim Nguyen :ntim 2016-12-22 19:44:54 PST
Got rid of the assertion: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96fe9c97bbf3&selectedJob=33357433
Still a few failures to go.
Comment 65 User image Tim Nguyen :ntim 2016-12-23 04:43:40 PST Comment hidden (mozreview-request)
Comment 66 User image Tim Nguyen :ntim 2016-12-23 04:43:40 PST Comment hidden (mozreview-request)
Comment 67 User image Tim Nguyen :ntim 2016-12-23 04:43:40 PST Comment hidden (mozreview-request)
Comment 68 User image Tim Nguyen :ntim 2016-12-23 04:43:40 PST Comment hidden (mozreview-request)
Comment 69 User image Tim Nguyen :ntim 2016-12-23 04:43:40 PST Comment hidden (mozreview-request)
Comment 70 User image Tim Nguyen :ntim 2016-12-23 04:43:40 PST Comment hidden (mozreview-request)
Comment 71 User image Tim Nguyen :ntim 2016-12-23 04:50:12 PST Comment hidden (mozreview-request)
Comment 72 User image Tim Nguyen :ntim 2016-12-23 04:50:12 PST Comment hidden (mozreview-request)
Comment 73 User image Tim Nguyen :ntim 2016-12-23 04:50:12 PST Comment hidden (mozreview-request)
Comment 74 User image Tim Nguyen :ntim 2016-12-23 04:50:12 PST Comment hidden (mozreview-request)
Comment 75 User image Tim Nguyen :ntim 2016-12-23 04:50:12 PST Comment hidden (mozreview-request)
Comment 76 User image Tim Nguyen :ntim 2016-12-23 04:50:12 PST Comment hidden (mozreview-request)
Comment 78 User image Pulsebot 2016-12-23 07:02:30 PST
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c88ef02c5d4
Basic implementation of HTMLDialogElement. r=smaug, masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf6c65628dd0
Emit close event when HTMLDialogElement.prototype.close() is called. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2992c400da4d
Update <dialog> element Web Platform Tests expected results. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3bed08a1f6f
Put <dialog> element behind preference. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d04a7f9567b
Test whether dom.dialog_element.enabled works properly. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d9ed256d618
Make the HTML tree builder aware of <dialog>. r=wchen
Comment 79 User image Pulsebot 2016-12-23 12:35:21 PST
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e7f46d574e
needs a clobber for test_lowDiskSpace.html on Android. r=clobber on a CLOSED TREE
Comment 81 User image Tim Nguyen :ntim 2016-12-23 19:33:11 PST
(In reply to Wes Kocher (:KWierso) from comment #80)
> Backed out for the test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=40991023&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c95614dac022

Comment #79 fixed the relevant failure, I guess you missed that ?
Comment 82 User image Pulsebot 2016-12-23 19:43:25 PST
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b6ed4bb4263
Basic implementation of HTMLDialogElement. r=smaug, masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bdaf0dd932
Emit close event when HTMLDialogElement.prototype.close() is called. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/91c8b6d99e35
Update <dialog> element Web Platform Tests expected results. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d93937b3f80e
Put <dialog> element behind preference. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f3c2e660d28
Test whether dom.dialog_element.enabled works properly. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/45b81607acd2
Make the HTML tree builder aware of <dialog>. r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/1de6bfc4c973
needs a clobber for test_lowDiskSpace.html on Android. r=clobber
Comment 83 User image Wes Kocher (:KWierso) 2016-12-23 21:02:21 PST
Backed out again for the failures in https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9625f2382a

The clobber was a last-ditch effort to fix the problems after what was thought to be a backout of this bug's patches failed to fix the failures. But it turns out that the backed out patches was for a different bug, so would clearly not fix anything here, and the clobber didn't fix anything.
Comment 84 User image Jorg K (GMT+1) 2016-12-25 09:14:58 PST
Comment on attachment 8820380 [details]
Bug 1322938 - Put <dialog> element behind preference.

https://reviewboard.mozilla.org/r/99884/#review101518

::: testing/web-platform/meta/html/semantics/interfaces.html.ini:34
(Diff revision 8)
>      expected: FAIL
>  
>    [Interfaces for SLOT]
>      expected: FAIL
>  
>    [Interfaces for å-bar]

Sorry for the drive-by comment.
\[Interfaces for å-bar\] looks odd, it this intentional?
Comment 86 User image Pulsebot 2017-01-03 10:13:36 PST
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73c4baa877dd
Basic implementation of HTMLDialogElement. r=smaug, masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/96351402aff9
Emit close event when HTMLDialogElement.prototype.close() is called. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/01002f848bdd
Update <dialog> element Web Platform Tests expected results. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/156d46587617
Put <dialog> element behind preference. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e80693c958a
Test whether dom.dialog_element.enabled works properly. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc5070168e9
Disable test_lowDiskSpace.html on Android debug (bug 1328321). r=me
Comment 87 User image Pulsebot 2017-01-03 10:58:00 PST
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78d315ef8ebf
Make the HTML tree builder aware of <dialog>. r=wchen
Comment 89 User image Tim Nguyen :ntim 2017-01-13 10:28:39 PST
Hi Henri, thanks for your help on this bug. Can you help me land attachment 8821074 [details] [diff] [review] on the parser repo? Thanks!
Comment 90 User image Henri Sivonen (:hsivonen) 2017-01-16 23:25:38 PST
https://hg.mozilla.org/projects/htmlparser/rev/d9bdf700af247563fec9b07b52ede6bf61edd513
Mozilla bug 1322938 - Make the tree builder aware of <dialog>. r=wchen.
Comment 91 User image Henri Sivonen (:hsivonen) 2017-01-16 23:26:09 PST
(In reply to Tim Nguyen :ntim from comment #89)
> Hi Henri, thanks for your help on this bug. Can you help me land attachment
> 8821074 [details] [diff] [review] on the parser repo? Thanks!

Landed per comment 90.

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