Closed
Bug 1132918
Opened 10 years ago
Closed 9 years ago
Unrecognized words on direct voice input will display two error notifications
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(firefox38 ?, firefox39 affected, firefox42 fixed, fennec+)
RESOLVED
FIXED
Firefox 42
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).
Comment 1•10 years ago
|
||
> Make some unrecognizable sounds
Nominate this for Bugzilla best-of
Updated•10 years ago
|
tracking-fennec: --- → ?
status-firefox38:
--- → affected
Updated•10 years ago
|
Assignee: nobody → jhugman
tracking-fennec: ? → 38+
Comment 2•10 years ago
|
||
We're holding this in Nightly, so AIUI 38 is unaffected. Tracking 39+ instead of 38+.
Updated•10 years ago
|
tracking-fennec: 39+ → +
Updated•9 years ago
|
Assignee: jhugman → nobody
Comment 3•9 years ago
|
||
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•9 years ago
|
||
(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•9 years ago
|
Assignee: nobody → kbenhmida
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
Assignee | ||
Comment 5•9 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•9 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•9 years ago
|
||
Bug 1132918 - Unrecognized words on direct voice input will display two error notifications. r=liuche
Attachment #8636359 -
Flags: review?(liuche)
Assignee | ||
Comment 7•9 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 8•9 years ago
|
||
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+
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
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•9 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•9 years ago
|
Flags: needinfo?(liuche)
Comment 13•9 years ago
|
||
(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•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
Flags: needinfo?(liuche)
Comment 14•9 years ago
|
||
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•