Second argument to document.createElement is sometimes not handled properly

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: robin, Assigned: yrliou)

Tracking

48 Branch
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

3 years ago
Posted file 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.

Updated

3 years ago
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core

Comment 1

3 years ago
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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Is this a regression?
Flags: needinfo?(wchen)
Blocks: 856140
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.
Flags: needinfo?(wchen)
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.
Depends on: 1276579
I think we should fix this asap, since it may take still some time to get CustomElements released
Flags: needinfo?(wchen)
Flags: needinfo?(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.
Assignee: nobody → joliu
Flags: needinfo?(wchen)
Flags: needinfo?(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.
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>​
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.
(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.
(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.
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 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.
Attachment #8765857 - Flags: review?(bugs) → review+
(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 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

3 years ago
Pushed by joliu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0edb9df3c97f
Ignore typeExtension argument if the value is null or undefined. r=smaug

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0edb9df3c97f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.