Bug 60451 – speech input doesn't fire textInput events

• Component: UI Events
• Status: RESOLVED FIXED

[Eli Grey (:sephr)]

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.

[Leandro Graciá Gil]

Created attachment 105852 [details]
Patch

[WebKit Review Bot]

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]

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]

Created attachment 105934 [details]
Patch

[Leandro Graciá Gil]

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

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

[Leandro Graciá Gil]

Created attachment 106319 [details]
Patch

[Leandro Graciá Gil]

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

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

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]

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

Created attachment 106404 [details]
Patch

[Leandro Graciá Gil]

Created attachment 107037 [details]
Patch

[Leandro Graciá Gil]

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

Created attachment 107054 [details]
Patch

[Leandro Graciá Gil]

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]

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

[Ryosuke Niwa]

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]

Created attachment 108083 [details]
Patch

[Leandro Graciá Gil]

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

Created attachment 108154 [details]
Patch

[Leandro Graciá Gil]

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

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

Created attachment 108289 [details]
Patch

[Leandro Graciá Gil]

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

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

[Ryosuke Niwa]

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

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