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

Status: VERIFIED FIXED

(Firefox :: Translations, defect)

Description

[Bogdan Maris, Desktop QA]

Attachment: Screencast showing the issue

Reproducible on try build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-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.

Comment 1

[Florian Quèze [:florian]]

Attachment: WIP

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.

Comment 2

[Florian Quèze [:florian]]

Mass move of translation bugs to the new Translation component.

Comment 3

[Florian Quèze [:florian]]

Attachment: Different WIP

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?

Comment 4

[Neil Deakin]

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.

Comment 5

[Florian Quèze [:florian]]

(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?

Comment 6

[Neil Deakin]

(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.

Comment 7

[Florian Quèze [:florian]]

Attachment: Patch v3

Patch using activeElement (as suggested in comment 4) and blur (as suggested in comment 6).

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8426369 [details] [diff] [review]
Patch v3

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

Is bug 988336 the same prob?

Comment 9

[Florian Quèze [:florian]]

(In reply to :Felipe Gomes from comment #8)

> Is bug 988336 the same prob?

Yes, the patch fixes both.

Comment 10

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/7cf22ab8d6ac

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/7cf22ab8d6ac

Comment 12

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed using revision 7cf22ab8d6ac on Windows 8.1 64bit, Mac OS X 10.9.3 and Ubuntu 14.04 32bit.