Bug 78473 - ShadowRoot needs innerHTML
Summary: ShadowRoot needs innerHTML
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 63601
  Show dependency treegraph
 
Reported: 2012-02-13 01:16 PST by Shinya Kawanaka
Modified: 2012-02-29 13:17 PST (History)
11 users (show)

See Also:


Attachments
WIP (3.76 KB, patch)
2012-02-16 02:53 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Patch (10.57 KB, patch)
2012-02-17 02:05 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Patch with layout tests (12.84 KB, patch)
2012-02-17 05:12 PST, Kaustubh Atrawalkar
morrita: review-
morrita: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (16.42 KB, patch)
2012-02-21 02:07 PST, Kaustubh Atrawalkar
morrita: review-
morrita: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (16.79 KB, patch)
2012-02-23 02:54 PST, Kaustubh Atrawalkar
morrita: review-
morrita: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (17.32 KB, patch)
2012-02-23 23:15 PST, Kaustubh Atrawalkar
morrita: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (17.28 KB, patch)
2012-02-28 06:50 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Updated Patch (18.44 KB, patch)
2012-02-28 23:18 PST, Kaustubh Atrawalkar
morrita: review-
morrita: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (17.20 KB, patch)
2012-02-29 00:22 PST, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 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
Comment 1 Kaustubh Atrawalkar 2012-02-16 02:53:06 PST
Created attachment 127343 [details]
WIP

I wish to implement this. Attaching WIP for the same.
Comment 2 Hajime Morrita 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.
Comment 3 Kaustubh Atrawalkar 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?
Comment 4 Hayato Ito 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?
Comment 5 Kaustubh Atrawalkar 2012-02-17 05:12:42 PST
Created attachment 127573 [details]
Patch with layout tests

Added test case.
Comment 6 Shinya Kawanaka 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'.
Comment 7 Hajime Morrita 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.
Comment 8 Kaustubh Atrawalkar 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.
Comment 9 Hajime Morrita 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?
Comment 10 Kaustubh Atrawalkar 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.
Comment 11 Roland Steiner 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?
Comment 12 Hajime Morrita 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.
Comment 13 Kaustubh Atrawalkar 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.
Comment 14 Hajime Morrita 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.
Comment 15 Kaustubh Atrawalkar 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.
Comment 16 Hajime Morrita 2012-02-24 02:32:52 PST
Comment on attachment 128662 [details]
Updated Patch

Looks good! let's land this.
Comment 17 Kaustubh Atrawalkar 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 :)
Comment 18 WebKit Review Bot 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
Comment 19 Kaustubh Atrawalkar 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.
Comment 20 WebKit Review Bot 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
Comment 21 Hajime Morrita 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.
Comment 22 Kaustubh Atrawalkar 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.
Comment 23 Kaustubh Atrawalkar 2012-02-28 23:18:12 PST
Created attachment 129397 [details]
Updated Patch

Guess the patch didn't made it. Updated patch
Comment 24 Hajime Morrita 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.
Comment 25 Kaustubh Atrawalkar 2012-02-29 00:22:00 PST
Created attachment 129404 [details]
Updated Patch
Comment 26 WebKit Review Bot 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.
Comment 27 Hajime Morrita 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...
Comment 28 WebKit Review Bot 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.
Comment 29 WebKit Review Bot 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
Comment 30 Kaustubh Atrawalkar 2012-02-29 02:52:37 PST
Comment on attachment 129404 [details]
Updated Patch

svn.webkit.org was down :(
Comment 31 WebKit Review Bot 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>
Comment 32 WebKit Review Bot 2012-02-29 13:17:42 PST
All reviewed patches have been landed.  Closing bug.