Created attachment 105852 [details] Patch

Comment on attachment 105852 [details] Patch Attachment 105852 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9536016 New failing tests: fast/speech/input-onspeechchange-event.html fast/speech/speech-input-scripting.html fast/speech/speech-button-ignore-generated-events.html fast/speech/input-text-speechbutton.html fast/speech/input-text-language-tag.html

For reference, the TextEvent draft spec which this patch seems to implement is at http://www.w3.org/TR/DOM-Level-3-Events/#events-TextEvent

Created attachment 105934 [details] Patch

Yes, that's the one pointed in the URL by the reporter. (In reply to comment #3) > For reference, the TextEvent draft spec which this patch seems to implement is at http://www.w3.org/TR/DOM-Level-3-Events/#events-TextEvent

Comment on attachment 105934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105934&action=review > Source/WebCore/dom/TextEvent.idl:44 > + readonly attribute unsigned long inputMethod; Adding this property to TextEvent should be done in a separate patch, or else you should describe the change in your change log and point to the appropriate spec. r- because of this. Also, if you're adding this property, then you'll need to test every one of those values you're adding. Furthermore, this is a new feature, so you should be announcing it on webkit-dev.

I have created bug 67426 for splitting up the speech triggering textInput and TextEvent.inputMethod portions of your patch.

Created attachment 106319 [details] Patch

