1276240 - Second argument to document.createElement is sometimes not handled properly

Status: RESOLVED FIXED

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

Description

[Robin Berjon]

Attachment: index.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160520004020

Steps to reproduce:

In the console, if you type:

document.createElement('p', null)
<p is="null">
document.createElement('p', undefined)
<p is="undefined">

This also triggered a bug in React (https://github.com/facebook/react/pull/6896, discussed https://discuss.reactjs.org/t/is-null-attribute-on-every-tag/4032/).

For some reason, trying to reproduce it in the WHATWG Live DOM Viewer or in JSBin does not work, but the simple page attached exhibits the issue.


Actual results:

Gecko stringifies the second argument and uses that to set the is attribute. A case could be made for applying that behaviour to null (though it's unhelpful) but certainly not for undefined.


Expected results:

It should ignore the second argument, as per https://dom.spec.whatwg.org/#dom-document-createelement.

Comment 1

[Liviu Cirdei]

User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID 	20160530071207

Confirmed on Windows 10, latest Nightly (49.0a1).

Comment 2

[Markus Stange [:mstange]]

Is this a regression?

Comment 3

[William Chen [:wchen]]

This is an old regression from bug 856140. There is currently work on updating to the new API which should fix this issue but if we need a more immediate fix, we can remove the custom element version of createElement.

Comment 4

[Jocelyn Liu [:jocelyn] [:joliu]]

I'm now working on createElement[NS] methods for custom element v1 in Bug 1276579.
The issue couldn't be reproduced anymore with my ongoing patch.
We could confirm again after Bug 1276579 fixed.
I could also provide immediate fix here first as William mentioned if this bug is urgent.

Comment 5

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

I think we should fix this asap, since it may take still some time to get CustomElements released

Comment 6

[Jocelyn Liu [:jocelyn] [:joliu]]

Per irc discussion with Olli, I will add pref checking in C++ code.
My approach here would be ignoring second argument in the overloaded method and calling createElement('p') if the pref of web components is not enabled.

Comment 7

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #6)
> Per irc discussion with Olli, I will add pref checking in C++ code.
> My approach here would be ignoring second argument in the overloaded method
> and calling createElement('p') if the pref of web components is not enabled.

This is also the same behavior as chrome and safari.

Comment 8

[Patrick Westerhoff]

So you are saying that once the pref is enabled or web components are stable enough to be enabled by default, that the behavior outlined above is *correct*? That is, specifying `undefined` or `null` resulting in `is="undefined"` and `is="null"` respectively? That sounds very wrong to me.

> This is also the same behavior as chrome and safari.

I’ve enabled experimental-web-platform-features on Chrome to test this and Chrome does the natural thing:

  document.createElement('p', 'foo')
  <p is=​"foo">​</p>​
  document.createElement('p', null)
  <p>​</p>​
  document.createElement('p', undefined)
  <p>​</p>​

Comment 9

[Jocelyn Liu [:jocelyn] [:joliu]]

Hi,

(In reply to Patrick Westerhoff from comment #8)
> So you are saying that once the pref is enabled or web components are stable
> enough to be enabled by default, that the behavior outlined above is
> *correct*? That is, specifying `undefined` or `null` resulting in
> `is="undefined"` and `is="null"` respectively? That sounds very wrong to me.
> 

You were right, that also sounds wrong to me.

> > This is also the same behavior as chrome and safari.
> 
> I’ve enabled experimental-web-platform-features on Chrome to test this and
> Chrome does the natural thing:
> 
>   document.createElement('p', 'foo')
>   <p is=​"foo">​</p>​
>   document.createElement('p', null)
>   <p>​</p>​
>   document.createElement('p', undefined)
>   <p>​</p>​

This happens in Firefox because of the binding layer will convert null and undefined directly into string if the parameter was non-nullable DOMString.
In order to fix this, we might also need to tweak the the API definition to |createElement(DOMString localName, DOMString? typeExtension);|

I could see Chrome also use a nullable parameter in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.idl?q=Document.idl&sq=package:chromium&l=181 though I'm not sure what's their reason.

We're already implementing new API in Bug 1276579, which will not have this problem since it is using an optional dictionary for 'is' instead of DOMString directly.
For short term solution before that bug lands, we could make the parameter nullable first in this bug.
Personally I think it's okay to change this API since the spec is already changed and this method should be removed soon.

In summary, my approach would be calling setupCustomElement only if the preference is enabled && |typeExtension.IsEmpty()| is false.

Comment 10

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #9)
> 
> In summary, my approach would be calling setupCustomElement only if the
> preference is enabled && |typeExtension.IsEmpty()| is false.

Actually it should be two separate checks, pref check for |setupCustomElement()|; |IsEmpty()| for setting 'is' attribute.
Sorry for not being clear enough.

Comment 11

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1276240 - Ignore typeExtension argument if the value is null or undefined.

Review commit: https://reviewboard.mozilla.org/r/61018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61018/

Comment 12

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #11)
> Created attachment 8765857 [details]
> Bug 1276240 - Ignore typeExtension argument if the value is null or
> undefined.
> 
> Review commit: https://reviewboard.mozilla.org/r/61018/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/61018/

Sorry, the bug number in the test was wrong, I will change it to Bug 1276240 in the next revision.

Comment 13

[Patrick Westerhoff]

https://reviewboard.mozilla.org/r/61018/#review57836

::: dom/tests/mochitest/webcomponents/test_bug1276240.html:41
(Diff revision 1)
> +SpecialPowers.setBoolPref("dom.webcomponents.enabled", true);
> +test();

Maybe add another test that shows that specifying a proper value will actually set the `is` attribute? (if webcomponents are enabled)

Comment 14

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

Comment on attachment 8765857 [details]
Bug 1276240 - Ignore typeExtension argument if the value is null or undefined.

https://reviewboard.mozilla.org/r/61018/#review57838

::: dom/tests/mochitest/webcomponents/test_bug1276240.html:39
(Diff revision 1)
> +
> +  e = document.createElementNS("http://www.w3.org/1999/xhtml", "p", undefined);
> +  is(e.getAttribute("is"), null);
> +}
> +
> +SpecialPowers.setBoolPref("dom.webcomponents.enabled", false);

Could you use SpecialPowers.pushPrefEnv to set prefs.

Comment 15

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Patrick Westerhoff from comment #13)
> https://reviewboard.mozilla.org/r/61018/#review57836
> 
> ::: dom/tests/mochitest/webcomponents/test_bug1276240.html:41
> (Diff revision 1)
> > +SpecialPowers.setBoolPref("dom.webcomponents.enabled", true);
> > +test();
> 
> Maybe add another test that shows that specifying a proper value will
> actually set the `is` attribute? (if webcomponents are enabled)

I think the normal 'is' case is already covered mainly in test_document_register.html.

Comment 16

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8765857 [details]
Bug 1276240 - Ignore typeExtension argument if the value is null or undefined.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61018/diff/1-2/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0edb9df3c97f
Ignore typeExtension argument if the value is null or undefined. r=smaug

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0edb9df3c97f