Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2837823004:
Set active document url to <template> content URL. (Closed)
|
Created:
3 years, 8 months ago by Shanmuga Pandi Modified:
3 years, 3 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet active document url to <template> content URL.
Template element's content is documentFragment which
naturally doesn't have an URL of its Own.
So set active Document Url to it.
BUG=713012
Patch Set 1 #
Total comments: 7
Messages
Total messages: 16 (7 generated)
The CQ bit was checked by shanmuga.m@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
shanmuga.m@samsung.com changed reviewers: + foolip@chromium.org, fs@opera.com
PTAL !! Thanks
https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp (right): https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp:53: content_->GetDocument().SetURL(GetDocument().Url()); Shouldn't this rather be handled when creating the template document?
foolip@chromium.org changed reviewers: + tkent@chromium.org
tkent@ would probably be a better reviewer of this, but left my thoughts. https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/HTMLAnchorElement/anchor-in-template.html (right): https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/HTMLAnchorElement/anchor-in-template.html:10: var a = document.querySelector('template').content.querySelector('a'); Can you put this test in LayoutTests/external/wpt/html/semantics/scripting-1/the-template-element/template-element/? Whatever the right behavior is, we have a difference between Blink and Gecko, and sharing the test would be valuable. (See https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat... for some more documentation.) Trying to understand this, I wrote https://jsbin.com/babalob/edit?html,output which I think tests what the spec says, and fails in Firefox. https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp (right): https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp:53: content_->GetDocument().SetURL(GetDocument().Url()); On 2017/04/25 09:13:45, fs wrote: > Shouldn't this rather be handled when creating the template document? The relevant bit of spec: https://html.spec.whatwg.org/multipage/scripting.html#template-contents https://html.spec.whatwg.org/multipage/scripting.html#appropriate-template-co... AFAICT, there's nothing that sets that document's URL.
https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp (right): https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp:53: content_->GetDocument().SetURL(GetDocument().Url()); On 2017/04/25 at 09:23:44, foolip_UTC7 wrote: > On 2017/04/25 09:13:45, fs wrote: > > Shouldn't this rather be handled when creating the template document? > > The relevant bit of spec: > https://html.spec.whatwg.org/multipage/scripting.html#template-contents > https://html.spec.whatwg.org/multipage/scripting.html#appropriate-template-co... > > AFAICT, there's nothing that sets that document's URL. Right, https://dom.spec.whatwg.org/#interface-document only calls for 'setting' the origin. (Maybe that should be asserted by the test as well then.)
https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp (right): https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp:53: content_->GetDocument().SetURL(GetDocument().Url()); On 2017/04/25 10:20:47, fs wrote: > On 2017/04/25 at 09:23:44, foolip_UTC7 wrote: > > On 2017/04/25 09:13:45, fs wrote: > > > Shouldn't this rather be handled when creating the template document? > > > > The relevant bit of spec: > > https://html.spec.whatwg.org/multipage/scripting.html#template-contents > > > https://html.spec.whatwg.org/multipage/scripting.html#appropriate-template-co... > > > > AFAICT, there's nothing that sets that document's URL. > > Right, https://dom.spec.whatwg.org/#interface-document only calls for 'setting' > the origin. (Maybe that should be asserted by the test as well then.) Do you mean "The Document() constructor, when invoked, must return a new document whose origin is the origin of current global object’s associated Document"? It's a bit odd, but I'm pretty sure that is for the script-exposed constructor only, and that when HTML says "Let /new doc/ be a new Document", it's using a magical internal constructor.
https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp (right): https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp:53: content_->GetDocument().SetURL(GetDocument().Url()); On 2017/04/25 at 14:23:28, foolip_UTC7 wrote: > On 2017/04/25 10:20:47, fs wrote: > > On 2017/04/25 at 09:23:44, foolip_UTC7 wrote: > > > On 2017/04/25 09:13:45, fs wrote: > > > > Shouldn't this rather be handled when creating the template document? > > > > > > The relevant bit of spec: > > > https://html.spec.whatwg.org/multipage/scripting.html#template-contents > > > > > https://html.spec.whatwg.org/multipage/scripting.html#appropriate-template-co... > > > > > > AFAICT, there's nothing that sets that document's URL. > > > > Right, https://dom.spec.whatwg.org/#interface-document only calls for 'setting' > > the origin. (Maybe that should be asserted by the test as well then.) > > Do you mean "The Document() constructor, when invoked, must return a new document whose origin is the origin of current global object’s associated Document"? It's a bit odd, but I'm pretty sure that is for the script-exposed constructor only, and that when HTML says "Let /new doc/ be a new Document", it's using a magical internal constructor. That may very well be the case. But shouldn't the magic internal constructor be well-defined? (Based on the discrepancies observed here, one might argue that it isn't.)
https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp (right): https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp:53: content_->GetDocument().SetURL(GetDocument().Url()); On 2017/04/25 at 14:39:26, fs wrote: > On 2017/04/25 at 14:23:28, foolip_UTC7 wrote: > > On 2017/04/25 10:20:47, fs wrote: > > > On 2017/04/25 at 09:23:44, foolip_UTC7 wrote: > > > > On 2017/04/25 09:13:45, fs wrote: > > > > > Shouldn't this rather be handled when creating the template document? > > > > > > > > The relevant bit of spec: > > > > https://html.spec.whatwg.org/multipage/scripting.html#template-contents > > > > > > > https://html.spec.whatwg.org/multipage/scripting.html#appropriate-template-co... > > > > > > > > AFAICT, there's nothing that sets that document's URL. > > > > > > Right, https://dom.spec.whatwg.org/#interface-document only calls for 'setting' > > > the origin. (Maybe that should be asserted by the test as well then.) > > > > Do you mean "The Document() constructor, when invoked, must return a new document whose origin is the origin of current global object’s associated Document"? It's a bit odd, but I'm pretty sure that is for the script-exposed constructor only, and that when HTML says "Let /new doc/ be a new Document", it's using a magical internal constructor. > > That may very well be the case. But shouldn't the magic internal constructor be well-defined? (Based on the discrepancies observed here, one might argue that it isn't.) Found what I was looking for: 'Unless stated otherwise, a document’s encoding is the utf-8 encoding, content type is "application/xml", URL is "about:blank", origin is an opaque origin, type is "xml", and its mode is "no-quirks".' So, our current behavior seems right.
On 2017/04/25 14:51:08, fs wrote: > https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp (right): > > https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp:53: > content_->GetDocument().SetURL(GetDocument().Url()); > On 2017/04/25 at 14:39:26, fs wrote: > > On 2017/04/25 at 14:23:28, foolip_UTC7 wrote: > > > On 2017/04/25 10:20:47, fs wrote: > > > > On 2017/04/25 at 09:23:44, foolip_UTC7 wrote: > > > > > On 2017/04/25 09:13:45, fs wrote: > > > > > > Shouldn't this rather be handled when creating the template document? > > > > > > > > > > The relevant bit of spec: > > > > > https://html.spec.whatwg.org/multipage/scripting.html#template-contents > > > > > > > > > > https://html.spec.whatwg.org/multipage/scripting.html#appropriate-template-co... > > > > > > > > > > AFAICT, there's nothing that sets that document's URL. > > > > > > > > Right, https://dom.spec.whatwg.org/#interface-document only calls for > 'setting' > > > > the origin. (Maybe that should be asserted by the test as well then.) > > > > > > Do you mean "The Document() constructor, when invoked, must return a new > document whose origin is the origin of current global object’s associated > Document"? It's a bit odd, but I'm pretty sure that is for the script-exposed > constructor only, and that when HTML says "Let /new doc/ be a new Document", > it's using a magical internal constructor. > > > > That may very well be the case. But shouldn't the magic internal constructor > be well-defined? (Based on the discrepancies observed here, one might argue that > it isn't.) > > Found what I was looking for: > > 'Unless stated otherwise, a document’s encoding is the utf-8 encoding, content > type is "application/xml", URL is "about:blank", origin is an opaque origin, > type is "xml", and its mode is "no-quirks".' > > So, our current behavior seems right. Thank you fs@ and @fooip. Shall we mark the bug as "WontFix" ?
On 2017/04/26 05:57:51, Shanmuga Pandi wrote: > On 2017/04/25 14:51:08, fs wrote: > > > https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp (right): > > > > > https://codereview.chromium.org/2837823004/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/html/HTMLTemplateElement.cpp:53: > > content_->GetDocument().SetURL(GetDocument().Url()); > > On 2017/04/25 at 14:39:26, fs wrote: > > > On 2017/04/25 at 14:23:28, foolip_UTC7 wrote: > > > > On 2017/04/25 10:20:47, fs wrote: > > > > > On 2017/04/25 at 09:23:44, foolip_UTC7 wrote: > > > > > > On 2017/04/25 09:13:45, fs wrote: > > > > > > > Shouldn't this rather be handled when creating the template > document? > > > > > > > > > > > > The relevant bit of spec: > > > > > > > https://html.spec.whatwg.org/multipage/scripting.html#template-contents > > > > > > > > > > > > > > https://html.spec.whatwg.org/multipage/scripting.html#appropriate-template-co... > > > > > > > > > > > > AFAICT, there's nothing that sets that document's URL. > > > > > > > > > > Right, https://dom.spec.whatwg.org/#interface-document only calls for > > 'setting' > > > > > the origin. (Maybe that should be asserted by the test as well then.) > > > > > > > > Do you mean "The Document() constructor, when invoked, must return a new > > document whose origin is the origin of current global object’s associated > > Document"? It's a bit odd, but I'm pretty sure that is for the script-exposed > > constructor only, and that when HTML says "Let /new doc/ be a new Document", > > it's using a magical internal constructor. > > > > > > That may very well be the case. But shouldn't the magic internal constructor > > be well-defined? (Based on the discrepancies observed here, one might argue > that > > it isn't.) > > > > Found what I was looking for: > > > > 'Unless stated otherwise, a document’s encoding is the utf-8 encoding, content > > type is "application/xml", URL is "about:blank", origin is an opaque origin, > > type is "xml", and its mode is "no-quirks".' > > > > So, our current behavior seems right. > > Thank you fs@ and @fooip. > Shall we mark the bug as "WontFix" ? Yes, but there is something interesting here. If you could write just a test under LayoutTests/external/wpt/html/semantics/scripting-1/the-template-element/template-element/ that already passes for us but fails in Firefox, that'd be great. Something like https://jsbin.com/babalob/edit?html,output Filing a Gecko bug pointing to that test would also increase the chances of a fix.
Description was changed from ========== Set active document url to <template> content URL. Template element's content is documentFragment which naturally doesn't have an URL of its Own. So set active Document Url to it. BUG=713012 ========== to ========== Set active document url to <template> content URL. Template element's content is documentFragment which naturally doesn't have an URL of its Own. So set active Document Url to it. BUG=713012 ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
