Toolbar keyboard navigation is in reverse for RTL locales

VERIFIED FIXED in Firefox 67

Status

()

defect
P1
normal
VERIFIED FIXED
5 months ago
5 months ago

People

(Reporter: itiel_yn8, Assigned: Jamie)

Tracking

(Blocks 1 bug, {rtl})

unspecified
Firefox 68
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox67 fixed, firefox68 verified)

Details

Attachments

(2 attachments)

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

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

Assignee: nobody → jteh

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

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

Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ef1a8840cb08
Toolbar keyboard navigation: Make arrow keys behave as expected for RTL locales. r=Gijs

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

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jteh)
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68

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

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

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

Status: RESOLVED → VERIFIED
Flags: needinfo?(jteh)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7992d7459707
part 2: Add RTL test for toolbar keyboard navigation. r=Gijs

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
Attachment #9053487 - Flags: approval-mozilla-beta?

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

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

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

QA Whiteboard: [qa-triaged]

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.

Attachment #9053487 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.