1806823 - [AR] Selecting Next/Previous Month Arrows in a Calendar will not allow the user to reach the Days/Weeks Selector using Tab in AR builds

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Rares Doghi, Desktop QA]

Attachment: 2022-12-21_15h20_10.mp4

Found in

• Beta 109.0b5

Affected versions

• Firefox Nightly 110.0a1 (2022-12-21)
• Beta 109.0b5

Affected platforms

• All

Steps to reproduce

1. Reach data:text/html,<input type='datetime-local'> in an AR build.
2. Open the Calendar
3. Use Tab in order to reach all controls including the day/weeks selector.
4. Reach the Next or Previous Month arrow and select it using Space.
5. Reach the Day selector again.

Expected result

• User should be able to reach the Day Selector after hitting one of the Next/Previous Arrows.

Actual result

• The Tab key will only cycle through the Next / Previous Month and the Month/Year selector.
• Unable to reach the Day selector after hitting the Next/Previous arrows

Regression range
Not a Regression.

Comment 1

[Daniel Holbert [:dholbert]]

I can repro using arabic-localized Nightly 110 from
https://ftp.mozilla.org/pub/firefox/nightly/2022/12/2022-12-20-21-46-32-mozilla-central-l10n/firefox-110.0a1.ar.linux-x86_64.tar.bz2

