Bug 77930 - INPUT shouldn't create ShadowRoot dynamically.
: INPUT shouldn't create ShadowRoot dynamically.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 77503 77608
  Show dependency treegraph
 
Reported: 2012-02-06 20:31 PST by Shinya Kawanaka
Modified: 2012-05-30 18:55 PDT (History)
7 users (show)

See Also:


Attachments
W.I.P. (15.40 KB, patch)
2012-02-07 23:27 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
W.I.P. (15.40 KB, patch)
2012-02-07 23:37 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (18.58 KB, patch)
2012-02-09 18:12 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (18.52 KB, patch)
2012-02-09 22:18 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (18.54 KB, patch)
2012-02-10 00:05 PST, Shinya Kawanaka
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-06 20:31:43 PST
Currently INPUT recreates shadow root if its type is changed.
We can reuse shadow root to stop recreating it.
Comment 1 Shinya Kawanaka 2012-02-07 23:27:08 PST
Created attachment 126017 [details]
W.I.P.
Comment 2 Shinya Kawanaka 2012-02-07 23:37:34 PST
Created attachment 126018 [details]
W.I.P.
Comment 3 WebKit Review Bot 2012-02-08 01:26:40 PST
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
Comment 4 Shinya Kawanaka 2012-02-09 18:12:32 PST
Created attachment 126420 [details]
Patch
Comment 5 Dominic Cooney 2012-02-09 21:05:52 PST
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.*
Comment 6 Shinya Kawanaka 2012-02-09 21:53:44 PST
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.
Comment 7 Shinya Kawanaka 2012-02-09 21:56:05 PST
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...
Comment 8 Shinya Kawanaka 2012-02-09 22:18:21 PST
Created attachment 126453 [details]
Patch
Comment 9 WebKit Review Bot 2012-02-09 22:49:53 PST
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
Comment 10 Shinya Kawanaka 2012-02-09 23:15:10 PST
(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...?
Comment 11 Shinya Kawanaka 2012-02-10 00:01:19 PST
(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.
Comment 12 Shinya Kawanaka 2012-02-10 00:05:56 PST
Created attachment 126468 [details]
Patch
Comment 13 Dimitri Glazkov (Google) 2012-02-10 09:19:08 PST
Comment on attachment 126468 [details]
Patch

This is great! Nice and simple.
Comment 14 WebKit Review Bot 2012-02-12 19:53:22 PST
Comment on attachment 126468 [details]
Patch

Clearing flags on attachment: 126468

Committed r107524: <http://trac.webkit.org/changeset/107524>
Comment 15 WebKit Review Bot 2012-02-12 19:53:28 PST
All reviewed patches have been landed.  Closing bug.