RESOLVED FIXED 78473
ShadowRoot needs innerHTML
https://bugs.webkit.org/show_bug.cgi?id=78473
Summary ShadowRoot needs innerHTML
Shinya Kawanaka
Reported 2012-02-13 01:16:10 PST
Currently ShadowRoot does not have innerHTML API. We should have this. See spec: http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#shadow-root-attributes
Attachments
WIP (3.76 KB, patch)
2012-02-16 02:53 PST, Kaustubh Atrawalkar
no flags
Patch (10.57 KB, patch)
2012-02-17 02:05 PST, Kaustubh Atrawalkar
no flags
Patch with layout tests (12.84 KB, patch)
2012-02-17 05:12 PST, Kaustubh Atrawalkar
morrita: review-
morrita: commit-queue-
Updated Patch (16.42 KB, patch)
2012-02-21 02:07 PST, Kaustubh Atrawalkar
morrita: review-
morrita: commit-queue-
Updated Patch (16.79 KB, patch)
2012-02-23 02:54 PST, Kaustubh Atrawalkar
morrita: review-
morrita: commit-queue-
Updated Patch (17.32 KB, patch)
2012-02-23 23:15 PST, Kaustubh Atrawalkar
morrita: review-
webkit.review.bot: commit-queue-
Updated Patch (17.28 KB, patch)
2012-02-28 06:50 PST, Kaustubh Atrawalkar
no flags
Updated Patch (18.44 KB, patch)
2012-02-28 23:18 PST, Kaustubh Atrawalkar
morrita: review-
morrita: commit-queue-
Updated Patch (17.20 KB, patch)
2012-02-29 00:22 PST, Kaustubh Atrawalkar
no flags
Kaustubh Atrawalkar
Comment 1 2012-02-16 02:53:06 PST
Created attachment 127343 [details] WIP I wish to implement this. Attaching WIP for the same.
Hajime Morrita
Comment 2 2012-02-16 03:05:37 PST
(In reply to comment #1) > Created an attachment (id=127343) [details] > WIP > > I wish to implement this. Attaching WIP for the same. Great!!! Could you consider to extract existing implementation and share it with this? Having two similar code tend to have a trouble in future change.
Kaustubh Atrawalkar
Comment 3 2012-02-17 02:05:46 PST
Created attachment 127555 [details] Patch Update patch as per Morita's suggestion to share common code between HTMLElement & ShadowRoot. Moved code to markup as its relevant code and can be re-factored to reside there. Can u review once?
Hayato Ito
Comment 4 2012-02-17 02:27:48 PST
You need to write tests when adding APIs. Could you write Layout tests for new APIs? (In reply to comment #3) > Created an attachment (id=127555) [details] > Patch > > Update patch as per Morita's suggestion to share common code between HTMLElement & ShadowRoot. Moved code to markup as its relevant code and can be re-factored to reside there. Can u review once?
Kaustubh Atrawalkar
Comment 5 2012-02-17 05:12:42 PST
Created attachment 127573 [details] Patch with layout tests Added test case.
Shinya Kawanaka
Comment 6 2012-02-20 21:25:03 PST
Comment on attachment 127573 [details] Patch with layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=127573&action=review > LayoutTests/fast/dom/shadow/shadow-root-innerHTML.html:20 > +root = new WebKitShadowRoot(div); 'new WebKitShadowRoot()' is enabled only if SHADOW_DOM flag is enabled. Actually this flag is currently enabled in chromium-port. So you add this test in skipped lists. See LayoutTests/platform/<port>/Skipped and add test name in it. <port> will be 'efl', 'gtk', 'qt', 'mac', 'win', 'wincairo', and 'wk2'.
Hajime Morrita
Comment 7 2012-02-20 22:18:35 PST
Comment on attachment 127573 [details] Patch with layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=127573&action=review > Source/WebCore/dom/ShadowRoot.cpp:156 > + replaceChildrenWithFragment(host(), fragment.release(), ec); No, this is not what we need. we need to inject fragment to this ShadowRoot, not the host. > LayoutTests/fast/dom/shadow/shadow-root-innerHTML.html:24 > +shouldBeDefined("root.innerHTML"); This is not enough. We need to test - the resulted string of innerHTML after setting innerHTML. - the resulted DOM tree after setting innerHTML - the innerHTML value for existing shadow DOM tree.
Kaustubh Atrawalkar
Comment 8 2012-02-21 02:07:07 PST
Created attachment 127944 [details] Updated Patch Updated patch with fixed test case. Added test case to Skipped for other platforms.
Hajime Morrita
Comment 9 2012-02-21 15:55:16 PST
Comment on attachment 127944 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127944&action=review > Source/WebCore/dom/ShadowRoot.cpp:157 > + replaceChildrenWithFragment(toElement(this), fragment.release(), ec); Does this really work? > LayoutTests/fast/dom/shadow/shadow-root-innerHTML.html:36 > + I'm confused. Why is div2 available even after setting another innerHTML?
Kaustubh Atrawalkar
Comment 10 2012-02-21 23:37:09 PST
(In reply to comment #9) > (From update of attachment 127944 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127944&action=review > > > Source/WebCore/dom/ShadowRoot.cpp:157 > > + replaceChildrenWithFragment(toElement(this), fragment.release(), ec); > > Does this really work? > I am assuming this works as DocumentFragment is derived from ContainerNode. > > LayoutTests/fast/dom/shadow/shadow-root-innerHTML.html:36 > > + > > I'm confused. Why is div2 available even after setting another innerHTML? Its only to show that it resides in the dom tree of the ShadowRoot. After setting innerHTML, the children gets replaced with the fragment.
Roland Steiner
Comment 11 2012-02-22 03:45:28 PST
Comment on attachment 127944 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127944&action=review >>> Source/WebCore/dom/ShadowRoot.cpp:157 >>> + replaceChildrenWithFragment(toElement(this), fragment.release(), ec); >> >> Does this really work? > > I am assuming this works as DocumentFragment is derived from ContainerNode. Even so this is beyond fragile. It only happens to work because currently replaceChildrenWithFragment uses ContainerNode methods only. I suggest making the parameter a ContainerNode* and do away with toElement(). Then any changes to to replaceChildrenWithFragment will break at compile time rather than runtime. > Source/WebCore/editing/markup.cpp:940 > +PassRefPtr<DocumentFragment> createFragmentFromSource(const String& markup, Element* element, ExceptionCode& ec) editing/ doesn't seem the right place to me for these methods. IMHO they'd better be moved to dom/ - either into ContainerNode or, better yet, into a separate file. >>> LayoutTests/fast/dom/shadow/shadow-root-innerHTML.html:36 >>> + >> >> I'm confused. Why is div2 available even after setting another innerHTML? > > Its only to show that it resides in the dom tree of the ShadowRoot. After setting innerHTML, the children gets replaced with the fragment. I have to say I'm likewise confused - as you say, it should get replaced and no longer be in the tree. So why is it still accessible via getElementById?
Hajime Morrita
Comment 12 2012-02-22 16:58:28 PST
Comment on attachment 127944 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127944&action=review >> Source/WebCore/editing/markup.cpp:940 >> +PassRefPtr<DocumentFragment> createFragmentFromSource(const String& markup, Element* element, ExceptionCode& ec) > > editing/ doesn't seem the right place to me for these methods. IMHO they'd better be moved to dom/ - either into ContainerNode or, better yet, into a separate file. I think this is OK for this time if we have a bug to move this to appropriate place. >>>> LayoutTests/fast/dom/shadow/shadow-root-innerHTML.html:36 >>>> + >>> >>> I'm confused. Why is div2 available even after setting another innerHTML? >> >> Its only to show that it resides in the dom tree of the ShadowRoot. After setting innerHTML, the children gets replaced with the fragment. > > I have to say I'm likewise confused - as you say, it should get replaced and no longer be in the tree. So why is it still accessible via getElementById? I tried this locally and it triggered an assertion failure on the debug build.
Kaustubh Atrawalkar
Comment 13 2012-02-23 02:54:02 PST
Created attachment 128447 [details] Updated Patch updated replaceChildrenWithFragment to use ContainerNode. Added few more use cases in test.
Hajime Morrita
Comment 14 2012-02-23 17:42:16 PST
Comment on attachment 128447 [details] Updated Patch Almost looks good! Could you add another test case which ensures that setting a HTML fragment not only results a innerHTML value change, but also actually creates DOM nodes? we can test it even without (currently broken) getElementId() or something. querySelector() or just traversing the tree by firstChild etc. will be sufficient.
Kaustubh Atrawalkar
Comment 15 2012-02-23 23:15:01 PST
Created attachment 128662 [details] Updated Patch Added test case to check actual dom tree getting created after setting innerHTML fragment to ShadowRoot.
Hajime Morrita
Comment 16 2012-02-24 02:32:52 PST
Comment on attachment 128662 [details] Updated Patch Looks good! let's land this.
Kaustubh Atrawalkar
Comment 17 2012-02-24 02:44:09 PST
(In reply to comment #16) > (From update of attachment 128662 [details]) > Looks good! let's land this. Thanks Morrita for immediate review :)
WebKit Review Bot
Comment 18 2012-02-24 04:35:45 PST
Comment on attachment 128662 [details] Updated Patch Rejecting attachment 128662 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: .html patching file LayoutTests/platform/gtk/Skipped patching file LayoutTests/platform/mac/Skipped patching file LayoutTests/platform/qt/Skipped Hunk #1 succeeded at 166 (offset 1 line). patching file LayoutTests/platform/win/Skipped patching file LayoutTests/platform/wincairo/Skipped patching file LayoutTests/platform/wk2/Skipped Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Hajime Mor..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/11603939
Kaustubh Atrawalkar
Comment 19 2012-02-24 06:14:00 PST
Comment on attachment 128662 [details] Updated Patch Aah..Morrita can u make c+ again? Guess commit bot failed. Retrying.
WebKit Review Bot
Comment 20 2012-02-26 17:12:55 PST
Comment on attachment 128662 [details] Updated Patch Rejecting attachment 128662 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: le LayoutTests/platform/gtk/Skipped.rej patching file LayoutTests/platform/mac/Skipped patching file LayoutTests/platform/qt/Skipped Hunk #1 succeeded at 167 with fuzz 1 (offset 2 lines). patching file LayoutTests/platform/win/Skipped patching file LayoutTests/platform/wincairo/Skipped patching file LayoutTests/platform/wk2/Skipped Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Hajime Mor..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/11627819
Hajime Morrita
Comment 21 2012-02-26 20:45:27 PST
Comment on attachment 128662 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128662&action=review Tried this again. And found that this doesn't work. Please test this using debug build so that you can see the assertion failure. > Source/WebCore/dom/ShadowRoot.cpp:154 > + RefPtr<DocumentFragment> fragment = createFragmentFromSource(markup, toElement(this), ec); You cannot use toElement() here.
Kaustubh Atrawalkar
Comment 22 2012-02-28 06:50:27 PST
Created attachment 129239 [details] Updated Patch For creation of DocumentFragmet I can use host() as HTMLDocumentParser currently take Element as reference to create HTMLTree. The injected fragment will be replaced on actual ShadowRoot only. Tested on debug build. Morrita can u review once? Thanks.
Kaustubh Atrawalkar
Comment 23 2012-02-28 23:18:12 PST
Created attachment 129397 [details] Updated Patch Guess the patch didn't made it. Updated patch
Hajime Morrita
Comment 24 2012-02-28 23:37:49 PST
Comment on attachment 129397 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129397&action=review > Source/WebCore/editing/markup.h:30 > +#include "ExceptionCode.h" You can just typedef int ExceptionCode. instead of including this. Same for ContainerNode.h. Less includes are better. > LayoutTests/platform/mac/Skipped:426 > +fast/dom/shadow Please don do this unless you are a maintainer of this port. There some test which can run without ENABLE_SHADOW_DOM under shadow/ We should've splited these from one which needs API. But it's another story. > LayoutTests/platform/qt/Skipped:162 > +fast/dom/shadow Ditto. > LayoutTests/platform/win/Skipped:1452 > +fast/dom/shadow Ditto. > LayoutTests/platform/wincairo/Skipped:1968 > Ditto. > LayoutTests/platform/wk2/Skipped:1066 > +fast/dom/shadow Ditto.
Kaustubh Atrawalkar
Comment 25 2012-02-29 00:22:00 PST
Created attachment 129404 [details] Updated Patch
WebKit Review Bot
Comment 26 2012-02-29 00:25:19 PST
Attachment 129404 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/editing/markup.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Hajime Morrita
Comment 27 2012-02-29 00:47:42 PST
Comment on attachment 129404 [details] Updated Patch Looks good! thanks for your cooperation. It's time to land...
WebKit Review Bot
Comment 28 2012-02-29 02:43:17 PST
The commit-queue encountered the following flaky tests while processing attachment 129404 [details]: css3/filters/effect-invert-hw.html bug 79639 (author: cmarrin@apple.com) The commit-queue is continuing to process your patch.
WebKit Review Bot
Comment 29 2012-02-29 02:47:03 PST
Comment on attachment 129404 [details] Updated Patch Rejecting attachment 129404 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295 Died at Tools/Scripts/update-webkit line 164. Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 9 Updating OpenSource RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295 Died at Tools/Scripts/update-webkit line 164. Full output: http://queues.webkit.org/results/11695257
Kaustubh Atrawalkar
Comment 30 2012-02-29 02:52:37 PST
Comment on attachment 129404 [details] Updated Patch svn.webkit.org was down :(
WebKit Review Bot
Comment 31 2012-02-29 13:17:36 PST
Comment on attachment 129404 [details] Updated Patch Clearing flags on attachment: 129404 Committed r109251: <http://trac.webkit.org/changeset/109251>
WebKit Review Bot
Comment 32 2012-02-29 13:17:42 PST
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.