Bug 40984 – Rendering the speech button in input elements.

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Satish Sampath]

Renders a generic speech input button on all platforms (enabled with the '@speech' attribute in <input> elements). Individual ports can customize the button if necessary in their RenderTheme implementation. Also added pixel tests.

All code changes are behind the speech input compile time flag (disabled by default).

More information about the speech input API proposal can be found in the following links:
 Parent bug: https://bugs.webkit.org/show_bug.cgi?id=39485
 Full API proposal: https://docs.google.com/View?id=dcfg79pz_5dhnp23f5
 Relevant discussions in the WHATWG list:
  - http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-May/026338.html
  - http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-June/026747.html

[Satish Sampath]

Created attachment 59648 [details]
Patch

[WebKit Review Bot]

Attachment 59648 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebsettings.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebsettings.h"
WebCore/platform/graphics/qt/ImageQt.cpp:67:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Satish Sampath]

Created attachment 59649 [details]
Patch

[WebKit Review Bot]

Attachment 59649 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3326686

[Jeremy Orlow]

Comment on attachment 59649 [details]
Patch

Drive by comments.

WebCore/rendering/RenderInputSpeech.cpp:34
 +  #include "RenderInputSpeech.h"
You only put this within guards when the file is generated only when enabled.  Since this file is being checked in, you know it exists and thus it can go right under the config.h part.

[Satish Sampath]

Created attachment 59716 [details]
Patch

[Satish Sampath]

Thanks Jeremy. Latest patch has your suggestion implemented.
The cr-linux build fails for all these patches because of a known issue in the cr-linux build bot (it does not run 'gclient runhooks' for GYP/GYPI changes so keeps using the earlier makefiles). I have verified again on my local machine that cr-linux builds fine.

[Satish Sampath]

Created attachment 59717 [details]
Address Jeremy's comments

[WebKit Review Bot]

Attachment 59717 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3267674

[Kent Tamura]

Comment on attachment 59717 [details]
Address Jeremy's comments

I don't think we need to introduce new files; RenderInputSpeech.{cpp,h}.

[Satish Sampath]

Future patches will be adding more speech input related rendering code hence I created a new file as it would help in better code organization, looking at how the media controls were implemented in RenderMediaControls.cpp. If you strongly feel about not creating new files I can change it.

[Kent Tamura]

(In reply to comment #11)
> Future patches will be adding more speech input related rendering code hence I created a new file as it would help in better code organization, looking at how the media controls were implemented in RenderMediaControls.cpp. If you strongly feel about not creating new files I can change it.

Ok, I understand. I don't have strong objection.

The code looks ok.
We should have layout tests for type=number + speech and type=search + speech.

[Satish Sampath]

Created attachment 59776 [details]
Add new tests per tkent and some fixes

[WebKit Review Bot]

Attachment 59776 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3307709

[Satish Sampath]

(adding Adele)

Kent, thanks for the additional layout tests suggestion. I found some issues in my patch due to that and fixed now (in RenderThemeMac.mm and RenderTextControlSingleLine.cpp). Please take another look at them in particular.

Adele - can you please let me know if the changes to RenderThemeMac.mm and switching from setDisplay(BLOCK) to setDisplay(INLINE_BLOCK) in RenderTextControlSingleLine.cpp look ok? (Background info: in this patch I am enabling render of a speech input button in <input> elements, the feature need to be explicitly enabled with an @speech attribute and all the code is behind a define).
- The setDisplay() change to INLINE_BLOCK is required because RenderTextControlSingleLine can now contain both innerBlock and other child elements (i.e. spin button in the 'type=number' case), so to render them both in the same line I'm using INLINE_BLOCK
- The RenderThemeMac.mm change is required because the search field's clear (x) button may not be the right-most child element if the speech button is enabled.
For both the above changes, all layout tests passed in my local machine.

[Kent Tamura]

Comment on attachment 59776 [details]
Add new tests per tkent and some fixes

LayoutTests/ChangeLog:15
 +          * platform/mac/fast/forms/input-appearance-numberandspeech.html: Added.
These tests should be put to fast/forms/, not platform/mac/fast/forms/.  We should check appearance on other platforms.

[Kent Tamura]

(In reply to comment #15)
> Adele - can you please let me know if the changes to RenderThemeMac.mm and switching from setDisplay(BLOCK) to setDisplay(INLINE_BLOCK) in RenderTextControlSingleLine.cpp look ok? (Background info: in this patch I am enabling render of a speech input button in <input> elements, the feature need to be explicitly enabled with an @speech attribute and all the code is behind a define).

I think these changes are reasonable.

[Satish Sampath]

Created attachment 60000 [details]
Moved layout tests and updated Skipped lists.

[Kent Tamura]

Comment on attachment 60000 [details]
Moved layout tests and updated Skipped lists.

LayoutTests/ChangeLog:24
 +          * platform/chromium/Skipped: Skipped all these new tests as the speech input feature is disabled by default.
chromium/Skpped -> chromium/test_expectations.txt

LayoutTests/platform/chromium/test_expectations.txt:2896
 +  BUG44844: fast/forms/input-appearance-numberandspeech.html = TEXT
I think "TEXT" should be "IMAGE+TEXT" or "FAIL"

[Satish Sampath]

Created attachment 60001 [details]
Updated changelog and test_expectations.txt

[Satish Sampath]

Created attachment 60004 [details]
Merged with latest revision

[WebKit Commit Bot]

Comment on attachment 60004 [details]
Merged with latest revision

Rejecting patch 60004 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
dertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19232 test cases.
fast/css/input-search-padding.html -> failed

Exiting early after 1 failures. 5961 tests run.
108.46s total testing time

5960 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3352216

[Satish Sampath]

Created attachment 60216 [details]
Changed RenderTextControlSingleLine to use INLINE_BLOCK only if a spin button is also present, so all existing layout test and the new ones pass now.

[WebKit Commit Bot]

Comment on attachment 60216 [details]
Changed RenderTextControlSingleLine to use INLINE_BLOCK only if a spin button is also present, so all existing layout test and the new ones pass now.

Clearing flags on attachment: 60216

Committed r62249: <http://trac.webkit.org/changeset/62249>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/62249 might have broken Chromium Linux Release