Bug 62184

Summary: ShadowRoot.getElementById() returns removed node.
Product: WebKit Reporter: Kent Tamura <tkent>
Component: DOMAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, dglazkov, dominicc, morrita, rniwa, rolandsteiner, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 59832    
Attachments:
Description Flags
Test
none
Patch none

Kent Tamura
Reported 2011-06-06 22:07:50 PDT
tkent doesn't like registering a shadow ID in insertedIntoDocument() and unregistering it in removedFromDocument(). See https://bugs.webkit.org/show_bug.cgi?id=62116#c5
Attachments
Test (3.31 KB, text/html)
2012-02-23 01:24 PST, Roland Steiner 		no flags
Details
Patch (6.55 KB, patch)
2012-05-29 03:57 PDT, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Hajime Morrita
Comment 1 2011-06-06 22:20:48 PDT
I'm sorry for the lack of response on this issue. Another possible idea is to have insertedIntoScope() in addition or instead of insertedIntoTree(). As we pulled some functionality up from Document to TreeScope, associated part on Element and Node is also need to be generalized.
Roland Steiner
Comment 2 2011-06-06 22:37:20 PDT
I added a comment at https://bugs.webkit.org/show_bug.cgi?id=62116#c14
Roland Steiner
Comment 3 2011-06-06 22:45:14 PDT
In Summary: If an element has an ID it just unconditionally calls treeScope()->registerID() from insertedIntoTree(), and treeScope()->unregisterID() from willRemove(). Nodes that are not in the Document are inserted into a new TreeScope-derived class DocumentAssociated, of which there is exactly 1 instance per Document, owned by the Document. DocumentAssociated::registerID()/unregisterID() are no-ops. With this, we can also get rid of all the instances where inDocument() is queried.
Dimitri Glazkov (Google)
Comment 4 2011-06-07 10:10:38 PDT
(In reply to comment #3) > In Summary: > > If an element has an ID it just unconditionally calls treeScope()->registerID() from insertedIntoTree(), and treeScope()->unregisterID() from willRemove(). > > Nodes that are not in the Document are inserted into a new TreeScope-derived class DocumentAssociated, of which there is exactly 1 instance per Document, owned by the Document. I remember you talking about this a while back. It seems like a pretty cool idea. What are the compatibility implications? Any observable changes in behavior - especially if we introduce treeScope() accessor on an Element as public API? > > DocumentAssociated::registerID()/unregisterID() are no-ops. > > With this, we can also get rid of all the instances where inDocument() is queried.
Roland Steiner
Comment 5 2011-06-07 17:19:51 PDT
(In reply to comment #4) > I remember you talking about this a while back. It seems like a pretty cool idea. What are the compatibility implications? Any observable changes in behavior - especially if we introduce treeScope() accessor on an Element as public API? I guess it'll have the same implications as the shadow root if we allow it to be reached from JS (by accessing it directly via treeScope, or by climbing the parent pointers) - i.e., mainly what is the node type and IDL of this DocumentAssociated node?
Roland Steiner
Comment 6 2011-06-07 19:47:44 PDT
Actually, we might be able to just use a DocumentFragment for this if likewise we extend DocumentFragment to also be a TreeScope.
Hajime Morrita
Comment 7 2012-02-14 20:39:13 PST
Now TreeScope is landed. We need to test the scope is working as intended.
Roland Steiner
Comment 8 2012-02-23 01:24:24 PST
Created attachment 128431 [details] Test Turns out, removing elements from a shadow DOM doesn't properly update the ID cache. See attached test file (using removeChild) and also the test bug 78473 attachment 127944 [details] (using innerHTML).
Hajime Morrita
Comment 9 2012-05-29 03:57:14 PDT
Created attachment 144505 [details] Patch
Hajime Morrita
Comment 10 2012-05-29 03:58:03 PDT
Hi Ryosuke, Dimitri, could you take a look? It's a simple bug fix.
WebKit Review Bot
Comment 11 2012-05-29 10:50:21 PDT
Comment on attachment 144505 [details] Patch Clearing flags on attachment: 144505 Committed r118804: <http://trac.webkit.org/changeset/118804>
WebKit Review Bot
Comment 12 2012-05-29 10:50:27 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.