Bug 45181 – Ignore programmatic clicks on speech input button.

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

[Satish Sampath]

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.

[Satish Sampath]

Created attachment 66515 [details]
Patch

[Jeremy Orlow]

Comment on attachment 66515 [details]
Patch

r=me

[Jeremy Orlow]

Comment on attachment 66515 [details]
Patch

Needs layout test.

[Satish Sampath]

Created attachment 66621 [details]
Patch

Added new layout test as suggested.

[Jeremy Orlow]

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?

[Satish Sampath]

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

[Jeremy Orlow]

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  :-)

[Satish Sampath]

> > 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.

[Satish Sampath]

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.

[Jeremy Orlow]

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).

[Satish Sampath]

(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?

[Satish Sampath]

Created attachment 66702 [details]
Patch

Rewrote test to include the success case as well and made the programmatic click test work outside DRT.

[Jeremy Orlow]

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.

[Satish Sampath]

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.

[Jeremy Orlow]

Comment on attachment 66706 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 66706 [details]
Patch

Clearing flags on attachment: 66706

Committed r66878: <http://trac.webkit.org/changeset/66878>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66878 might have broken Leopard Intel Debug (Tests)