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

HTMLElement constructor step 5.2 refers to mysterious document #1154

Closed
dominiccooney opened this issue Apr 30, 2016 · 13 comments
Closed

HTMLElement constructor step 5.2 refers to mysterious document #1154

dominiccooney opened this issue Apr 30, 2016 · 13 comments
Assignees
Labels
topic: custom elements Relates to custom elements (as defined in DOM and HTML)

Comments

@dominiccooney
Copy link

I am trying to implement HTMLElement constructor in Blink. At step 5.2, the part that says “and node document set to document”, I don't know what document refers to.

I think this should follow the behavior of Image, which says: “The element's node document must be the active document of the browsing context of the Window object on which the interface object of the invoked constructor is found.”

Apologies in advance if I am missing something obvious here.

@domenic domenic self-assigned this Apr 30, 2016
@domenic
Copy link
Member

domenic commented Apr 30, 2016

Thanks for the catch. I see two alternatives:

  • (A) the active document of the browsing context of realm's global object (= the Window object on which the interface object of the invoked constructor is found)
  • (B) realm's global object's newest Document object.

These will differ after navigation: if you saved the window.HTMLElement constructor of some iframe, then navigated the iframe, then register class extends SavedHTMLElement { } and new it up:

  • (A) will create an element whose node document is the post-navigation document
  • (B) will create an element whose node document is the pre-navigation document

Consistency with Image (and presumably Audio and other named constructors) probably means it's wisest to go with (A), although to me at least intuitively (B) makes more sense, both in terms of the actual outcome and in terms of it being a shorter path from realm.

Opinions @rniwa @annevk @smaug----?

@annevk
Copy link
Member

annevk commented Apr 30, 2016

Ideally the same as new Document(), new Text(), etc. Though DOM is a little vague for those: https://dom.spec.whatwg.org/#dom-document-document.

@domenic
Copy link
Member

domenic commented May 1, 2016

Yeah, DOM's "node document is the global object’s associated document" doesn't exactly define which global (incumbent, entry, current being the possibilities) but it's probably current, which would be (B).

(Note to self: I should probably clean up this constructor to use "the current Realm", instead of "GetFunctionRealm(the currently executing HTMLElement function)".)

I tested Image in http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4119 and it turns out that Chrome does (B) for both Image and Text, whereas Firefox does (A) for both Image and Text. Whereas per spec, Text is probably (B), and Image is (A).

So I think we have some flexibility here to fix custom elements and Image to match (B). Unless anyone objects, I'll make that change here (and PR DOM to increase clarity), then file some Firefox and Edge bugs.

@domenic
Copy link
Member

domenic commented May 1, 2016

BTW it would be good to merge #1091 to give me a solid basis for talking about this stuff in my fix.

@dominiccooney
Copy link
Author

FWIW Blink implements the Text constructor like Image (in Blink bindings, this is governed by the ConstructorCallWith extended attribute.) We don't implement Document constructor yet, but will.

@annevk
Copy link
Member

annevk commented May 2, 2016

@bzbarsky the suggestion here is to make the associated document for new Text(), new Image(), new Document(), etc. the document associated with the current Realm's global object. As @domenic points out above this is what Chrome does, but not what Firefox does, and the specifications are a mixed bag at the moment.

@bzbarsky
Copy link
Contributor

bzbarsky commented May 2, 2016

Er.. Firefox does in fact use the current Realm's newest document, option (B) above.

In the testcase at http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4119 the load in the iframe is same-origin, so when it completes the same Window is used for the new Document as far as I can tell (initial about:blank reuse stuff). In Firefox, when the Text constructor is called, it examines its current Realm global (the Window), gets its most recent document (doc2 in this case), and uses that as ownerDocument.

I modified Domenic's testcase to explicitly check whether the initial about:blank Window is being preserved and you can see the result at http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4120 -- it looks like Firefox is doing that and Chrome is not, which is why the rest of the behavior differs between them. I have no idea why Chrome is doing what it's doing there...

@bzbarsky
Copy link
Contributor

bzbarsky commented May 2, 2016

Er, that testcase with a typo corrected: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4122

On http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4123 (click the button to run the test, yay popup blockers) Chrome seems to do the right thing with initial about:blank.

@bzbarsky
Copy link
Contributor

bzbarsky commented May 2, 2016

And on http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4126 Firefox and Chrome also agree: both reuse the Window of the initial about:blank.

They also agree on http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4124 in that neither one reuses the initial about:blank Window there. I know why that happens in Firefox and I think we consider it a bug, but I'd have to step through the spec carefully to see what's specced for this case right now.

@domenic
Copy link
Member

domenic commented May 2, 2016

OK, so I didn't mean to get the initial about:blank involved; I forgot about the initial load.

http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4129 is a revised version, but still seems to show Chrome using (B) for both Image and Text, and Firefox using (A). (And, it still fails for weird seemingly-unrelated reasons in Edge.)

@bzbarsky
Copy link
Contributor

bzbarsky commented May 2, 2016

http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4129 is identical to http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4119 except for the const log = w; bit and use of log instead of w. Did you mean to post some other link?

@domenic
Copy link
Member

domenic commented May 2, 2016

Yes, I meant to post something more like http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4134

This shows both Chrome and Firefox aligning on (B), it seems. There's some other different issue where setting and reading doc.name does not work in Firefox...

domenic added a commit that referenced this issue May 2, 2016
Fixes #1154, which originally covered only the undefined "document"
variable in the HTMLElement constructor, but expanded on further
investigation to reveal that the spec's current choice of node document
for Audio, Image, and Option was strange and not implemented in
browsers.

While there, cleans up all three of these named constructors to
correctly use "create an element," and to be in numbered algorithm form
instead of giant-prose-block form.
@bzbarsky
Copy link
Contributor

bzbarsky commented May 3, 2016

This shows both Chrome and Firefox aligning on (B), it seems.

OK. I did tell you that Firefox does (B). ;)

There's some other different issue where setting and reading doc.name does not work in Firefox...

Please do feel free to link me to a testcase showing this problem. It doesn't seem to be showing up on http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4134 as far as I can tell.

domenic added a commit that referenced this issue May 3, 2016
Fixes #1154, which originally covered only the undefined "document"
variable in the HTMLElement constructor, but expanded on further
investigation to reveal that the spec's current choice of node document
for Audio, Image, and Option was strange and not implemented in
browsers.

While there, cleans up all three of these named constructors to
correctly use "create an element," and to be in numbered algorithm form
instead of giant-prose-block form.
domenic added a commit that referenced this issue May 3, 2016
Fixes #1154, which originally covered only the undefined "document"
variable in the HTMLElement constructor, but expanded on further
investigation to reveal that the spec's current choice of node document
for Audio, Image, and Option was strange and not implemented in
browsers.

While there, cleans up all three of these named constructors to
correctly use "create an element," and to be in numbered algorithm form
instead of giant-prose-block form.
domenic added a commit that referenced this issue May 4, 2016
Fixes #1154, which originally covered only the undefined "document"
variable in the HTMLElement constructor, but expanded on further
investigation to reveal that the spec's current choice of node document
for Audio, Image, and Option was strange and not implemented in
browsers.

While there, cleans up all three of these named constructors to
correctly use "create an element," and to be in numbered algorithm form
instead of giant-prose-block form; also gives the HTMLElement constructor
its own <dfn> and inline some of its steps.
@domenic domenic added the topic: custom elements Relates to custom elements (as defined in DOM and HTML) label May 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: custom elements Relates to custom elements (as defined in DOM and HTML)
Development

No branches or pull requests

4 participants