Bug 60451

Summary: speech input doesn't fire textInput events
Product: WebKit Reporter: Eli Grey (:sephr) <bugmail>
Component: UI EventsAssignee: Leandro Graciá Gil <leandrogracia>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cshu, darin, dglazkov, enrica, leandrogracia, morrita, ojan, rniwa, sam, satish, tkent, tonikitoo, tonyg, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.w3.org/TR/DOM-Level-3-Events/#events-DOM_INPUT_METHOD_VOICE
Bug Depends on:    
Bug Blocks: 39485    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Updating the startspeech test in a more appropriate way. rniwa: review+, rniwa: commit-queue-

Eli Grey (:sephr)
Reported 2011-05-08 12:32:01 PDT
textInput events are not dispatched text fields when text is input using textInput. When fixed, the event object's inputMethod property should also equal the DOM_INPUT_METHOD_VOICE input method code. The whole point of textInput is to be able to handle text input from any input device, not limited to keyboards.
Attachments
Patch (9.45 KB, patch)
2011-08-31 16:17 PDT, Leandro Graciá Gil
no flags
Patch (9.46 KB, patch)
2011-09-01 03:44 PDT, Leandro Graciá Gil
no flags
Patch (5.90 KB, patch)
2011-09-05 03:31 PDT, Leandro Graciá Gil
no flags
Patch (6.48 KB, patch)
2011-09-06 04:20 PDT, Leandro Graciá Gil
no flags
Patch (6.07 KB, patch)
2011-09-12 04:03 PDT, Leandro Graciá Gil
no flags
Patch (6.16 KB, patch)
2011-09-12 09:32 PDT, Leandro Graciá Gil
no flags
Patch (6.17 KB, patch)
2011-09-20 17:11 PDT, Leandro Graciá Gil
no flags
Patch (6.47 KB, patch)
2011-09-21 07:24 PDT, Leandro Graciá Gil
no flags
Patch (7.01 KB, patch)
2011-09-22 00:30 PDT, Leandro Graciá Gil
no flags
Updating the startspeech test in a more appropriate way. (7.14 KB, patch)
2011-09-22 01:47 PDT, Leandro Graciá Gil
rniwa: review+
rniwa: commit-queue-
Leandro Graciá Gil
Comment 1 2011-08-31 16:17:25 PDT
WebKit Review Bot
Comment 2 2011-08-31 17:42:17 PDT
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
Satish Sampath
Comment 3 2011-09-01 01:46:25 PDT
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
Leandro Graciá Gil
Comment 4 2011-09-01 03:44:35 PDT
Leandro Graciá Gil
Comment 5 2011-09-01 03:45:59 PDT
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
Ryosuke Niwa
Comment 6 2011-09-01 11:01:55 PDT
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.
Eli Grey (:sephr)
Comment 7 2011-09-01 12:41:12 PDT
I have created bug 67426 for splitting up the speech triggering textInput and TextEvent.inputMethod portions of your patch.
Leandro Graciá Gil
Comment 8 2011-09-05 03:31:09 PDT
Leandro Graciá Gil
Comment 9 2011-09-05 03:33:14 PDT
(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.
WebKit Review Bot
Comment 10 2011-09-05 04:07:53 PDT
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
Alexey Proskuryakov
Comment 11 2011-09-05 12:32:44 PDT
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.
Leandro Graciá Gil
Comment 12 2011-09-05 12:41:13 PDT
(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?
Alexey Proskuryakov
Comment 13 2011-09-05 13:04:02 PDT
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.
Leandro Graciá Gil
Comment 14 2011-09-06 03:35:47 PDT
(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.
Leandro Graciá Gil
Comment 15 2011-09-06 04:20:05 PDT
Leandro Graciá Gil
Comment 16 2011-09-12 04:03:27 PDT
Leandro Graciá Gil
Comment 17 2011-09-12 04:04:18 PDT
(In reply to comment #16) > Created an attachment (id=107037) [details] > Patch Moving the selection of the input element to a safer place.
Alexey Proskuryakov
Comment 18 2011-09-12 08:54:04 PDT
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.
Leandro Graciá Gil
Comment 19 2011-09-12 09:32:47 PDT
Leandro Graciá Gil
Comment 20 2011-09-12 09:34:11 PDT
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.
Leandro Graciá Gil
Comment 21 2011-09-20 15:38:45 PDT
Is there anybody willing to do the (potentially) final review? Thanks.
Ryosuke Niwa
Comment 22 2011-09-20 16:14:18 PDT
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?
Leandro Graciá Gil
Comment 23 2011-09-20 17:11:31 PDT
Leandro Graciá Gil
Comment 24 2011-09-20 17:12:24 PDT
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.
WebKit Review Bot
Comment 25 2011-09-20 19:36:24 PDT
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
Ryosuke Niwa
Comment 26 2011-09-20 19:41:20 PDT
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?
Leandro Graciá Gil
Comment 27 2011-09-21 01:27:53 PDT
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?
Leandro Graciá Gil
Comment 28 2011-09-21 07:24:12 PDT
Leandro Graciá Gil
Comment 29 2011-09-21 07:25:28 PDT
(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.
Ryosuke Niwa
Comment 30 2011-09-21 18:36:31 PDT
(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.
Leandro Graciá Gil
Comment 31 2011-09-22 00:30:40 PDT
Leandro Graciá Gil
Comment 32 2011-09-22 00:32:03 PDT
(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.
Leandro Graciá Gil
Comment 33 2011-09-22 01:47:21 PDT
Created attachment 108294 [details] Updating the startspeech test in a more appropriate way.
Ryosuke Niwa
Comment 34 2011-09-22 09:05:41 PDT
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.
Leandro Graciá Gil
Comment 35 2011-09-22 09:14:48 PDT
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.
Leandro Graciá Gil
Comment 36 2011-09-22 09:17:57 PDT
Note You need to log in before you can comment on or make changes to this bug.