1538575 - Toolbar keyboard navigation is in reverse for RTL locales

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Itiel]

STR:

1. Latest Nightly, intl.uidirection = 1
2. Hit Ctrl+L
3. Shift+Tab x2 -- the Back button should be highlighted (or the Reload button, if you're on a new tab)
4. Hit the left arrow key to go other buttons -- nothing happens
5. Hit the right arrow key -- navigation goes to the left
6. Now hit the left arrow key again and the navigation will go to the right

Comment 1

[James Teh [:Jamie]]

I wasn't sure about this. Just to clarify (I'm blind and can't see the visual layout), for RTL locales, is the entire order of the toolbar (not just the text of each button) RTL? That is, from right to left, you get Back, Forward, Home, Reload, URL bar, Library, etc.?

Comment 2

[Itiel]

(In reply to James Teh [:Jamie] from comment #1)

I wasn't sure about this. Just to clarify (I'm blind and can't see the visual layout), for RTL locales, is the entire order of the toolbar (not just the text of each button) RTL?

Correct.

That is, from right to left, you get Back, Forward, Home, Reload, URL bar, Library, etc.?

Yes.

Comment 3

[James Teh [:Jamie]]

Gijs, fixing this is really trivial (checking window.RTL_UI and behaving accordingly). However, I'm not sure how to test it. It seems that changing intl.uidirection doesn't update window.RTL_UI in existing windows. Is there some established pattern for this kind of testing I can refer to? (Once again, writing the tests is just so unbelievably painful compared to writing the code.)

Comment 4

[James Teh [:Jamie]]

Attachment: Bug 1538575: Toolbar keyboard navigation: Make arrow keys behave as expected for RTL locales.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef1a8840cb08
Toolbar keyboard navigation: Make arrow keys behave as expected for RTL locales. r=Gijs

Comment 6

[:Gijs (he/him)]

(In reply to James Teh [:Jamie] from comment #3)

Gijs, fixing this is really trivial (checking window.RTL_UI and behaving accordingly). However, I'm not sure how to test it. It seems that changing intl.uidirection doesn't update window.RTL_UI in existing windows. Is there some established pattern for this kind of testing I can refer to? (Once again, writing the tests is just so unbelievably painful compared to writing the code.)

Sorry, I missed this needinfo before reviewing and triggering landing on the patch. I think we can add a test in a follow-up.

FTR, I think it's a bug that window.RTL_UI doesn't update (which we should probably file orthogonally from whatever else we end up doing here). This is because, AIUI, we're moving towards a world where users can switch locales at runtime (which will start being possible once we finish removing DTDs and their user-unfriendly failure mode (yellow screen of death)), so anything that doesn't update for this at runtime will end up being a bug.

In the short term for having a test here, you can open a new window from the test using BrowserTestUtils.openNewBrowserWindow(), after setting the pref, as window.RTL_UI is per-window and will be fetched using Services.locale.appLocaleIsRTL or whatever it is the first time someone asks for the value. If we set the pref before opening the window, that's guaranteed to provide the right result. Is that enough info for you to write the test or were there other things you were getting stuck on?

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ef1a8840cb08

Comment 8

[:Gijs (he/him)]

I'm guessing we want to uplift this, unless we intend to turn off the keyboard navigation stuff for 67 again?

Comment 9

[James Teh [:Jamie]]

Attachment: Bug 1538575 part 2: Add RTL test for toolbar keyboard navigation.

Comment 10

[James Teh [:Jamie]]

Verified fixed in Firefox 68.0a1 build 20190326214944 Windows 64 bit.

I'll submit an uplift request once the tests land.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7992d7459707
part 2: Add RTL test for toolbar keyboard navigation. r=Gijs

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/7992d7459707

Comment 13

[James Teh [:Jamie]]

Comment on attachment 9053487 [details]
Bug 1538575: Toolbar keyboard navigation: Make arrow keys behave as expected for RTL locales.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1436086 .
• User impact if declined: Toolbar keyboard navigation (new to Firefox 67) won't work as expected for users of right-to-left locales.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Extremely simple patch covered by automated tests.
• String changes made/needed:
• Do you want to request approval of these patches as well?: on

Comment 14

[Pascal Chevrel:pascalc (PTO until April 26)]

Looks safe, but let's get it verified by QA on Nightly before uplifting since we have STR in comment #0

Comment 15

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/projects/ash/rev/ef1a8840cb087d7bc7fbe82f80cd2944cb82d0d0
Bug 1538575: Toolbar keyboard navigation: Make arrow keys behave as expected for RTL locales. r=Gijs

Comment 16

[Itiel]

(In reply to Pascal Chevrel:pascalc from comment #14)

Looks safe, but let's get it verified by QA on Nightly before uplifting since we have STR in comment #0

fwiw I can confirm this is fixed on Nightly now.

Comment 17

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9053487 [details]
Bug 1538575: Toolbar keyboard navigation: Make arrow keys behave as expected for RTL locales.

P1, fix broken tab navigation with RTL locales, tab navigation in the toolbar is a 67 a11y feature. Low risk patch verified by QA, uplift approved for 67 beta 8, thanks.

Comment 18

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/21ab0c178e23

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2255b45c876b