Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polymer encouraged incorrect document.createElement() usage #544

Closed
annevk opened this issue Aug 11, 2016 · 39 comments
Closed

Polymer encouraged incorrect document.createElement() usage #544

annevk opened this issue Aug 11, 2016 · 39 comments

Comments

@annevk
Copy link
Collaborator

annevk commented Aug 11, 2016

See https://bugzilla.mozilla.org/show_bug.cgi?id=1294100 and references from there. It seems like we need to support document.createElement(string, string). Not entirely sure if ignoring the argument or treating it as id is better.

Sigh.

@domenic
Copy link
Collaborator

domenic commented Aug 11, 2016

@dominiccooney, have we seen fallout from this in Blink?

Ugh.

I'd definitely prefer ignoring the argument; we should not opt in old code to the new semantics...

@annevk
Copy link
Collaborator Author

annevk commented Aug 11, 2016

@smaug---- was saying the same.

@WebReflection
Copy link

My 2cents: document-register-element now supports 3 document.createElement overloads: (string), (string, stringIs), (string, object{is:string})

After all I don't see why the is case should fail in polyfilled browsers, sent either as string, like it was already in v0 specs, or object with an is property.

@domenic
Copy link
Collaborator

domenic commented Aug 11, 2016

The reason it should fail (ideally) or do nothing silently (as is probably what we'll have to spec) is that the semantics have changed. It is the same reason we had to rename all the APIs. We cannot have old v0 code transparently upgraded to v1 semantics, since they are different.

@WebReflection
Copy link

So, if I understand correctly, passing an object should be V1 only and passing a string V0 only?

@domenic
Copy link
Collaborator

domenic commented Aug 11, 2016

Yes (in browsers that implement v0, i.e. Chrome only).

@WebReflection
Copy link

WebReflection commented Aug 11, 2016

Thanks, mine right now is a polyfill for v0 but I'm planning to bring in V1 too.

Although I thought WebKit didn't agree about the is property, did they change their mind? If the V1 is final and agreed by all vendors then it's time to polyfill it.

@domenic
Copy link
Collaborator

domenic commented Aug 11, 2016

WebKit does not agree with customized built-in elements, but they are included in the standard since we have multiple vendors interested in implementing. If it turns out that for some reason that doesn't happen, they will be removed.

Nothing in spec land is ever final, however. It's a judgment call when you want to start implementing something. All major web browsers already have.

@WebReflection
Copy link

I understand nothing in spec is never final but V0 somehow is, and V1 hopefully is as well.
Versioning is a common "nothing ever final" approach 'cause vendors/implementors need to actually implement something so ... is V1 still heavily discussed or something worth polyfill?

I'm just trying to have an ETA for my poly users and myself too. Thanks

@domenic
Copy link
Collaborator

domenic commented Aug 11, 2016

v0 is abandoned, not final. v1 is still being heavily discussed as it is being implemented.

@rniwa
Copy link
Collaborator

rniwa commented Aug 11, 2016

Like we've stated multiple times in the past, we're not going to implement custom builtin elements in WebKit but all other aspects of custom elements should be interoperable across browsers.

@annevk
Copy link
Collaborator Author

annevk commented Aug 12, 2016

I guess that would be another way for Fx to dodge this. Just not implement custom builtins…

@dominiccooney
Copy link
Contributor

Blink implements an old draft of the custom elements spec where that option is a DOMString. Our implementation of custom elements "v1" and that old stuff are basically separate. We won't support interpreting that second argument as a string when creating customized built-in elements in V1 and will deprecate that along with document.registerElement and the rest of the old implementation.

@miketaylr
Copy link

miketaylr commented Aug 17, 2016

Blink implements an old draft of the custom elements spec where that option is a DOMString

Looks like that's here: https://www.w3.org/TR/2016/WD-custom-elements-20160226/#extensions-to-document-interface-to-instantiate

Also it looks like versions of react.js used the stringy 2nd argument until fairly recently. See facebook/react#6896 for discussion and related links.

Seems like a lot of deployed code requiring the old custom elements overload behavior...

@WebReflection
Copy link

Since still open, and just FYI, my alternative Custom Elements only polyfill implements both cases accordingly if you defined through V0 or V1. If in V0 it accepts an optional string for the is="...", if in V1 it accepts an object like {is: "button"}.

@dglazkov
Copy link
Contributor

I think we should just use the new syntax and not introduce any new complexity into the spec. Our plan is as follows:

  • Blink will start warning when the old (v0) syntax is used, along with the rest of v0 deprecation. We might even start warning sooner.
  • Polymer will update the v0 polyfill to accept the new syntax, and create a warning if someone is using the old syntax. The next version of Polymer and v1 polyfill will only allow the new syntax.
  • We'll update Polymer/html5rocks/webcomponents.org docs to only talk about the v1 syntax.

@smaug----
Copy link

atm browsers are forced to support also the old syntax, if they support also the new syntax. If only new syntax is supported, browsers throw when passing string as the second param.

@dglazkov
Copy link
Contributor

@smaug---- right. I am proposing that we only support the new syntax and throw. There's a simple workaround for the error (literally a few characters' worth), and we (Blink) will provide the ramp-down for the old usage in form of (first) deprecation messages and (then) removal.

@smaug----
Copy link

smaug---- commented Aug 22, 2016

Throwing breaks pages rather easily. Note, browsers not supporting any CustomElement stuff, don't throw.

@WebReflection
Copy link

WebReflection commented Aug 22, 2016

@dglazkov I agree with @smaug---- that throwing breaks badly libraries and sites that already bet on what html5rocks and all documentation around Custom Elements 'till now promoted.

I wouldn't throw and if it's easy for developers to use new second argument, it's even easier for blink to keep what's there already and warn instead.

