Bug 77930 – INPUT shouldn't create ShadowRoot dynamically.

• Component: DOM
• Status: RESOLVED FIXED

[Shinya Kawanaka]

Currently INPUT recreates shadow root if its type is changed.
We can reuse shadow root to stop recreating it.

[Shinya Kawanaka]

Created attachment 126017 [details]
W.I.P.

[Shinya Kawanaka]

Created attachment 126018 [details]
W.I.P.

[WebKit Review Bot]

Comment on attachment 126018 [details]
W.I.P.

Attachment 126018 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11460321

New failing tests:
fast/speech/speech-input-scripting.html

[Shinya Kawanaka]

Created attachment 126420 [details]
Patch

[Dominic Cooney]

Comment on attachment 126420 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126420&action=review

> Source/WebCore/ChangeLog:12
> +        Media control elements are implemented input elements. These elements should also

The meaning of "Media control elements are implemented input elements" is not clear to me.

> Source/WebCore/html/FileInputType.cpp:270
> +    element()->shadowRoot()->appendChild(element()->multiple() ? UploadButtonElement::createForMultiple(element()->document()): UploadButtonElement::create(element()->document()), ec);

Won’t this be problematic if I attach an author shadow root to the input element? Then the UploadButtonElement will be leaked into my author shadow root.

> Source/WebCore/html/InputType.cpp:382
> +        while (root->hasChildNodes())

What about root->removeAllChildren() or root->removeChildren()

*I’m not sure which one of these you want. Probably removeChildren.*

[Shinya Kawanaka]

Comment on attachment 126420 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126420&action=review

>> Source/WebCore/html/FileInputType.cpp:270
>> +    element()->shadowRoot()->appendChild(element()->multiple() ? UploadButtonElement::createForMultiple(element()->document()): UploadButtonElement::create(element()->document()), ec);
> 
> Won’t this be problematic if I attach an author shadow root to the input element? Then the UploadButtonElement will be leaked into my author shadow root.

Since we don't support multiple shadow subtrees, and we have disabled to add an author shadow root into INPUT element, this shouldn't cause a problem.
I'm now writing a patch to support multiple shadow subtrees. Your anxiety will be fixed in that (coming) patch.

[Shinya Kawanaka]

Comment on attachment 126420 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126420&action=review

>> Source/WebCore/html/InputType.cpp:382
>> +        while (root->hasChildNodes())
> 
> What about root->removeAllChildren() or root->removeChildren()
> 
> *I’m not sure which one of these you want. Probably removeChildren.*

Ah, I didn't notice ContainerNode has removeChildren() method...

[Shinya Kawanaka]

Created attachment 126453 [details]
Patch

[WebKit Review Bot]

Comment on attachment 126453 [details]
Patch

Attachment 126453 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11447573

New failing tests:
fast/speech/speech-input-scripting.html

[Shinya Kawanaka]

(In reply to comment #9)
> (From update of attachment 126453 [details])
> Attachment 126453 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11447573
> 
> New failing tests:
> fast/speech/speech-input-scripting.html

Why is chromium bot complaining...?

[Shinya Kawanaka]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 126453 [details] [details])
> > Attachment 126453 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/11447573
> > 
> > New failing tests:
> > fast/speech/speech-input-scripting.html
> 
> Why is chromium bot complaining...?

Ah, removeChildren() is not correct, maybe.

[Shinya Kawanaka]

Created attachment 126468 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 126468 [details]
Patch

This is great! Nice and simple.

[WebKit Review Bot]

Comment on attachment 126468 [details]
Patch

Clearing flags on attachment: 126468

Committed r107524: <http://trac.webkit.org/changeset/107524>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

If this is fixed, should we remove the check here: http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/ShadowRoot.cpp&exact_package=chromium&q=allowsAuthorShadowRoot&type=cs&l=75