RESOLVED FIXED 99232
Content element does not expose distributedNodes property 
https://bugs.webkit.org/show_bug.cgi?id=99232
Summary Content element does not expose distributedNodes property
Steve Orvell
Reported 2012-10-12 19:06:52 PDT
Created attachment 168529 [details] Reduction to test existence of <content>.distributedNodes A content element that is a shadowRoot insertion point is expected to expose a distributedNodes property. This property is not currently set. Note: This property may have been proposed as 'distributedNodes', but it should be 'distributedElements' since it can contain textNodes. Regardless, neither property is currently set.
Attachments
Reduction to test existence of <content>.distributedNodes (500 bytes, text/html)
2012-10-12 19:06 PDT, Steve Orvell 		no flags
Details
Add a distributedNodes(). (4.87 KB, patch)
2012-10-17 02:13 PDT, Hayato Ito 		no flags
DetailsFormatted DiffDiff
Call updateLayout() explicitly. (6.54 KB, patch)
2012-10-17 04:56 PDT, Hayato Ito 		no flags
DetailsFormatted DiffDiff
Renamed to getDistributedNodes(). Remove distributedNodes() from Internals. (11.51 KB, patch)
2012-10-17 19:32 PDT, Hayato Ito 		no flags
DetailsFormatted DiffDiff
Patch for landing (12.52 KB, patch)
2012-10-17 19:59 PDT, Hayato Ito 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Hayato Ito
Comment 1 2012-10-16 03:58:03 PDT
I think the corresponding bug in the spec is here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=18753 It seems we should resolve the bug in the spec at first.
Dimitri Glazkov (Google)
Comment 2 2012-10-16 07:45:34 PDT
(In reply to comment #1) > I think the corresponding bug in the spec is here: > https://www.w3.org/Bugs/Public/show_bug.cgi?id=18753 > > It seems we should resolve the bug in the spec at first. Sure, but just like with the rest of reprojection, the spec bug doesn't have to block this bug. Besides, you already have this accessor on HTMLContentElement.
Hayato Ito
Comment 3 2012-10-16 20:02:43 PDT
Okay. Let's expose the attribute. That won't break anything. (In reply to comment #2) > (In reply to comment #1) > > I think the corresponding bug in the spec is here: > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=18753 > > > > It seems we should resolve the bug in the spec at first. > > Sure, but just like with the rest of reprojection, the spec bug doesn't have to block this bug. Besides, you already have this accessor on HTMLContentElement.
Dimitri Glazkov (Google)
Comment 4 2012-10-16 20:04:40 PDT
(In reply to comment #3) > Okay. Let's expose the attribute. That won't break anything. > > (In reply to comment #2) > > (In reply to comment #1) > > > I think the corresponding bug in the spec is here: > > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=18753 > > > > > > It seems we should resolve the bug in the spec at first. > > > > Sure, but just like with the rest of reprojection, the spec bug doesn't have to block this bug. Besides, you already have this accessor on HTMLContentElement. Great! Also -- the item in this nodelist could certainly be an node, so it should be called distributedNodes.
Hajime Morrita
Comment 5 2012-10-16 20:20:05 PDT
It looks that the spec is going to make it a dynamic list, but this one isn't. so it will change the behavior once it's written out to the standard. Is it in your mind?
Dimitri Glazkov (Google)
Comment 6 2012-10-16 20:44:06 PDT
(In reply to comment #5) > It looks that the spec is going to make it a dynamic list, but this one isn't. > so it will change the behavior once it's written out to the standard. > Is it in your mind? I am not going to make this a dynamic list. Dynamic lists are evil.
Shinya Kawanaka
Comment 7 2012-10-16 21:12:59 PDT
The current implementation assumes that an element is attached when distributedNode() is called. So it won't work if it's called like: var sr = new WebKitShadowRoot(host); var content = document.createElement('content'); sr.appendChild(content); content.distributedNodes This will always return null. It's ok if we use setTimeout like the following: var sr = new WebKitShadowRoot(host); var content = document.createElement('content'); sr.appendChild(content); setTimeout(function() content.distributedNodes }, 0) Maybe we have to call ensureDistribution() when distributedNodes is called. We also have to ensure distribution for younger shadows and so on.
Hajime Morrita
Comment 8 2012-10-16 21:48:38 PDT
> > I am not going to make this a dynamic list. Dynamic lists are evil. Hmm. Added comment on W3C bug.
Hayato Ito
Comment 9 2012-10-17 02:13:44 PDT
Created attachment 169132 [details] Add a distributedNodes().
Shinya Kawanaka
Comment 10 2012-10-17 02:17:30 PDT
Comment on attachment 169132 [details] Add a distributedNodes(). View in context: https://bugs.webkit.org/attachment.cgi?id=169132&action=review > Source/WebCore/html/shadow/InsertionPoint.cpp:94 > + root->owner()->ensureDistribution(); Could you add a test case for nested shadow dom? I'm afraid that this causes an error (or returns nonsense result) If distributedNodes() is called before attaching parent shadow dom.
Hayato Ito
Comment 11 2012-10-17 02:27:53 PDT
Okay. It seems we should call ensureDistribution *recursively*. Let me update the patch. (In reply to comment #10) > (From update of attachment 169132 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169132&action=review > > > Source/WebCore/html/shadow/InsertionPoint.cpp:94 > > + root->owner()->ensureDistribution(); > > Could you add a test case for nested shadow dom? > I'm afraid that this causes an error (or returns nonsense result) If distributedNodes() is called before attaching parent shadow dom.
Hayato Ito
Comment 12 2012-10-17 04:56:16 PDT
Created attachment 169159 [details] Call updateLayout() explicitly.
Dimitri Glazkov (Google)
Comment 13 2012-10-17 10:11:50 PDT
Comment on attachment 169159 [details] Call updateLayout() explicitly. View in context: https://bugs.webkit.org/attachment.cgi?id=169159&action=review > Source/WebCore/html/shadow/HTMLContentElement.idl:33 > + NodeList distributedNodes(); I don't understand why this is a method? It should be an attribute, just like Node.childNodes or MutationRecord.addedNodes/removedNodes. We may want to optimize not to re-create the array on each access though.
Dimitri Glazkov (Google)
Comment 14 2012-10-17 10:13:38 PDT
Another thing -- probably should include removing Internals::distributedNodes in this patch?
Ojan Vafai
Comment 15 2012-10-17 10:20:23 PDT
Comment on attachment 169159 [details] Call updateLayout() explicitly. View in context: https://bugs.webkit.org/attachment.cgi?id=169159&action=review >> Source/WebCore/html/shadow/HTMLContentElement.idl:33 >> + NodeList distributedNodes(); > > I don't understand why this is a method? It should be an attribute, just like Node.childNodes or MutationRecord.addedNodes/removedNodes. We may want to optimize not to re-create the array on each access though. Is this a static NodeList? If so, we should make it so. Also, once it's static, I think we want it to be a method since each call will return a different list.
Dimitri Glazkov (Google)
Comment 16 2012-10-17 10:50:26 PDT
(In reply to comment #15) > (From update of attachment 169159 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169159&action=review > > >> Source/WebCore/html/shadow/HTMLContentElement.idl:33 > >> + NodeList distributedNodes(); > Is this a static NodeList? If so, we should make it so. Also, once it's static, I think we want it to be a method since each call will return a different list. I spoke with Ojan, and he convinced me it needs to be a method. Can we name it getDistributedNodes to make it action-oriented?
Erik Arvidsson
Comment 18 2012-10-17 12:04:20 PDT
(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 169159 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=169159&action=review > > > > >> Source/WebCore/html/shadow/HTMLContentElement.idl:33 > > >> + NodeList distributedNodes(); > > > Is this a static NodeList? If so, we should make it so. Also, once it's static, I think we want it to be a method since each call will return a different list. > > I spoke with Ojan, and he convinced me it needs to be a method. Can we name it getDistributedNodes to make it action-oriented? For the record we are having a related discussion regarding ES6 APIs. One thing that we managed to agree on is that property getters must be idempotent. node.distributedNodes === node.distributedNodes so if it returns different static NodeLists every time it MUST not be a property (WebIDL attribute).
Hayato Ito
Comment 19 2012-10-17 19:32:38 PDT
Created attachment 169325 [details] Renamed to getDistributedNodes(). Remove distributedNodes() from Internals.
Dimitri Glazkov (Google)
Comment 20 2012-10-17 19:34:13 PDT
Comment on attachment 169325 [details] Renamed to getDistributedNodes(). Remove distributedNodes() from Internals. yay!
Build Bot
Comment 21 2012-10-17 19:40:05 PDT
Comment on attachment 169325 [details] Renamed to getDistributedNodes(). Remove distributedNodes() from Internals. Attachment 169325 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14396829
Hayato Ito
Comment 22 2012-10-17 19:59:40 PDT
Created attachment 169326 [details] Patch for landing
WebKit Review Bot
Comment 23 2012-10-17 21:00:05 PDT
Comment on attachment 169326 [details] Patch for landing Clearing flags on attachment: 169326 Committed r131701: <http://trac.webkit.org/changeset/131701>
WebKit Review Bot
Comment 24 2012-10-17 21:00:12 PDT
All reviewed patches have been landed. Closing bug.
Eric Carlson
Comment 25 2012-10-18 10:49:32 PDT
It looks like this has broken the Windows build: http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/
Dimitri Glazkov (Google)
Comment 26 2012-10-18 10:55:20 PDT
(In reply to comment #25) > It looks like this has broken the Windows build: http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/ Oops, sorry about that! It's looking for a symbol that was removed, and it looks like the internals lib was not recompiled. Smells like VS build didn't understand this change in dependency. Can we clobber?
Roger Fong
Comment 27 2012-10-18 13:01:11 PDT
Clean build is failing locally on my machine. Bots still haven't finished it seems. I got rid of he distributedNodes error by getting rid of the export line in WebKit2.def. Not sure why the OpaqueJSString errors are there though. THey don'y seem related?
Dimitri Glazkov (Google)
Comment 28 2012-10-18 13:02:20 PDT
(In reply to comment #27) > Clean build is failing locally on my machine. Bots still haven't finished it seems. > > I got rid of he distributedNodes error by getting rid of the export line in WebKit2.def. That was it! I am so sorry. > > Not sure why the OpaqueJSString errors are there though. THey don'y seem related? No, those aren't related.
Roger Fong
Comment 29 2012-10-18 13:17:24 PDT
No worries! Yeah, the other two failures look like they're caused by a different patch that landed this morning so we're all clear here :) Thanks
Hayato Ito
Comment 30 2012-10-18 18:43:36 PDT
Ops, sorry about that. commit-queue could not catch such an error. Lesson: I have to update a list of exported symbols when we remove an entry from Internals.idl. (In reply to comment #25) > It looks like this has broken the Windows build: http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/
Hajime Morrita
Comment 31 2012-12-06 00:32:09 PST
*** Bug 104229 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.