Regardless, the reason my polyfill supports both is to indeed help developers migrating to the new V1 API so if blink will start throwing, I'll catch that, and switch blink to a polyfilled blink which would be inconvenient for everyone.

WebSQL is still there and it worked well for many, same goes for Custom Elements V0, imo.
𝄞 Please don't throw ♩♫♬

@annevk
Copy link
Collaborator Author

annevk commented Aug 23, 2016

If Chrome ships a breaking change soon we might be able to turn this around. Firefox could most likely follow were that to happen. I'm not going to recommend Firefox to ship a breaking change now though if they want to ship v1 faster than Chrome.

@WebReflection
Copy link

at least we should agree nobody should throw anything unless the new API is fully available and widely deployed, otherwise developers will have broken sites with no way to fix them.

Thank you.

@WebReflection
Copy link

FYI: Firefox nightly is already throwing when passing a string as second argument to document.createElement(el, is) which broke everything online that isn't updated to my latest v1.0.7 polyfill that catches that. Bad fox!

@domenic
Copy link
Collaborator

domenic commented Aug 25, 2016

I don't think it's appropriate to castigate browser vendors for following the specification.

@WebReflection
Copy link

I don't think "castigate" is an appropriate word for my basic follow up of what happened out there and specs here are changing daily so all I am saying: you are throwing away 2 years of your own Web promotion about this stuff.

If you like this precedent, go on throwing right away: I'm sure devs that believed in V0 and followed Chrome implementing it will love such decision.

/sarcasm

@domenic
Copy link
Collaborator

domenic commented Aug 25, 2016

@WebReflection I'd like to remind you of the Code of Ethics and Professional Conduct that governs your participation in this repository. Sarcasm, "bad fox!", and other forms of derogatory and disrespectful interaction with members of the working group are not welcome here. Please consider this a warning.

@WebReflection
Copy link

WebReflection commented Aug 25, 2016

@domenic I don't think you should bring your (documented) historic personal hostility against me to this group neither, picking on a sentence like "Bad fox" could be, after years of my personal contribution to MDN and Mozilla.

However, I'll note the warning and avoid interactions that brings real-world developers experience I'm aware of in here.

@domenic
Copy link
Collaborator

domenic commented Aug 25, 2016

Thank you. It's important to maintain a civil environment, so I'm glad you're taking the warning seriously.

@dominiccooney
Copy link
Contributor

What's left to do on this bug, or can it be closed?

FWIW, Blink is starting to implement deprecation warnings, etc.

@annevk
Copy link
Collaborator Author

annevk commented Oct 3, 2016

@dominiccooney well, we haven't determined yet which way the specification needs to go, right? Whether we can throw for the second argument being a string or not.

@bzbarsky
Copy link

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1338889 createElementNS has the same issue...

@annevk
Copy link
Collaborator Author

annevk commented Feb 15, 2017

Hurray.

@dominiccooney I don't see a warning in Blink for document.createElement("test", "test") or document.createElementNS("test", "test", "test"). What is the status on this?

@dominiccooney
Copy link
Contributor

We're working on it. Blink is reticent to add a deprecation warning until there's a clear path to removal, so this is blocked on a few things:

  • Use counter data. That is now available—the use counter has just reached the stable channel. Use is high though.
  • An alternative for developers. We're implementing customized built-in elements, but it has not shipped yet.

@annevk
Copy link
Collaborator Author

annevk commented Feb 16, 2017

An update here would have been good, since I think that effectively means that if Fx wants to ship custom elements, including support for the is attribute, we just have to support a string argument as well due to all the compatibility fallout and Blink moving slowly.

@dominiccooney
Copy link
Contributor

Yeah, so the way we're doing this is the string argument won't create customized built in elements (the new thing in the HTML living standard); to do that, authors will have to use the dictionary. Here's the code.

The upside of this is no other browser should have to handle the second string argument. At worst it might mean not throwing a TypeError but instead ignoring the argument.

The downside of this is that it makes it harder for authors to move code from document.registerElement to customElements.define because you need to migrate element creation code before/at the same time as definition code, which is a bit painful, and that could slow adoption of customized built-in elements. This cost is all borne by Chrome, which may end up having to support document.registerElement longer. Other browsers already decided not to implement document.registerElement so there's nothing new there.

(The way we have it now authors would have to change definition and creation code at the same time, IIRC. But I think this is overly onerous and we should allow the dictionary to create legacy custom elements so that authors can fix all their createElement/NS calls and then go back and port registerElement to define. But foolip had some questions about that and we've punted that decision for now.)

This is the best we could come up with but I'm open to feedback if there's a way to do it better.

@annevk
Copy link
Collaborator Author

annevk commented Feb 19, 2018

So the status quo is that both Chrome and Firefox allow a string argument here still and I don't see any effort toward changing that.

Firefox will simply ignore the second argument if it's a string. I'm going to change the DOM Standard to align with that.

@emilio
Copy link

emilio commented Feb 19, 2018

@annevk note that the webidl you've linked from Firefox is really from Servo. The one you're looking for is:

https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/dom/webidl/Document.webidl#60

@annevk
Copy link
Collaborator Author

annevk commented Feb 19, 2018

Oops, thanks. Same conclusion fortunately.

@rniwa
Copy link
Collaborator

rniwa commented Feb 19, 2018

FWIW, WebKit does not support the second argument at all.

annevk added a commit to whatwg/dom that referenced this issue Feb 20, 2018
annevk added a commit to whatwg/dom that referenced this issue Feb 26, 2018
For compatibility with deployed content, when a string is passed rather than a dictionary, simply ignore the string rather than throw.

Tests: web-platform-tests/wpt#9586.

Fixes WICG/webcomponents#544.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants