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-

Description Eli Grey (:sephr) 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.
Comment 1 Leandro Graciá Gil 2011-08-31 16:17:25 PDT
Created attachment 105852 [details]
Patch
Comment 2 WebKit Review Bot 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
Comment 3 Satish Sampath 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
Comment 4 Leandro Graciá Gil 2011-09-01 03:44:35 PDT
Created attachment 105934 [details]
Patch
Comment 5 Leandro Graciá Gil 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
Comment 6 Ryosuke Niwa 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.
Comment 7 Eli Grey (:sephr) 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.
Comment 8 Leandro Graciá Gil 2011-09-05 03:31:09 PDT
Created attachment 106319 [details]
Patch
Comment 9 Leandro Graciá Gil 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.
Comment 10 WebKit Review Bot 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
Comment 11 Alexey Proskuryakov 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.
Comment 12 Leandro Graciá Gil 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?
Comment 13 Alexey Proskuryakov 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.
Comment 14 Leandro Graciá Gil 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.
Comment 15 Leandro Graciá Gil 2011-09-06 04:20:05 PDT
Created attachment 106404 [details]
Patch
Comment 16 Leandro Graciá Gil 2011-09-12 04:03:27 PDT
Created attachment 107037 [details]
Patch
Comment 17 Leandro Graciá Gil 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.
Comment 18 Alexey Proskuryakov 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.
Comment 19 Leandro Graciá Gil 2011-09-12 09:32:47 PDT
Created attachment 107054 [details]
Patch
Comment 20 Leandro Graciá Gil 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.
Comment 21 Leandro Graciá Gil 2011-09-20 15:38:45 PDT
Is there anybody willing to do the (potentially) final review?
Thanks.
Comment 22 Ryosuke Niwa 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?
Comment 23 Leandro Graciá Gil 2011-09-20 17:11:31 PDT
Created attachment 108083 [details]
Patch
Comment 24 Leandro Graciá Gil 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.
Comment 25 WebKit Review Bot 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
Comment 26 Ryosuke Niwa 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?
Comment 27 Leandro Graciá Gil 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?
Comment 28 Leandro Graciá Gil 2011-09-21 07:24:12 PDT
Created attachment 108154 [details]
Patch
Comment 29 Leandro Graciá Gil 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.
Comment 30 Ryosuke Niwa 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.
Comment 31 Leandro Graciá Gil 2011-09-22 00:30:40 PDT
Created attachment 108289 [details]
Patch
Comment 32 Leandro Graciá Gil 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.
Comment 33 Leandro Graciá Gil 2011-09-22 01:47:21 PDT
Created attachment 108294 [details]
Updating the startspeech test in a more appropriate way.
Comment 34 Ryosuke Niwa 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.
Comment 35 Leandro Graciá Gil 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.
Comment 36 Leandro Graciá Gil 2011-09-22 09:17:57 PDT
Committed r95722: <http://trac.webkit.org/changeset/95722>