Closed Bug 1132918 Opened 7 years ago Closed 6 years ago

Unrecognized words on direct voice input will display two error notifications

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox38 ?, firefox39 affected, firefox42 fixed, fennec+)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox38 --- ?
firefox39 --- affected
firefox42 --- fixed
fennec + ---

People

(Reporter: cos_flaviu, Assigned: karim)

References

Details

Attachments

(3 files)

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).
> Make some unrecognizable sounds

Nominate this for Bugzilla best-of
tracking-fennec: --- → ?
Assignee: nobody → jhugman
tracking-fennec: ? → 38+
We're holding this in Nightly, so AIUI 38 is unaffected. Tracking 39+ instead of 38+.
tracking-fennec: 38+ → 39+
tracking-fennec: 39+ → +
Assignee: jhugman → nobody
Flaviu - Can you retest (I don't have the device) to see if any of our other fixes affected this bug?
Flags: needinfo?(flaviu.cos)
(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.
Flags: needinfo?(flaviu.cos)
Assignee: nobody → kbenhmida
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Summary: Unrecognised words on direct voice input will display two error notifications → Unrecognized words on direct voice input will display two error notifications
Bug 1132918 - Unrecognized words on direct voice input will display two error notifications. r=liuche
Attachment #8636359 - Flags: review?(liuche)
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 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.)
Attachment #8636359 - Flags: review?(liuche) → review+
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.
Flags: needinfo?(alam)
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!
Flags: needinfo?(alam)
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?
Flags: needinfo?(liuche)
(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.
Keywords: checkin-needed
Flags: needinfo?(liuche)
https://hg.mozilla.org/mozilla-central/rev/180b015c939c
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.