Bug 77930

Summary: INPUT shouldn't create ShadowRoot dynamically.
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dominicc, hayato, morrita, shinyak, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 77503, 77608    
Attachments:
Description Flags
W.I.P.
none
W.I.P.
none
Patch
none
Patch
none
Patch none

Shinya Kawanaka
Reported 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.
Attachments
W.I.P. (15.40 KB, patch)
2012-02-07 23:27 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
W.I.P. (15.40 KB, patch)
2012-02-07 23:37 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (18.58 KB, patch)
2012-02-09 18:12 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (18.52 KB, patch)
2012-02-09 22:18 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (18.54 KB, patch)
2012-02-10 00:05 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Shinya Kawanaka
Comment 1 2012-02-07 23:27:08 PST
Created attachment 126017 [details] W.I.P.
Shinya Kawanaka
Comment 2 2012-02-07 23:37:34 PST
Created attachment 126018 [details] W.I.P.
WebKit Review Bot
Comment 3 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
Shinya Kawanaka
Comment 4 2012-02-09 18:12:32 PST
Created attachment 126420 [details] Patch
Dominic Cooney
Comment 5 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.*
Shinya Kawanaka
Comment 6 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.
Shinya Kawanaka
Comment 7 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...
Shinya Kawanaka
Comment 8 2012-02-09 22:18:21 PST
Created attachment 126453 [details] Patch
WebKit Review Bot
Comment 9 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
Shinya Kawanaka
Comment 10 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...?
Shinya Kawanaka
Comment 11 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.
Shinya Kawanaka
Comment 12 2012-02-10 00:05:56 PST
Created attachment 126468 [details] Patch
Dimitri Glazkov (Google)
Comment 13 2012-02-10 09:19:08 PST
Comment on attachment 126468 [details] Patch This is great! Nice and simple.
WebKit Review Bot
Comment 14 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>
WebKit Review Bot
Comment 15 2012-02-12 19:53:28 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.