Bug 45181

Created attachment 66515 [details] Patch

Comment on attachment 66515 [details] Patch r=me

Comment on attachment 66515 [details] Patch Needs layout test.

Created attachment 66621 [details] Patch Added new layout test as suggested.

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?

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

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

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

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

Created attachment 66702 [details] Patch Rewrote test to include the success case as well and made the programmatic click test work outside DRT.

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.

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 on attachment 66706 [details] Patch r=me

Comment on attachment 66706 [details] Patch Clearing flags on attachment: 66706 Committed r66878: <http://trac.webkit.org/changeset/66878>

All reviewed patches have been landed. Closing bug.

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