Bug 50077

Summary: Fix rendering of speech button when setting the attribute dynamically.
Product: WebKit Reporter: Satish Sampath <satish>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: jorlow, tkent
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 39485    
Attachments:
Description Flags
Patch
none
Patch tkent: review+

Satish Sampath
Reported 2010-11-25 06:56:13 PST
Setting the 'x-webkit-speech' attribute on an input field dynamically (via setAttribute or other means) was causing the speech/mic button to show up at the wrong position. This patch fixes that by explicitly reinitializing the rendering components when the attribute is changed at runtime. I have added this case to one of the existing pixel tests. This will cause the tests to fail in chromium and I'll submit new baseline images once the change propagates to the chromium canary bots.
Attachments
Patch (4.60 KB, patch)
2010-11-25 06:58 PST, Satish Sampath 		no flags
DetailsFormatted DiffDiff
Patch (4.93 KB, patch)
2010-11-30 07:19 PST, Satish Sampath 		tkent: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Satish Sampath
Comment 1 2010-11-25 06:58:42 PST
Created attachment 74873 [details] Patch
Satish Sampath
Comment 2 2010-11-29 03:30:14 PST
Adding tkent@ as a reviewer as he took a look at this speech input UI layout code in the past.
Kent Tamura
Comment 3 2010-11-29 17:04:44 PST
Comment on attachment 74873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74873&action=review > WebCore/rendering/RenderTextControlSingleLine.cpp:676 > + setChildrenInline(true); Is this needed?
Satish Sampath
Comment 4 2010-11-30 00:24:43 PST
Comment on attachment 74873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74873&action=review >> WebCore/rendering/RenderTextControlSingleLine.cpp:676 >> + setChildrenInline(true); > > Is this needed? Yes it was needed because the m_childrenInline flag of this render element gets set to false in layouts without speech. In this method we are recreating the child elements and setting this flag to its default value (set to true in constructor of RenderBlock.cpp) gets us the accurate layout.
Kent Tamura
Comment 5 2010-11-30 00:35:35 PST
Comment on attachment 74873 [details] Patch ok. Does this code work well when x-webkit-speech attribute is removed?
Satish Sampath
Comment 6 2010-11-30 02:37:27 PST
(In reply to comment #5) > (From update of attachment 74873 [details]) > ok. Does this code work well when x-webkit-speech attribute is removed? Yes it works as expected, layout matches what was shown without this patch.
Kent Tamura
Comment 7 2010-11-30 06:52:36 PST
(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 74873 [details] [details]) > > ok. Does this code work well when x-webkit-speech attribute is removed? > > Yes it works as expected, layout matches what was shown without this patch. We should have a test for a case of removeAttribute('x-webkit-speech').
Kent Tamura
Comment 8 2010-11-30 06:53:54 PST
Comment on attachment 74873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74873&action=review > LayoutTests/fast/speech/input-appearance-speechbutton.html:37 > +var inputWithAttribute = document.createElement('input'); > +inputWithAttribute.setAttribute('x-webkit-speech', 'x-webkit-speech'); > +document.body.appendChild(inputWithAttribute); This doesn't test the new code. We should call setAttribute() after document.body.appendChild().
Satish Sampath
Comment 9 2010-11-30 07:19:52 PST
Created attachment 75145 [details] Patch
Satish Sampath
Comment 10 2010-11-30 07:21:12 PST
Uploaded new patch, with a fix to the setAttribute test case as suggested and an addition to test removeAttribute calls. As mentioned above this patch will cause the tests to fail in chromium and I'll submit new baseline images once the change propagates to the chromium canary bots.
Kent Tamura
Comment 11 2010-11-30 07:26:52 PST
Comment on attachment 75145 [details] Patch ok
Satish Sampath
Comment 12 2010-11-30 07:33:39 PST
Committed r72915: <http://trac.webkit.org/changeset/72915>
Note You need to log in before you can comment on or make changes to this bug.