Unrecognized words on direct voice input will display two error notifications

RESOLVED FIXED in Firefox 42

Status

()

Firefox for Android
Awesomescreen
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Flaviu Cos, Assigned: karim)

Tracking

Trunk
Firefox 42
ARM
Android
Points:
---

Firefox Tracking Flags

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

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
Created attachment 8564150 [details]
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).
> Make some unrecognizable sounds

Nominate this for Bugzilla best-of

Updated

3 years ago
tracking-fennec: --- → ?
status-firefox38: --- → affected
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+
status-firefox38: affected → ?
status-firefox39: --- → affected
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)
(Reporter)

Comment 4

3 years ago
Created attachment 8628127 [details]
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.
Flags: needinfo?(flaviu.cos)
(Assignee)

Updated

3 years ago
Assignee: nobody → kbenhmida
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WORKSFORME
(Assignee)

Comment 5

3 years ago
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 → ---
(Assignee)

Updated

3 years ago
Summary: Unrecognised words on direct voice input will display two error notifications → Unrecognized words on direct voice input will display two error notifications
(Assignee)

Comment 6

3 years ago
Created attachment 8636359 [details]
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
Attachment #8636359 - Flags: review?(liuche)
(Assignee)

Comment 7

3 years ago
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)
(Assignee)

Comment 12

3 years ago
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?
(Assignee)

Updated

3 years ago
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.
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Flags: needinfo?(liuche)
https://hg.mozilla.org/mozilla-central/rev/180b015c939c
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.