1419316 - Setting HTMLOutputElement.defaultValue makes it being reset to "" when it has childs and in default mode

Status: RESOLVED FIXED

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

Description

[John Dai[:jdai]]

There is a test failure at HTMLOutputElement.html[1][2][3].

[1] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/testing/web-platform/tests/custom-elements/reactions/HTMLOutputElement.html
[2] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/testing/web-platform/meta/custom-elements/reactions/HTMLOutputElement.html.ini
[3] custom-elements/reactions/HTMLOutputElement.html

Comment 1

[John Dai[:jdai]]

Update [3] link: http://w3c-test.org/custom-elements/reactions/HTMLOutputElement.html

Comment 2

[John Dai[:jdai]]

Attachment: Fix defaultValue on HTMLOutputElement must enqueue disconnectedCallback when removing a custom element. r=smaug

We can't pass mDefaultValue[1] as a parameter, it'll be removed by nsContentUtils::SetNodeTextContent.

[1] https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/dom/html/HTMLOutputElement.cpp#153

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdd0327e4b34a013cd2c630d2e051240f1437260&filter-tier=1&group_state=expanded

Comment 3

[Edgar Chen [:edgar]]

I am curious if we have some problem in https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/dom/html/HTMLOutputElement.cpp#62?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8931255 [details] [diff] [review]
Fix defaultValue on HTMLOutputElement must enqueue disconnectedCallback when removing a custom element. r=smaug

Yeah, I think Reset has similar issue.
And please add some comment why mDefaultValue can't be passed. It is very much non-obvious. Had to look at the mutation observing methods.

Feel free to fix Reset (+ add a test) if needed in a separate bug.

Comment 5

[Edgar Chen [:edgar]]

We do enqueue disconnectedCallback for the removed custom element, this bug is more about the setting HTMLOutputElement.defaultValue is not handled correct.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

yeah, which is why reviewing took a bit time. Had to realize this isn't a custom element issue at all.

Comment 7

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #3)
> I am curious if we have some problem in
> https://searchfox.org/mozilla-central/rev/
> 33c90c196bc405e628bc868a4f4ba29b992478c0/dom/html/HTMLOutputElement.cpp#62?

It will not have similar issue, because per spec of reset algorithm[1] for output elements is to set the element's value mode flag to default and then to set the element's textContent IDL attribute to the value of the element's default value (thus replacing the element's child nodes). Therefore, the text node for default value should keep it and remove the element's child nodes.

[1] https://html.spec.whatwg.org/#the-output-element:concept-form-reset-control


(In reply to Olli Pettay [:smaug] (ni?s and f?s processed after r?s) from comment #4)
> Comment on attachment 8931255 [details] [diff] [review]
> Fix defaultValue on HTMLOutputElement must enqueue disconnectedCallback when
> removing a custom element. r=smaug
> 
> Yeah, I think Reset has similar issue.
> And please add some comment why mDefaultValue can't be passed. It is very
> much non-obvious. Had to look at the mutation observing methods.
> 
> Feel free to fix Reset (+ add a test) if needed in a separate bug.

I will add a reset testcase that also enqueue disconnectedCallback when removing a custom element. Thank you.

Comment 8

[Edgar Chen [:edgar]]

(In reply to John Dai[:jdai] from comment #7)
> It will not have similar issue

Spec has no problem with the reset algorithm, but our implementation has.
Consider this test: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/5597
The defaultValue is set to "" after reset steps in our implementation.

Comment 9

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #8)
> (In reply to John Dai[:jdai] from comment #7)
> > It will not have similar issue
> 
> Spec has no problem with the reset algorithm, but our implementation has.
> Consider this test:
> http://software.hixie.ch/utilities/js/live-dom-viewer/saved/5597
> The defaultValue is set to "" after reset steps in our implementation.

I file bug 1422260 for this. Thank you.

Comment 10

[John Dai[:jdai]]

Attachment: Bug 1419316 - Fix setting HTMLOutputElement.defaultValue makes it being reset to empty string when it has childs. r=smaug

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b190e474f760052fe954f83fc3d989a97509133e&group_state=expanded&filter-tier=1

Comment 11

[John Dai[:jdai]]

Attachment: Bug 1419316 - Fix setting HTMLOutputElement.defaultValue makes it being reset to empty string when it has childs and in default mode. r=smaug

Address comment 4.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63747de53588
Fix setting HTMLOutputElement.defaultValue makes it being reset to empty string when it has childs and in default mode. r=smaug

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/63747de53588