(In reply to comment #6) > (From update of attachment 105934 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105934&action=review > > > Source/WebCore/dom/TextEvent.idl:44 > > + readonly attribute unsigned long inputMethod; > > Adding this property to TextEvent should be done in a separate patch, or else you should describe the change in your change log and point to the appropriate spec. r- because of this. > Also, if you're adding this property, then you'll need to test every one of those values you're adding. Furthermore, this is a new feature, so you should be announcing it on webkit-dev. Following the discussion of bug 67426, I'm removing the inputMethod part of the patch and firing the event for consistency with the existing code. I've also removed the bug dependency.

Comment on attachment 106319 [details] Patch Attachment 106319 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9590619 New failing tests: fast/speech/input-text-speechstart.html

Comment on attachment 106319 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106319&action=review The concept here is right - textInput events should definitely be fired for any input. r- for failing test, and for my comments that likely require code changes. > Source/WebCore/dom/TextEventInputType.h:38 > + TextEventInputVoice, Why add a new unused enum for voice? It's not exposed to clients, and all it can do is confuse developers (as it's not set for regular voice input, but only for Chromium's custom speech input feature). I suggest adding an "other" value. Or perhaps you should use one of the existing values, like TextEventInputKeyboard, so that callers that check for that work correctly? TextEventInputType.h code smells very wrong, so you should be extra careful with it. Look at TextEventInputLineBreak - its comment says "any tab characters in the text are backtabs". Other options may have no relation to their names, too. > Source/WebCore/html/shadow/TextControlInnerElements.cpp:495 > + // Sending the text event first to avoid incoherent situations where the speech change event is used to select an alternative result. > + ASSERT(document()->domWindow()); > + input->dispatchEvent(TextEvent::create(document()->domWindow(), results.isEmpty() ? "" : results[0]->utterance(), TextEventInputVoice)); > input->dispatchEvent(SpeechInputEvent::create(eventNames().webkitspeechchangeEvent, results)); These events look quite irregular. Why is there a webkitspeechchange event, in the first place? What happens if preventDefault() is called by textInput event handler? You just go ahead and dispatch webkitspeechchange, which seems wrong.

(In reply to comment #11) > (From update of attachment 106319 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106319&action=review > > The concept here is right - textInput events should definitely be fired for any input. r- for failing test, and for my comments that likely require code changes. I'm digging into the failing test and the more I learn the more I think implementing this event is a bad idea. The test is failing because TextEvents seem to not dispatch if the action has not ben initiated by the user. In this test, it is initiated by the LayoutTestController and that leads to problems that doesn't seem simple to fix. > > > Source/WebCore/dom/TextEventInputType.h:38 > > + TextEventInputVoice, > > Why add a new unused enum for voice? It's not exposed to clients, and all it can do is confuse developers (as it's not set for regular voice input, but only for Chromium's custom speech input feature). > > I suggest adding an "other" value. Or perhaps you should use one of the existing values, like TextEventInputKeyboard, so that callers that check for that work correctly? > > TextEventInputType.h code smells very wrong, so you should be extra careful with it. Look at TextEventInputLineBreak - its comment says "any tab characters in the text are backtabs". Other options may have no relation to their names, too. > > > Source/WebCore/html/shadow/TextControlInnerElements.cpp:495 > > + // Sending the text event first to avoid incoherent situations where the speech change event is used to select an alternative result. > > + ASSERT(document()->domWindow()); > > + input->dispatchEvent(TextEvent::create(document()->domWindow(), results.isEmpty() ? "" : results[0]->utterance(), TextEventInputVoice)); > > input->dispatchEvent(SpeechInputEvent::create(eventNames().webkitspeechchangeEvent, results)); > > These events look quite irregular. Why is there a webkitspeechchange event, in the first place? Speech Input it's following this spec proposal: https://docs.google.com/View?id=dcfg79pz_5dhnp23f5 > > What happens if preventDefault() is called by textInput event handler? You just go ahead and dispatch webkitspeechchange, which seems wrong. This looks like a clash between the Speech Input spec and the TextInput events. Considering the discussion about possibly removing them altogether and the problems that have to be solved to possibly replace them I'm really wondering if this event should be fired at all. It might be good for consistency with the existing code, but in any case we're already not planning to complete the implementation of TextEvent. Also, the Speech Input spec this is based is likely to be replaced soon by a new proposal. Maybe it's just better to drop it and mark it as won't fix. What do you think?

What I think is that WebKit should have nothing to do with speech input - it's between OS and input methods to support any kind of input interface a user needs, be it speech recognition, OCR, barcode reading, or something else. Barring that, it seems that any such in-engine implementation should pretend to be IME as much as possible, meaning that there should be no webkitspeechchange event, and textInput should be dispatched exactly like it is for regular IME input. Otherwise, the differences between Chromium and regular voice input would cause trouble for JS authors and for users. I agree with Eli that the whole point of textInput is to be able to handle text input from any input device, not limited to keyboards.

(In reply to comment #13) > What I think is that WebKit should have nothing to do with speech input - it's between OS and input methods to support any kind of input interface a user needs, be it speech recognition, OCR, barcode reading, or something else. > > Barring that, it seems that any such in-engine implementation should pretend to be IME as much as possible, meaning that there should be no webkitspeechchange event, and textInput should be dispatched exactly like it is for regular IME input. Otherwise, the differences between Chromium and regular voice input would cause trouble for JS authors and for users. > > I agree with Eli that the whole point of textInput is to be able to handle text input from any input device, not limited to keyboards. I guess this kind of event is something that needs to be discussed in the incoming Speech Input API proposal. It is possible that it might disappear altogether. For now it's main difference with the TextInput events is that it provides alternative recognition results with their confidences. I've found a fix for the failing test that is far simpler than I expected. I'll take your advice and use a TextEventInputOther value (or the Keyboard one if you think is more appropriate). About calling preventDefault, after looking into it again it should not affect this onwebkitspeechchange as the latter provides recognition alternatives, but does not set them. The actual setting is done by the TextInput event.

Created attachment 106404 [details] Patch

Created attachment 107037 [details] Patch

(In reply to comment #16) > Created an attachment (id=107037) [details] > Patch Moving the selection of the input element to a safer place.

Comment on attachment 107037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107037&action=review > What happens if preventDefault() is called by textInput event handler? You just go ahead and dispatch webkitspeechchange, which seems wrong. Actually, that's not necessarily wrong - preventDefault() doesn't seem to prevent input for IME either. I'm not in love with Speech Input API in general, but this patch looks like a small improvement. I'll leave final review for someone who is more interested in this feature. > Source/WebCore/html/shadow/TextControlInnerElements.cpp:494 > + // Required for setting the results on TextEvents when recognition is started programatically. Typo: should be "programmatically". > Source/WebCore/html/shadow/TextControlInnerElements.cpp:498 > + // Sending the text event first to avoid incoherent situations where the speech change event is used to select an alternative result. This needs more explanation, and is possibly wrong. The text in TextEvent should be the text that's being inserted, so it seems that it should be dispatched after any mutations, not before. That said, a page that performs mutation in webkitspeechchange listener may not need textInput to make decisions.

Created attachment 107054 [details] Patch

Comment on attachment 107037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107037&action=review >> Source/WebCore/html/shadow/TextControlInnerElements.cpp:494 >> + // Required for setting the results on TextEvents when recognition is started programatically. > > Typo: should be "programmatically". Done. >> Source/WebCore/html/shadow/TextControlInnerElements.cpp:498 >> + // Sending the text event first to avoid incoherent situations where the speech change event is used to select an alternative result. > > This needs more explanation, and is possibly wrong. The text in TextEvent should be the text that's being inserted, so it seems that it should be dispatched after any mutations, not before. > > That said, a page that performs mutation in webkitspeechchange listener may not need textInput to make decisions. Done.

Is there anybody willing to do the (potentially) final review? Thanks.

Comment on attachment 107054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107054&action=review > Source/WebCore/html/shadow/TextControlInnerElements.cpp:498 > + ASSERT(document()->domWindow()); Why can we assume that document()->domWindow() is not null? Can't onfocus event handler remove this element from the document?

Created attachment 108083 [details] Patch

Comment on attachment 107054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107054&action=review >> Source/WebCore/html/shadow/TextControlInnerElements.cpp:498 >> + ASSERT(document()->domWindow()); > > Why can we assume that document()->domWindow() is not null? Can't onfocus event handler remove this element from the document? Fixed.

Comment on attachment 108083 [details] Patch Attachment 108083 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9763545 New failing tests: svg/custom/svg-fonts-word-spacing.html

Comment on attachment 108083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108083&action=review > LayoutTests/fast/speech/input-ontextinput-event.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Please use HTML5 style DOCTYPE: <!DOCTYPE html> > Source/WebCore/ChangeLog:7 > + Please explain what kind of changes you're making. > Source/WebCore/html/shadow/TextControlInnerElements.cpp:496 > + // Required for setting the results on TextEvents when recognition is started programmatically. > + input->focus(); > + input->select(); Is there any specification that mandates/allows this behavior?

Comment on attachment 108083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108083&action=review >> LayoutTests/fast/speech/input-ontextinput-event.html:1 >> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > Please use HTML5 style DOCTYPE: <!DOCTYPE html> It will be fixed with the next patch. >> Source/WebCore/ChangeLog:7 >> + > > Please explain what kind of changes you're making. I'll add some more information in the next patch. >> Source/WebCore/html/shadow/TextControlInnerElements.cpp:496 >> + input->select(); > > Is there any specification that mandates/allows this behavior? No, this comes from the fact that someone else added the possibility of starting speech input programmatically from the layout test controller. In that only case the input object won't have the focus or be selected and consequently the text events will fail to add the text to the control. Otherwise this wouldn't be required at all. Any suggestions?

Created attachment 108154 [details] Patch

(In reply to comment #28) > Created an attachment (id=108154) [details] > Patch Failures of test svg/custom/svg-fonts-word-spacing.html seem to be completely unrelated to this patch.

(In reply to comment #27) > >> Source/WebCore/html/shadow/TextControlInnerElements.cpp:496 > >> + input->select(); > > > > Is there any specification that mandates/allows this behavior? > > No, this comes from the fact that someone else added the possibility of starting speech input programmatically from the layout test controller. In that only case the input object won't have the focus or be selected and consequently the text events will fail to add the text to the control. Otherwise this wouldn't be required at all. Any suggestions? Then we should fix those tests that rely on such a behavior. We certainly do not want to expose it to the Web if it's only required for the tests to work.

Created attachment 108289 [details] Patch

(In reply to comment #30) > (In reply to comment #27) > > >> Source/WebCore/html/shadow/TextControlInnerElements.cpp:496 > > >> + input->select(); > > > > > > Is there any specification that mandates/allows this behavior? > > > > No, this comes from the fact that someone else added the possibility of starting speech input programmatically from the layout test controller. In that only case the input object won't have the focus or be selected and consequently the text events will fail to add the text to the control. Otherwise this wouldn't be required at all. Any suggestions? > > Then we should fix those tests that rely on such a behavior. We certainly do not want to expose it to the Web if it's only required for the tests to work. Done. It should be fixed now.

Created attachment 108294 [details] Updating the startspeech test in a more appropriate way.

Comment on attachment 108294 [details] Updating the startspeech test in a more appropriate way. View in context: https://bugs.webkit.org/attachment.cgi?id=108294&action=review > LayoutTests/ChangeLog:6 > + Need a short description and bug URL (OOPS!) Please get rid of this line (and one blank line around it). > Source/WebCore/ChangeLog:10 > + Reviewed by NOBODY (OOPS!). This line needs to appear before the description.

Comment on attachment 108294 [details] Updating the startspeech test in a more appropriate way. View in context: https://bugs.webkit.org/attachment.cgi?id=108294&action=review >> LayoutTests/ChangeLog:6 >> + Need a short description and bug URL (OOPS!) > > Please get rid of this line (and one blank line around it). Fixing it for landing. >> Source/WebCore/ChangeLog:10 >> + Reviewed by NOBODY (OOPS!). > > This line needs to appear before the description. Fixing it for landing.

Committed r95722: <http://trac.webkit.org/changeset/95722>