Bug 45181

Summary: Ignore programmatic clicks on speech input button.
Product: WebKit Reporter: Satish Sampath <satish>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, eric, jorlow, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 39485    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Satish Sampath 2010-09-03 10:12:49 PDT
For security reasons, we want to start/stop speech recognition only based on direct user input and disallow the web page scripts sending click events.
Comment 1 Satish Sampath 2010-09-03 10:15:37 PDT
Created attachment 66515 [details]
Patch
Comment 2 Jeremy Orlow 2010-09-03 10:37:34 PDT
Comment on attachment 66515 [details]
Patch

r=me
Comment 3 Jeremy Orlow 2010-09-03 10:38:13 PDT
Comment on attachment 66515 [details]
Patch

Needs layout test.
Comment 4 Satish Sampath 2010-09-06 04:18:59 PDT
Created attachment 66621 [details]
Patch

Added new layout test as suggested.
Comment 5 Jeremy Orlow 2010-09-06 04:37:50 PDT
Comment on attachment 66621 [details]
Patch

Apparently using script-tests for stuff that involves WebCore is no longer in vogue.  Please remove TEMPLATE and merge the .js file into the generated html file, delete any boilerplate you might not need, and just check that in.  Add the element to the html file directly rather than dynamically

In the js file, use 4 space tabs.

The part that tests programatic clicks don't work is the most important thing here.  Definitely make sure it works even when a layout test controller isn't present.  Maybe even split into two tests?
Comment 6 Satish Sampath 2010-09-06 06:19:10 PDT
Created attachment 66633 [details]
Patch

Removed script-tests js file and template. Also removed test code to test success case, since that is already tested in input-text-speechbutton.html
Comment 7 Jeremy Orlow 2010-09-06 06:32:36 PDT
Comment on attachment 66633 [details]
Patch

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

> LayoutTests/fast/speech/speech-button-ignore-generated-events-expected.txt:9
> +TEST COMPLETE
Add newline at end.

> LayoutTests/fast/speech/speech-button-ignore-generated-events.html:20
> +    var input = document.createElement('input');
put this in the page itself

> LayoutTests/fast/speech/speech-button-ignore-generated-events.html:32
> +    }, 500);
this is flaky!!!!

is there some number of setTimeouts that makes this deterministic?   or something like that?

> LayoutTests/fast/speech/speech-button-ignore-generated-events.html:41
> +    event.initMouseEvent('click', true, true, window, 1, pageX, pageY, clientX, clientY,
is there any way to know for sure that this fired on the correct element?



sorry i'm being so nit picky, but this is a very important test  :-)
Comment 8 Satish Sampath 2010-09-06 06:52:18 PDT
> > LayoutTests/fast/speech/speech-button-ignore-generated-events.html:32
> > +    }, 500);
> this is flaky!!!!
> 
> is there some number of setTimeouts that makes this deterministic?   or something like that?

I can't think of any way, since we are checking for an event NOT being fired. I see such timeouts exist in other tests (such as http://trac.webkit.org/browser/trunk/LayoutTests/fast/forms/script-tests/textarea-metrics.js?rev=66825#L46). I can increase it to 1 second if you think that may make it more deterministic.

> > LayoutTests/fast/speech/speech-button-ignore-generated-events.html:41
> > +    event.initMouseEvent('click', true, true, window, 1, pageX, pageY, clientX, clientY,
> is there any way to know for sure that this fired on the correct element?

Unfortunately no, this depends on where and how the speech button is rendered and that is not exposed to javascript. These UI related tests need to be updated when the speech button rendering code changes.

If these are ok, I can upload a new patch with the other comments fixed.
Comment 9 Satish Sampath 2010-09-06 07:20:23 PDT
Created attachment 66639 [details]
Patch

Addressed all suggestions, added big explanatory comment in the test with pointers to what to look for if it fails.
Comment 10 Jeremy Orlow 2010-09-07 02:31:49 PDT
Comment on attachment 66639 [details]
Patch

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

> LayoutTests/fast/speech/speech-button-ignore-generated-events.html:13
> +// In this test, we send a generated click event via the dispatchEvent interface which is
Put this text in the body of the page.

> LayoutTests/fast/speech/speech-button-ignore-generated-events.html:17
> +// ** NOTE: If this test fails, please check the following:
Your target audience is people changing the test or re-baselining it--not those looking into why it's failing.  The point should be that if the implementation changes a lot, the calculated click point may no longer be the button.  And this should explain to people how to manually test it (while being fairly generic since code will move around over time).
Comment 11 Satish Sampath 2010-09-07 02:46:02 PDT
(In reply to comment #10)
> (From update of attachment 66639 [details])

I just realised that if the click coordinates no longer point to the speech button, this test would actually pass because it is only checking for the non-occurance of the onchange event. To make sure the coordinates are correct I need to add back the original code to test the success case (which was present in patch 2) so that an eventSender based click at that coordinate succeeds and returns the mock result, while a dispatchEvent based click at the same coordinate does not given an onchange event.

As for the comment, I could just mention in the description that "This test may time out if the calculated coordinates for the click event were incorrect and the speech button is not rendered at that point (i.e. test needs to be updated).". I say time out because the success case mentioned above will time out if the click did not happen on the button and onchange was never called while the script is waiting for it.

Does this sound ok?
Comment 12 Satish Sampath 2010-09-07 03:51:00 PDT
Created attachment 66702 [details]
Patch

Rewrote test to include the success case as well and made the programmatic click test work outside DRT.
Comment 13 Jeremy Orlow 2010-09-07 04:28:59 PDT
Comment on attachment 66702 [details]
Patch

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

> LayoutTests/fast/speech/speech-button-ignore-generated-events.html:59
> +            setTimeout(setupDispatchEventTest, 50);
There must be a way to not use a timeout.  Flaky tests are a MAJOR problem in Chrome and WebKit.  We should avoid adding more at all costs.
Comment 14 Satish Sampath 2010-09-07 04:39:02 PDT
Created attachment 66706 [details]
Patch

Removed the timeout value as it was immaterial, we just needed to return from onchange and get a callback once pending stuff got done. This style of setTimeout(..., 0) is quite prevalent within LayoutTests for such use cases.
Comment 15 Jeremy Orlow 2010-09-07 05:03:01 PDT
Comment on attachment 66706 [details]
Patch

r=me
Comment 16 WebKit Commit Bot 2010-09-07 05:47:44 PDT
Comment on attachment 66706 [details]
Patch

Clearing flags on attachment: 66706

Committed r66878: <http://trac.webkit.org/changeset/66878>
Comment 17 WebKit Commit Bot 2010-09-07 05:47:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 WebKit Review Bot 2010-09-07 08:00:48 PDT
http://trac.webkit.org/changeset/66878 might have broken Leopard Intel Debug (Tests)