Bug 183583

Summary: connectedCallback called when disconnected
Product: WebKit Reporter: Justin Ridgewell <jridgewell>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: cdumez, dbates, esprehn+autocc, ews-watchlist, fred.wang, jridgewell, kangil.han, rbuis, rniwa, rwlbuis, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 11   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
test.html
none
Patch
rniwa: review-, ews-watchlist: commit-queue-
Archive of layout-test-results from ews104 for mac-sierra-wk2 none

Justin Ridgewell
Reported 2018-03-12 15:44:04 PDT
Created attachment 335654 [details] test.html Safari Version: 11.0.3 (13604.5.6) Safari Tech Preview: Release 50 (Safari 11.2, WebKit 13606.1.5) OS: Mac OS X 10.13.3 What steps will reproduce the problem? (1) Go to https://output.jsbin.com/qumape/3/quiet (2) Custom element 1 (Reparenter) defines a `connectedCallback` that will reparent all children nodes into a new div element (which it then appends to itself). (3) Custom element 2 (Child) has any connectedCallback. (4) Create a second Document tree, with the innerHTML `<x-parenter><x-child></x-child></x-parenter>` (5) Import the second Document's body (deep: true), and append the result to the host body. What is the expected result? Child's connectedCallback should not fire when it is appended to the disconnected div. It should fire when the div is appended to the Reparenter (and the Reparenter is in the host DOM tree). What happens instead? Child's connectedCallback fires when it is appended to the disconneted div (making it disconnected from DOM tree). (This is displayed by adding the h1 element to the DOM)
Attachments
test.html (1.74 KB, text/html)
2018-03-12 15:44 PDT, Justin Ridgewell 		no flags
Details
Patch (4.69 KB, patch)
2018-05-21 12:29 PDT, Rob Buis 		rniwa: review-
ews-watchlist: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.94 MB, application/zip)
2018-05-21 13:17 PDT, EWS Watchlist 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2018-03-12 18:38:40 PDT
<rdar://problem/38400478>
Rob Buis
Comment 2 2018-05-21 12:29:38 PDT
Created attachment 340871 [details] Patch
Chris Dumez
Comment 3 2018-05-21 12:49:03 PDT
I think Ryosuke should take a look.
Justin Ridgewell
Comment 4 2018-05-21 12:56:22 PDT
I forgot to link the Chrome bug I also filed: https://bugs.chromium.org/p/chromium/issues/detail?id=821195&desc=2 They closed as a won't fix, for this specific case (reparenter and child with disconnectedCallback, let's call this Case A). However, there's a [similar case](https://bugs.webkit.org/show_bug.cgi?id=183586) (reparenter and child _without_ disconnectedCallback) that shouldn't cause the bad behavior (let's call this Case B). Chrome thinks Case A should fire when disconnected, and not fire in Case B. Safari (currently) thinks both Cases should fire. Firefox thinks neither Case should fire. In all, it's very confusing to me why these Cases should be different. And given the browser disagreement, it's probably confusing to everyone else too. Case A: https://output.jsbin.com/qumape/3/quiet Case B: https://output.jsbin.com/qumape/7/quiet
Justin Ridgewell
Comment 5 2018-05-21 13:03:10 PDT
Scratch the Firefox, I forgot to enable webcomponents. Firefox thinks Case A should fire, and Case B should not.
EWS Watchlist
Comment 6 2018-05-21 13:17:28 PDT
Comment on attachment 340871 [details] Patch Attachment 340871 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7755558 New failing tests: imported/w3c/web-platform-tests/custom-elements/reactions/Range.html imported/w3c/web-platform-tests/custom-elements/adopted-callback.html imported/w3c/web-platform-tests/custom-elements/reactions/Element.html imported/w3c/web-platform-tests/custom-elements/reactions/Node.html imported/w3c/web-platform-tests/custom-elements/reactions/ChildNode.html imported/w3c/web-platform-tests/custom-elements/reactions/ParentNode.html
EWS Watchlist
Comment 7 2018-05-21 13:17:29 PDT
Created attachment 340875 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Ryosuke Niwa
Comment 8 2018-05-21 13:33:47 PDT
Comment on attachment 340871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340871&action=review I don't think this change is correct. For that matter, I believe WebKit's current behavior is in accordance with the specification. > Source/WebCore/dom/CustomElementReactionQueue.cpp:80 > - elementInterface.invokeConnectedCallback(element); > + if (element.isConnected()) > + elementInterface.invokeConnectedCallback(element); There is no spec text which mandates a check like this: https://html.spec.whatwg.org/multipage/custom-elements.html#invoke-custom-element-reactions > LayoutTests/fast/custom-elements/connected-callback.html:12 > + while (this.firstChild) { Why while?
Ryosuke Niwa
Comment 9 2018-05-21 13:35:38 PDT
As far as I've read the spec, WebKit's behavior is correct. Moving a node inside a connectedCallback or any other custom element reaction callback DOES not the already. enqueued custom element reactions. If you feel this behavior is not what you want, please file a spec issue in https://github.com/w3c/webcomponents/issues
Justin Ridgewell
Comment 10 2018-05-21 13:55:20 PDT
Please take a look at the Case B issue, too: https://bugs.webkit.org/show_bug.cgi?id=183586
Ryosuke Niwa
Comment 11 2018-05-21 14:14:00 PDT
(In reply to Justin Ridgewell from comment #10) > Please take a look at the Case B issue, too: > https://bugs.webkit.org/show_bug.cgi?id=183586 https://webkit.org/b/183586 is a valid bug report.
Note You need to log in before you can comment on or make changes to this bug.