Bug 78473 – ShadowRoot needs innerHTML

• Component: DOM
• Status: RESOLVED FIXED

[Shinya Kawanaka]

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

[Kaustubh Atrawalkar]

Created attachment 127343 [details]
WIP

I wish to implement this. Attaching WIP for the same.

[Hajime Morrita]

(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]

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]

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]

Created attachment 127573 [details]
Patch with layout tests

Added test case.

[Shinya Kawanaka]

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 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]

Created attachment 127944 [details]
Updated Patch

Updated patch with fixed test case. Added test case to Skipped for other platforms.

[Hajime Morrita]

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]

(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 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 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]

Created attachment 128447 [details]
Updated Patch

updated replaceChildrenWithFragment to use ContainerNode. Added few more use cases in test.

[Hajime Morrita]

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]

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 on attachment 128662 [details]
Updated Patch

Looks good! let's land this.

[Kaustubh Atrawalkar]

(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 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 on attachment 128662 [details]
Updated Patch

Aah..Morrita can u make c+ again? Guess commit bot failed. Retrying.

[WebKit Review Bot]

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 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]

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]

Created attachment 129397 [details]
Updated Patch

Guess the patch didn't made it. Updated patch

[Hajime Morrita]

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]

Created attachment 129404 [details]
Updated Patch

[WebKit Review Bot]

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 on attachment 129404 [details]
Updated Patch

Looks good! thanks for your cooperation. It's time to land...

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 129404 [details]:

css3/filters/effect-invert-hw.html bug 79639 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

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 on attachment 129404 [details]
Updated Patch

svn.webkit.org was down :(

[WebKit Review Bot]

Comment on attachment 129404 [details]
Updated Patch

Clearing flags on attachment: 129404

Committed r109251: <http://trac.webkit.org/changeset/109251>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.