(I happen to be using Wayland, which means the popup actually fails to appear at all at the provided data-URI in arabic builds, due to bug 1805208. But if I add some margin, the popup does appear when I click the calendar icon, and I can repro this bug as-described.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Here's a testcase which avoids bug 1805208, for convenience.

I included input type='date' as well as 'datetime-local`; both widgets' calendar inputs are affected by this.

Interestingly: since I can repro in an AR-localized build, I thought I might be able to repro in a stock en-US nightly with RTL ui (activated by setting about:config pref intl.l10n.pseudo to bidi which forces us down this LocaleService::IsAppLocaleRTL() codepath and e.g. puts the calendar popup button on the left side of the datetime widget). But that configuration does not seem to trigger the issue; I get expected results under that config.

Comment 3

[Anna Yeddi [:ayeddi]]

Attachment: Bug 1806823 - Manage focus after Previous/Next month button is pressed. r?mconley,kcochrane,Jamie

Ensure that the focus stays on the Previous/Next Month button after it's pressed, but the calendar is updated and the same day of the month is made focusable within a calendar view. Also, ensure that this same day element is reacheable with keyboard alone on both RTL and LTR builds.

TODO: Add tests for RTL builds

Depends on D167309

Comment 4

[Anna Yeddi [:ayeddi]]

(In reply to Daniel Holbert [:dholbert] from comment #2)

Created attachment 9309421 [details]
testcase 1

Here's a testcase which avoids bug 1805208, for convenience.

I included input type='date' as well as 'datetime-local`; both widgets' calendar inputs are affected by this.

Interestingly: since I can repro in an AR-localized build, I thought I might be able to repro in a stock en-US nightly with RTL ui (activated by setting about:config pref intl.l10n.pseudo to bidi which forces us down this LocaleService::IsAppLocaleRTL() codepath and e.g. puts the calendar popup button on the left side of the datetime widget). But that configuration does not seem to trigger the issue; I get expected results under that config.

Thank you for the investigation! I am able to reproduce this discrepancy locally too.

I made a local Arabic build and tested the patch draft manually. It is working, but I will be adding only the intl.l10n.pseudo=bidi pref browser tests for now.

:dholbert - would that still be okay to submit? I am not sure I know (yet) how to run browser-chrome mochitests on a specific RTL build. Would you okay to be added as a reviewer?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/672458dd6851
Manage focus after Previous/Next month button is pressed. r=kcochrane

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Anna Yeddi [:ayeddi] from comment #4)

I made a local Arabic build and tested the patch draft manually. It is working,
but I will be adding only the intl.l10n.pseudo=bidi pref browser tests for now.

FWIW, I can't actually reproduce the bug with intl.l10n.pseudo=bidi -- per comment 2, that configuration is fine. I don't know enough about datepicker.js and locale-specific form control configurations to understand why. But it's worth noting that, if your automated test is only exercising that configuration, it might not actually be testing the real thing that was broken here.

:dholbert - would that still be okay to submit? I am not sure I know (yet) how to run browser-chrome mochitests on a specific RTL build. Would you okay to be added as a reviewer?

I'm not a good reviewer here (not having experience with datepicker.js ) but it looks like you ended up all set in terms of review. :)

RE running browser-chrome mochitests on a specific RTL build: I don't know if that's possible either, but I'd hope that it might be possible to find some combination of prefs that could produce the same issue that Arabic-localized builds were hitting here, which we could manaully set for a particular mochitest.

To do that, we would probably need to debug what's going on in the original STR for an Arabic build vs. for a intl.l10n.pseudo=bidi build and figure out where the codepath diverges in a way that "saves" the latter configuration. And then perhaps there's a flag that could be toggled to ensure we go down the (formerly) bad path, such that we can test that the bug is really fixed.

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/672458dd6851

Comment 8

[Rares Doghi, Desktop QA]

This issue is Verified as fixed in our latest Nightly build 111.0a1 (2023-01-22).

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:ayeddi, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox110 to wontfix.

For more information, please visit auto_nag documentation.

Comment 10

[Anna Yeddi [:ayeddi]]

Comment on attachment 9313136 [details]
Bug 1806823 - Manage focus after Previous/Next month button is pressed. r?mconley,kcochrane,Jamie

Beta/Release Uplift Approval Request

• User impact if declined: The keyboard accessibility allows users with screen readers and users with limited mobility access the functionality of this component - a date picker calendar for date and datetime-local inputs only. The patch ensures that even after the Previous/Next button is pressed by a user, the calendar grid itself still consistently provides a focusable day for a keyboard user to get to the calendar grid and be able to navigate between days to make a selection.
• 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: (included in the bug description)
• List of other uplifts needed: Bug 1803608, Bug 1806645
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The changes only impact keyboard accessibility for one component only (a date picker calendar for date and datetime-local inputs only), it does not change any looks and no other aspects of the feel of these components

• Low risk improvements early in the cycle (the keyboard accessibility was only added in 109)
• Enabling features that have been validated/tested by QA (the fix for this bug and for its parent in the stack, the bug 1806645, have been tested and confirmed by the QA team)

• String changes made/needed: none
• Is Android affected?: No

Comment 11

[Pascal Chevrel:pascalc]

Comment on attachment 9313136 [details]
Bug 1806823 - Manage focus after Previous/Next month button is pressed. r?mconley,kcochrane,Jamie

Approved for 110 beta 5, thanks.

Comment 12

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/187e5755b970

Comment 13

[Anna Yeddi [:ayeddi]]

(In reply to Daniel Holbert [:dholbert] from comment #6)

(In reply to Anna Yeddi [:ayeddi] from comment #4)

I made a local Arabic build and tested the patch draft manually. It is working,
but I will be adding only the intl.l10n.pseudo=bidi pref browser tests for now.

FWIW, I can't actually reproduce the bug with intl.l10n.pseudo=bidi -- per comment 2, that configuration is fine. I don't know enough about datepicker.js and locale-specific form control configurations to understand why. But it's worth noting that, if your automated test is only exercising that configuration, it might not actually be testing the real thing that was broken here.

:dholbert - would that still be okay to submit? I am not sure I know (yet) how to run browser-chrome mochitests on a specific RTL build. Would you okay to be added as a reviewer?

I'm not a good reviewer here (not having experience with datepicker.js ) but it looks like you ended up all set in terms of review. :)

RE running browser-chrome mochitests on a specific RTL build: I don't know if that's possible either, but I'd hope that it might be possible to find some combination of prefs that could produce the same issue that Arabic-localized builds were hitting here, which we could manaully set for a particular mochitest.

To do that, we would probably need to debug what's going on in the original STR for an Arabic build vs. for a intl.l10n.pseudo=bidi build and figure out where the codepath diverges in a way that "saves" the latter configuration. And then perhaps there's a flag that could be toggled to ensure we go down the (formerly) bad path, such that we can test that the bug is really fixed.

For future references, thanks to :zbraniecki from the L10n Dev room in Matrix:

intl.l10n.pseudo changes direction but doesn't change locale.
in result, if you keep locale en-US this line - https://searchfox.org/mozilla-central/source/toolkit/content/widgets/datepicker.js#89
will resolve to NumberFormat for en-US.
while if you test ar it will resolve to NumberFormat for ar.

Comment 14

[Rares Doghi, Desktop QA]

This issue is Verified as fixed in our latest Beta build 110.0b5.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9313136 [details]
Bug 1806823 - Manage focus after Previous/Next month button is pressed. r?mconley,kcochrane,Jamie

See comment 10.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9313136 [details]
Bug 1806823 - Manage focus after Previous/Next month button is pressed. r?mconley,kcochrane,Jamie

Fixes a regression caused by the datetime improvements shipped in 109. Approved for 109.0.1.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9313136 [details]
Bug 1806823 - Manage focus after Previous/Next month button is pressed. r?mconley,kcochrane,Jamie

Actually, looking more closely at this, there's a number of other dependencies which would also need to be uplifted if we wanted to fix this in 109. Given that, let's just let the fix ride 110. If you feel strongly otherwise, let me know!

Comment 18

[Anna Yeddi [:ayeddi]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)

Comment on attachment 9313136 [details]
Bug 1806823 - Manage focus after Previous/Next month button is pressed. r?mconley,kcochrane,Jamie

Actually, looking more closely at this, there's a number of other dependencies which would also need to be uplifted if we wanted to fix this in 109. Given that, let's just let the fix ride 110. If you feel strongly otherwise, let me know!

Thank you for considering this and for the follow up, Ryan!
While this and the bug 1803608 would further fix the keyboard navigation for a datepicker user, there were no keyboard navigation before 109 (there is nothing to be regressed per se) and there is a way to access previous/next months with arrow navigation too, so this does not block the use of a widget. Appreciate the thought though!

Comment 19

[Rares Doghi, Desktop QA]

Updating the Main status flags and removing the remaining flags.