Closed Bug 1322938 Opened 8 years ago Closed 7 years ago

Basic implementation of HTMLDialogElement

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(8 files)

Bug 1322938 - Basic implementation of HTMLDialogElement.
58 bytes, text/x-review-board-request
smaug
: review+
masayuki
: review+
Details
Bug 1322938 - Emit close event when HTMLDialogElement.prototype.close() is called.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1322938 - Update <dialog> element Web Platform Tests expected results.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1322938 - Put <dialog> element behind preference.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1322938 - Test whether dom.dialog_element.enabled works properly.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Parser changes
102.13 KB, patch
Details | Diff | Splinter Review
htmlparser repo patch
34.92 KB, patch
Details | Diff | Splinter Review
Bug 1322938 - Make the HTML tree builder aware of <dialog>.
59 bytes, text/x-review-board-request
wchen
: review+
Details 
This bug is about implementing a basic implementation of HTMLDialogElement.

This doesn't include:
- Anchoring
- Modal dialogs
Component: Developer Tools: Shared Components → DOM: Core & HTML
Product: Firefox → Core
(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

PR to web-platform-tests to remove anchoring:
https://github.com/w3c/web-platform-tests/pull/4312
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)
(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 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 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 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 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 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 on attachment 8817969 [details]
Bug 1322938 - Basic implementation of HTMLDialogElement.

https://reviewboard.mozilla.org/r/98148/#review98926
Attachment #8817969 - Flags: review?(bugs) → review-
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 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 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 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 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 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 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?
(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.
(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 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 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+
Olli, thanks for the reviews!

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

Thank you.
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-
Attached patch Parser changes — — Splinter Review 
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/.
Once attachment  8821073 [details] [diff] [review] lands in m-c, this corresponding patch should be pushed to https://hg.mozilla.org/projects/htmlparser/
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+
(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 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+
Got rid of the assertion: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96fe9c97bbf3&selectedJob=33357433
Still a few failures to go.
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
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
(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)
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 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?
Depends on: 1328321
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
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
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)
(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)
(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!
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.

Attachment

General

Creator:
Created:
Updated:
Size: