Closed Bug 988297 Opened 8 years ago Closed 8 years ago

Language is changed if up/down arrow keys are used while translation infobar displays the error message

Categories

(Firefox :: Translation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: bogdan_maris, Assigned: florian)

Details

(Whiteboard: [translation] p=2 s=it-32c-31a-30b.2 [qa!])

Attachments

(2 files, 2 obsolete files)

Reproducible on try build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/florian@queze.net-d33b91e475d0/ (BuildID: 20140325093017)

Steps to reproduce:
1. Start Firefox.
2. Load https://www.mozilla.org/fr/contribute/.
3. Click 'Translate' on the Translate infobar.
- If 'There has been an error translating this page' message appears hit 'Try Again'
4. In the first dropdown field/or the second, select languages until the error message will appear.
5. When error message appears press up/down arrow keys

Expected results: I expect nothing to happen.

Actual results: The language from step 4 moves up or down depending on what key I press.

Notes:
1. This is reproducible only in Windows and Linux.
2. In the screencast attached I pressed three times down arrow key so it jumped three languages.
Flags: firefox-backlog+
Assignee: nobody → florian
Status: NEW → ASSIGNED
Whiteboard: [translation] → [translation] p=2 s=it-32c-31a-30b.1 [qa?]
Attached patch WIP (obsolete) — Splinter Review
This detects that the focus is in a part of the infobar that is about to get hidden, and advances the focus into the new visible state of the infobar.

The problem with this patch is that there's no focusable element in the "Translating..." state, and each transition goes through that state, so we end up always setting the focus to the "Options" drop down, which isn't very useful.

I'm not sure what a good behavior would be here.
Whiteboard: [translation] p=2 s=it-32c-31a-30b.1 [qa?] → [translation] p=2 s=it-32c-31a-30b.1 [qa+]
Mass move of translation bugs to the new Translation component.
Component: General → Translation
Version: 31 Branch → unspecified
Attached patch Different WIP (obsolete) — Splinter Review
Trying a different approach. I'm now trying to not mess with the focus, and instead just prevent the elements from getting keyboard events if they are not in the currently selected panel of the deck.

The attached patch fixes the bug here (but unfortunately not bug 988336 as the oncommand event is used there, rather than just keyboard events), but causes another problem: global keyboard shortcuts (eg. Command+K to focus the search box) are also blocked, which isn't what I wanted. I would like to only prevent the event from propagating to the subtree of the deck.

Neil, do you have any suggestion?
Attachment #8420159 - Flags: feedback?(enndeakin)
Comment on attachment 8420159 [details] [diff] [review]
Different WIP

The first patch is more correct.

+          let focusedElt = document.commandDispatcher.focusedElement;

But you should just use document.activeElement to get the focused element here.
Attachment #8420159 - Flags: feedback?(enndeakin) → feedback-
(In reply to Neil Deakin from comment #4)

Thanks for the quick feedback! :-)

> The first patch is more correct.

The behavior of the first patch doesn't seem correct to me (it moves the focus to the "Options" drop down), and I don't see anything obviously better to do with the same approach. Should we add a dummy focusable element inside the 'translating' state that currently doesn't have any focusable element? Or should we remember in a boolean that the focus was moved automatically to the Options button, so that once we are no longer in the 'Translating' state, we can move it back to something inside the deck?
Flags: needinfo?(enndeakin)
(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> The behavior of the first patch doesn't seem correct to me (it moves the
> focus to the "Options" drop down), and I don't see anything obviously better
> to do with the same approach.

You should either do that or just call blur() on the element.

 Should we add a dummy focusable element inside
> the 'translating' state that currently doesn't have any focusable element?
> Or should we remember in a boolean that the focus was moved automatically to
> the Options button, so that once we are no longer in the 'Translating'
> state, we can move it back to something inside the deck?

I'm guessing here that the 'Translating' text appears only temporarily. You could, if the focus is inside the hidden area, blur the element then restore the focus to the same element.
Flags: needinfo?(enndeakin)
Whiteboard: [translation] p=2 s=it-32c-31a-30b.1 [qa+] → [translation] p=2 s=it-32c-31a-30b.2 [qa+]
Attached patch Patch v3Splinter Review
Patch using activeElement (as suggested in comment 4) and blur (as suggested in comment 6).
Attachment #8415397 - Attachment is obsolete: true
Attachment #8420159 - Attachment is obsolete: true
Attachment #8426369 - Flags: review?(felipc)
Comment on attachment 8426369 [details] [diff] [review]
Patch v3

Review of attachment 8426369 [details] [diff] [review]:
-----------------------------------------------------------------

Is bug 988336 the same prob?
Attachment #8426369 - Flags: review?(felipc) → review+
(In reply to :Felipe Gomes from comment #8)

> Is bug 988336 the same prob?

Yes, the patch fixes both.
https://hg.mozilla.org/mozilla-central/rev/7cf22ab8d6ac
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
QA Contact: bogdan.maris
Verified as fixed using revision 7cf22ab8d6ac on Windows 8.1 64bit, Mac OS X 10.9.3 and Ubuntu 14.04 32bit.
Status: RESOLVED → VERIFIED
Whiteboard: [translation] p=2 s=it-32c-31a-30b.2 [qa+] → [translation] p=2 s=it-32c-31a-30b.2 [qa!]
You need to log in before you can comment on or make changes to this bug.