Bug 62184 - ShadowRoot.getElementById() returns removed node.
: ShadowRoot.getElementById() returns removed node.
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 59832
  Show dependency treegraph
 
Reported: 2011-06-06 22:07 PST by
Modified: 2012-05-29 10:50 PST (History)


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 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-06-06 22:07:50 PST
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
------- Comment #1 From 2011-06-06 22:20:48 PST -------
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.
------- Comment #2 From 2011-06-06 22:37:20 PST -------
I added a comment at https://bugs.webkit.org/show_bug.cgi?id=62116#c14
------- Comment #3 From 2011-06-06 22:45:14 PST -------
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.
------- Comment #4 From 2011-06-07 10:10:38 PST -------
(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.
------- Comment #5 From 2011-06-07 17:19:51 PST -------
(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?
------- Comment #6 From 2011-06-07 19:47:44 PST -------
Actually, we might be able to just use a DocumentFragment for this if likewise we extend DocumentFragment to also be a TreeScope.
------- Comment #7 From 2012-02-14 20:39:13 PST -------
Now TreeScope is landed. We need to test the scope is working as intended.
------- Comment #8 From 2012-02-23 01:24:24 PST -------
Created an attachment (id=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).
------- Comment #9 From 2012-05-29 03:57:14 PST -------
Created an attachment (id=144505) [details]
Patch
------- Comment #10 From 2012-05-29 03:58:03 PST -------
Hi Ryosuke, Dimitri, could you take a look?
It's a simple bug fix.
------- Comment #11 From 2012-05-29 10:50:21 PST -------
(From update of attachment 144505 [details])
Clearing flags on attachment: 144505

Committed r118804: <http://trac.webkit.org/changeset/118804>
------- Comment #12 From 2012-05-29 10:50:27 PST -------
All reviewed patches have been landed.  Closing bug.