Basic implementation of HTMLDialogElement

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
8 months ago
a month ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

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

unspecified
mozilla53
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

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

Attachments

(8 attachments)

(Assignee)

Description

8 months ago
This bug is about implementing a basic implementation of HTMLDialogElement.

This doesn't include:
- Anchoring
- Modal dialogs
(Assignee)

Updated

8 months ago
Component: Developer Tools: Shared Components → DOM: Core & HTML
Product: Firefox → Core
(Assignee)

Comment 1

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

Comment 2

8 months ago

PR to web-platform-tests to remove anchoring:
https://github.com/w3c/web-platform-tests/pull/4312
Keywords: dev-doc-needed
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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?
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 8

8 months ago
(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.
Flags: needinfo?(ntim.bugs)

Comment 9

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

Comment 10

8 months ago
mozreview-review
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

8 months ago
mozreview-review
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.
Attachment #8817971 - Flags: review?(bugs) → review+
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.
Attachment #8817969 - Flags: review?(ehsan) → review?(masayuki)

Comment 13

8 months ago
mozreview-review
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

8 months ago
mozreview-review
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.
Attachment #8817971 - Flags: review+ → review-

Comment 15

8 months ago
mozreview-review
Comment on attachment 8817969 [details]
Bug 1322938 - Basic implementation of HTMLDialogElement.

https://reviewboard.mozilla.org/r/98148/#review98926
Attachment #8817969 - Flags: review?(bugs) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

8 months ago
mozreview-review-reply
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
(Assignee)

Comment 22

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

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

Comment 24

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

Comment 25

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

Comment 26

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

Comment 27

8 months ago
mozreview-review
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?
(Assignee)

Comment 28

8 months ago
(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.
(Assignee)

Comment 29

8 months ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 36

8 months ago
mozreview-review
Comment on attachment 8817970 [details]
Bug 1322938 - Emit close event when HTMLDialogElement.prototype.close() is called.

https://reviewboard.mozilla.org/r/98150/#review100584
Attachment #8817970 - Flags: review?(bugs) → review+

Comment 37

8 months ago
mozreview-review
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.
Attachment #8820495 - Flags: review?(bugs) → review+
(Assignee)

Comment 38

8 months ago
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

8 months ago
mozreview-review
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.)
Attachment #8817969 - Flags: review?(hsivonen) → review-
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/.
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

8 months ago
mozreview-review
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?
Attachment #8817969 - Flags: review?(masayuki) → review+
(Assignee)

Comment 43

8 months ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 56

8 months ago
mozreview-review
Comment on attachment 8821270 [details]
Bug 1322938 - Make the HTML tree builder aware of <dialog>.

https://reviewboard.mozilla.org/r/100574/#review101156
Attachment #8821270 - Flags: review?(wchen) → review+
(Assignee)

Comment 57

8 months ago
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa54edc6dfb5f97f58170e8c30359bb444c7d396
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 64

8 months ago
Got rid of the assertion: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96fe9c97bbf3&selectedJob=33357433
Still a few failures to go.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 77

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd6c8c36cbae

Comment 78

8 months ago
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

8 months ago
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
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
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 81

8 months ago
(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 ?
Flags: needinfo?(ntim.bugs)

Comment 82

8 months ago
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
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.
Flags: needinfo?(ntim.bugs)

Comment 84

8 months ago
mozreview-review
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?
(Assignee)

Comment 85

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=58be2978aaa43070024c32bcf6dc8249cb3c92c8
Flags: needinfo?(ntim.bugs)
(Assignee)

Updated

8 months ago
Depends on: 1328321

Comment 86

8 months ago
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

8 months ago
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 88

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/73c4baa877dd
https://hg.mozilla.org/mozilla-central/rev/96351402aff9
https://hg.mozilla.org/mozilla-central/rev/01002f848bdd
https://hg.mozilla.org/mozilla-central/rev/156d46587617
https://hg.mozilla.org/mozilla-central/rev/6e80693c958a
https://hg.mozilla.org/mozilla-central/rev/5cc5070168e9
https://hg.mozilla.org/mozilla-central/rev/78d315ef8ebf
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Comment 89

7 months ago
Hi Henri, thanks for your help on this bug. Can you help me land attachment 8821074 [details] [diff] [review] on the parser repo? Thanks!
Flags: needinfo?(hsivonen)
https://hg.mozilla.org/projects/htmlparser/rev/d9bdf700af247563fec9b07b52ede6bf61edd513
Mozilla bug 1322938 - Make the tree builder aware of <dialog>. r=wchen.
(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.
Flags: needinfo?(hsivonen)
I've updated the browser support information on the main interface page, and written all the member pages:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/open
https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/returnValue
https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/show
https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/showModal
https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/close

I've also improved the example and published it separately on our DOM examples GitHub repo:

https://mdn.github.io/dom-examples/htmldialogelement-basic/

I have not mentioned this in the Fx53 release notes, as it is currently behind a pref.

Let me know if this all looks OK. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
I've also added a note for it at https://developer.mozilla.org/en-US/Firefox/Experimental_features#HTML.

Sebastian
(In reply to Sebastian Zartner [:sebo] from comment #93)
> I've also added a note for it at
> https://developer.mozilla.org/en-US/Firefox/Experimental_features#HTML.
> 
> Sebastian

Ah, lovely. Thanks Sebo!
(Assignee)

Comment 95

6 months ago
Thank you Chris and Sebastian! I've modified the page for showModal to clarify that it's not currently fully supported yet.
(In reply to Tim Nguyen :ntim from comment #95)
> Thank you Chris and Sebastian! I've modified the page for showModal to
> clarify that it's not currently fully supported yet.

Ah, that's great - thanks Tim! I did wonder why the two methods currently seemed to behave in a very similar way ;-)
Depends on: 1379728
You need to log in before you can comment on or make changes to this bug.