V1 Custom element in HTML import gets connectedCallback, does not get attributeChangedCallback |
||||||||||||
Issue descriptionChrome Version: 55.0.2845.0 canary (64-bit) OS Version: OS X 10.11.6 What steps will reproduce the problem? 1. Define a custom element with constructor(), connectedCallback(), and attributeChangedCallback() that log when called (also ensure an observedAttribute is defined for an attribute) 2. Make an import document containing the custom element tag with attribute 3. Import the document into the main document after the script What is the expected result? The element in the import document should log "constructed" and "attributeChanged", and not "connected" What happens instead of that? The element in the import document logs "constructed", "connected", and no "attributeChanged" Please provide any additional information below. Attach a screenshot if possible. See attached main document "import-test.html" which imports "import.html".
,
Aug 31 2016
,
Sep 1 2016
I did a quick repro check and attributeChanged() callback doesn't work when a custom element is created synchronously in import (by parser), with the observed attribute. On upgrade scenario, or changing attribute after its creation, it seems to work.
,
Sep 1 2016
Now taking a look.
,
Sep 1 2016
@domenic - Based on the reading in https://dom.spec.whatwg.org/#connected, would you say Custom Elements in HTML Import documents _should_ have their connectedCallback run? We had assumed that the current Canary behavior was wrong to call connectedCallback (based on how V0 worked), but the V1 spec seems to say connected applies to _any_ document. So perhaps the fact that `connectedCallback` is running in imports is correct?
,
Sep 1 2016
,
Sep 2 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/94d61015c751d3d5c6ca9deedb0637cfcbbb1283 commit 94d61015c751d3d5c6ca9deedb0637cfcbbb1283 Author: kochi <kochi@chromium.org> Date: Fri Sep 02 03:12:17 2016 Fix 'will execute script' condition for creating custom elements in imports This CL fixes the 'will execute script' flag calculation by fixing the definition lookup in HTMLConstructionSite::lookUpCustomElementDefinition(), and attributeChangedCallback() will be invoked properly for custom elements parsed and created synchronously in imports. Upgrade case is not affected by this bug. BUG= 642839 Review-Url: https://codereview.chromium.org/2297853006 Cr-Commit-Position: refs/heads/master@{#416176} [add] https://crrev.com/94d61015c751d3d5c6ca9deedb0637cfcbbb1283/third_party/WebKit/LayoutTests/custom-elements/imports/attribute-changed-callback.html [add] https://crrev.com/94d61015c751d3d5c6ca9deedb0637cfcbbb1283/third_party/WebKit/LayoutTests/custom-elements/imports/resources/attribute-parsercreate.html [add] https://crrev.com/94d61015c751d3d5c6ca9deedb0637cfcbbb1283/third_party/WebKit/LayoutTests/custom-elements/imports/resources/attribute-upgrade.html [modify] https://crrev.com/94d61015c751d3d5c6ca9deedb0637cfcbbb1283/third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp
,
Sep 2 2016
attributeChangedCallback() part is fixed by r416176 above. Canary will catch up in a day or two, and next week I intend to request merge for M54 beta. For connectedCallback(), "connected" is defined as https://dom.spec.whatwg.org/#connected > An element is connected if its shadow-including root is a document. therefore Node.isConnected returns true for elements in imports or in detached documents. So as long as connectedCallback() is defined to run "when it becomes connected" at https://html.spec.whatwg.org/multipage/scripting.html#custom-element-reactions running connectedCallback() in imports still seems correct to me. So let me close this issue as fixed for now.
,
Sep 2 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bc636d23f40160f384f451a91c265aa1bbe2915b commit bc636d23f40160f384f451a91c265aa1bbe2915b Author: kochi <kochi@chromium.org> Date: Fri Sep 02 07:52:33 2016 Add DCHECKs for catching inconsistent custom element state Without the bug fix by https://codereview.chromium.org/2297853006, in the synchronous element creation path, an element could become "custom" state without its definition. Adding DCHECKs to catch any regression for the case. With the previous bugfix rolled back, the following tests crashed with these DCHECKs. custom-elements/imports/attribute-changed-callback.html custom-elements/imports/sync-create-element-order.html BUG= 642839 Review-Url: https://codereview.chromium.org/2303193002 Cr-Commit-Position: refs/heads/master@{#416211} [modify] https://crrev.com/bc636d23f40160f384f451a91c265aa1bbe2915b/third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp
,
Sep 2 2016
Running connectedCallback does seem to be correct, although the section https://html.spec.whatwg.org/multipage/scripting.html#custom-element-reactions is a summary section that is not precise, and maybe should be marked non-normative :(. The correct part of the spec to refer to is https://dom.spec.whatwg.org/#concept-node-insert which actually enqueues the connectedCallback. As kochi@ says, it doesn't seem to distinguish between documents there, so it should work. That said, as I've emphasized a few times in other threads, you can use any semantics that is convenient for HTML imports + custom elements, since HTML imports is not standard. Any time HTML imports documents are involved, you essentially have "undefined behavior".
,
Sep 3 2016
I would like us to at least publish the behavior somewhere in prose that's accessible to other implementers. The next person who tries something like HTML Imports should be interested in Chrome's implementer experience with the old one.
,
Sep 5 2016
Yes, I wrote up the doc (in prose, not in strict manner like formal specs) at https://gist.github.com/TakayoshiKochi/93a46a10c056d2a3d70495c6fc025310 and added notes for callbacks. I will send out the link to more visible place from other vendors.
,
Sep 5 2016
Requesting merge to M54. 2 changes in this issue (r416176 and r416211) but only the former is essential, and the .cpp change in the former is one-liner and minimal. The change went in canary for a few days without crash reports and its test coverage in layout tests should be enough.
,
Sep 5 2016
Your change meets the bar and is auto-approved for M54 (branch: 2840)
,
Sep 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b3fdcae1aeaf6a1113f57cb7a35caf0cc6336c58 commit b3fdcae1aeaf6a1113f57cb7a35caf0cc6336c58 Author: Takayoshi Kochi <kochi@chromium.org> Date: Mon Sep 05 03:28:23 2016 Fix 'will execute script' condition for creating custom elements in imports This CL fixes the 'will execute script' flag calculation by fixing the definition lookup in HTMLConstructionSite::lookUpCustomElementDefinition(), and attributeChangedCallback() will be invoked properly for custom elements parsed and created synchronously in imports. Upgrade case is not affected by this bug. BUG= 642839 Review-Url: https://codereview.chromium.org/2297853006 Cr-Commit-Position: refs/heads/master@{#416176} (cherry picked from commit 94d61015c751d3d5c6ca9deedb0637cfcbbb1283) Review URL: https://codereview.chromium.org/2312613002 . Cr-Commit-Position: refs/branch-heads/2840@{#149} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [add] https://crrev.com/b3fdcae1aeaf6a1113f57cb7a35caf0cc6336c58/third_party/WebKit/LayoutTests/custom-elements/imports/attribute-changed-callback.html [add] https://crrev.com/b3fdcae1aeaf6a1113f57cb7a35caf0cc6336c58/third_party/WebKit/LayoutTests/custom-elements/imports/resources/attribute-parsercreate.html [add] https://crrev.com/b3fdcae1aeaf6a1113f57cb7a35caf0cc6336c58/third_party/WebKit/LayoutTests/custom-elements/imports/resources/attribute-upgrade.html [modify] https://crrev.com/b3fdcae1aeaf6a1113f57cb7a35caf0cc6336c58/third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp
,
Sep 6 2016
Tested the same on win10 and mac 10.11.6 using chrome version 55.0.2845.0 - observed the error "Access to imported resource has been blocked by CORS policy" in the console on loading import-test.html Please find the screenshot Could you please let us know if this can be verified from test team end. If yes, please let us know where to see the import document output.
,
Sep 6 2016
tkonchada: thanks for the testing - HTML imports needs to be provided via HTTP, so probably you can get it working by putting the files in a directly that your HTTP server can serve the files. E.g. 1. download import-test.html and import.html in a directory X 2. Run "python -m SimpleHTTPServer" on the directory X 3. access "http://localhost:8000/import-test.html" For 2 you can use anything that can serve HTTP requests.
,
Sep 6 2016
Tested as per steps in comment #17 on mac 10.11.6 using chrome version 54.0.2840.14 - Observed the output as shown in the screenshot Could you please let us know if this is the expected behaviour
,
Sep 6 2016
Favicon.ico load error is not related to this bug. The other output is what we expected. Thanks for the testing!
,
Sep 6 2016
Tested the same on win10 and Linux 14.04 using chrome version 54.0.2840.14 - observed the output as shown in the screenshot in comment#18 Fix works as expected Adding TE-Verified labels
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b3fdcae1aeaf6a1113f57cb7a35caf0cc6336c58 commit b3fdcae1aeaf6a1113f57cb7a35caf0cc6336c58 Author: Takayoshi Kochi <kochi@chromium.org> Date: Mon Sep 05 03:28:23 2016 Fix 'will execute script' condition for creating custom elements in imports This CL fixes the 'will execute script' flag calculation by fixing the definition lookup in HTMLConstructionSite::lookUpCustomElementDefinition(), and attributeChangedCallback() will be invoked properly for custom elements parsed and created synchronously in imports. Upgrade case is not affected by this bug. BUG= 642839 Review-Url: https://codereview.chromium.org/2297853006 Cr-Commit-Position: refs/heads/master@{#416176} (cherry picked from commit 94d61015c751d3d5c6ca9deedb0637cfcbbb1283) Review URL: https://codereview.chromium.org/2312613002 . Cr-Commit-Position: refs/branch-heads/2840@{#149} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [add] https://crrev.com/b3fdcae1aeaf6a1113f57cb7a35caf0cc6336c58/third_party/WebKit/LayoutTests/custom-elements/imports/attribute-changed-callback.html [add] https://crrev.com/b3fdcae1aeaf6a1113f57cb7a35caf0cc6336c58/third_party/WebKit/LayoutTests/custom-elements/imports/resources/attribute-parsercreate.html [add] https://crrev.com/b3fdcae1aeaf6a1113f57cb7a35caf0cc6336c58/third_party/WebKit/LayoutTests/custom-elements/imports/resources/attribute-upgrade.html [modify] https://crrev.com/b3fdcae1aeaf6a1113f57cb7a35caf0cc6336c58/third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by rohitrao@chromium.org
, Aug 31 2016Labels: -OS-Mac OS-All
Status: Untriaged (was: Unconfirmed)