1132918 - Unrecognized words on direct voice input will display two error notifications

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Flaviu Cos]

Attachment: two error notification popups.png

Environment: 
Device: Asus Transformer Tab (Android 4.2.1);
Build: Nightly 38.0a1 (2015-02-13);

Steps to reproduce:
1. Launch fennec;
2. Tap on the url bar to enter edit mode;
3. Tap on the microphone icon from the url bar;
4. Make some unrecognizable sounds;
5. Tap on 'Cancel' button from the displayed error.

Expected result:
The popup is dismissed and about:home is displayed.

Actual result:
Nightly Voice Search error pops up saying about the same thing.

Notes:
Please check the attached screenshot.

Also reproducible on Samsung Galaxy S4 (Android 4.3).
Not reproducible on Google Nexus 4 (Android 4.4.4) and Google Nexus 7 (Android 5.0.2).

Comment 1

[Aaron Train [:aaronmt]]

> Make some unrecognizable sounds

Nominate this for Bugzilla best-of

Comment 2

[Richard Newman [:rnewman]]

We're holding this in Nightly, so AIUI 38 is unaffected. Tracking 39+ instead of 38+.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Flaviu - Can you retest (I don't have the device) to see if any of our other fixes affected this bug?

Comment 4

[Flaviu Cos]

Attachment: Try again notification.png

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Flaviu - Can you retest (I don't have the device) to see if any of our other
> fixes affected this bug?

The bug is no longer reproducible.
Attached is a screenshot of how the notification looks like.

Comment 5

[Karim Benhmida [:karim]]

I found out that this bug is reproducible on a Nexus 5 (Android 4). I fixed it.
Running some tests on different devices before uploading a patch.

Comment 6

[Karim Benhmida [:karim]]

Attachment: MozReview Request: Bug 1132918 - Unrecognized words on direct voice input will display two error notifications. r=liuche

Bug 1132918 - Unrecognized words on direct voice input will display two error notifications. r=liuche

Comment 7

[Karim Benhmida [:karim]]

https://reviewboard.mozilla.org/r/13677/#review12271

::: mobile/android/base/toolbar/ToolbarEditLayout.java
(Diff revision 1)
> -                switch (resultCode) {

I removed the switch because we don't handle any error. Maybe I should add a comment with the different possible values resultCode can take in case we want to do that in the future?

::: mobile/android/base/toolbar/ToolbarEditLayout.java
(Diff revision 1)
> -    private void handleVoiceSearchError(boolean offerRetry) {

This method is not needed. The voice intent handles error out of the box. This was causing the second notification on some devices.

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 8636359 [details]
MozReview Request: Bug 1132918 - Unrecognized words on direct voice input will display two error notifications. r=liuche

https://reviewboard.mozilla.org/r/13677/#review12445

I tried it with my phone, and I think this is fine. I would expect the voice recognizer to handle its own error, which it looks like it does. There is a possibility of a situation where the voice service errors, but doesn't handle it (which is why this code is here).

Anthony, what do you think? I'm okay erring on the side of not showing duplicate UI, at the risk of not having an error message if the voice handler itself doesn't handle garbled text, etc, because the user probably assumes it's the voice recognizer's fault.

::: mobile/android/base/toolbar/ToolbarEditLayout.java
(Diff revision 1)
> -                switch (resultCode) {

We can leave off the errors - someone can look at hg blame, or just look up the error codes. No need document them here. (Also, consider that these return values can change, if the API changes, so we want to force anyone making changes to consult the canonical source, not a comment that we're saving at this moment.)

Comment 9

[Chenxia Liu [:liuche]]

Antlam, if you have a moment, I'd like to hear your thoughts on what tradeoff we should make, wrt showing our own (possibly redundant) error, vs letting the voice recognizer handle it.

Comment 10

[Karim Benhmida [:karim]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d1218cf5745

Comment 11

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I agree, we shouldn't have duplicate UI here. Especially since Voice input is something that's most often used for convenience. Two-error messages that require tapping/pressing to dismiss is very inconvenient. 

Thanks Chenxia!

Comment 12

[Karim Benhmida [:karim]]

Can I ship this?
I think it's better to let the voice recognizer handle it, it also means that we are future-proof in case Google decides to change the error handler in future versions. Plus I noticed that the code under `case RecognizerIntent.RESULT_NO_MATCH` was never called when the error happened, instead the built-in error modal was shown. It was only called when clicking on the cancel button of the built-in error modal (The cancel button only appears on a few devices though)

(In reply to Chenxia Liu [:liuche] from comment #8)
> There is a possibility of a situation where the voice service errors,
> but doesn't handle it (which is why this code is here).

Do you have an example in mind?

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Karim Benhmida (:karim) from comment #12)
> Can I ship this?

Yes. Please land this code when ready (or use checkin-needed) and also land the patch to remove the NIGHTLY flag.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/180b015c939c

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/180b015c939c