fix: correctly format date/time in RTL #7423

yihuiliao · 2024-11-21T19:15:24Z

Close #4711

Need to double check that nothing broke in #6562 with these changes.

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Arabic should render in the following order:

TimeZone DayPeriod Hour:Minute Year/Month/Day

Hebrew should render in the following order:

TimeZone DayPeriod Hour:Minute Day/Month/Year

Tab order should be:

Day -> Month -> Year -> Hour -> Minute -> DayPeriod -> TimeZone

Arabic Placeholders:

يوم / شهر / سنة

Hebrew Placeholders:

שנה.חודש.יום

Also be sure to test keyboard navigation, deleting/backspace, resizing

Note that with backspacing, the focus should move the previous node. So basically the tab order above but go from right to left

🧢 Your Project:

rspbot · 2024-11-21T19:25:42Z

Build successful! 🎉

sadeghbarati · 2024-12-02T06:41:49Z

Hey, Thanks for working on this PR

Is this also going to fix keyboard focus management on RTL too?

For example DateField component in the storybook link, if you pick Farsi locale, in order to focus DateSegments with arrow key you need to do it opposite direction to focus next date segment

DateField with Farsi locale:
- yyyy/mm/dd (روز/ماه/سال) format is right ✅
- focus management with arrow keys is not right ❌
DatePicker and DateRangePicker with Farsi locale:
- dd/mm/yyyy (سال/ماه/روز) format is not right ❌
- focus management with arrow keys is right ✅

yihuiliao · 2024-12-02T16:05:28Z

@sadeghbarati Yes, the plan is to also correct the keyboard navigation for RTL as well. The PR is still in the early stages so there's a lot that has yet to be implemented.

rspbot · 2024-12-04T19:00:48Z

Build successful! 🎉

rspbot · 2024-12-06T16:34:29Z

Build successful! 🎉

rspbot · 2024-12-20T21:46:31Z

Build successful! 🎉

rspbot · 2025-01-08T23:57:34Z

Build successful! 🎉

rspbot · 2025-01-17T23:01:31Z

Build successful! 🎉

packages/@react-aria/datepicker/src/useDatePickerGroup.ts

packages/@react-spectrum/datepicker/test/DateField.test.js

packages/@react-spectrum/datepicker/test/DatePicker.test.js

snowystinger · 2025-01-19T22:42:25Z

packages/@react-spectrum/datepicker/test/DatePickerBase.test.js

+          let el = this;
+
+          if (el.getAttribute('role') === 'spinbutton') {
+            if (el.parentElement.getAttribute('data-testid') === 'end-date') {


i thought the issue with some of these was that the dom order was not in alignment with the rendered order?
this test is a good base, but should we include some tests where the order differs? I think you could use el.getAttribute('data-type') to order based on the day/month/hour/etc

ah yes, it's more obvious with when using examples with time segments so i should update the examples to include those

snowystinger · 2025-01-19T22:46:32Z

packages/react-aria-components/stories/DatePicker.stories.tsx

@@ -92,11 +92,11 @@ export const DateRangePickerExample = () => (
    <Label style={{display: 'block'}}>Date</Label>
    <Group style={{display: 'inline-flex'}}>
      <div className={styles.field}>
-        <DateInput data-testid="date-range-picker-date-input" slot="start" style={{display: 'inline-flex'}}>
+        <DateInput data-testid="date-range-picker-date-input" slot="start" style={{display: 'inline'}}>


sort of a breaking change
what will happen to people who have used this as an example to build their DatePicker's in the past?

Also, why is the style change different from DateField.mdx?

so i experimented a bit and found that for the rtl segments to display in the correct order, DateInput needs to have display inline, inline-block, or block. hence why one is display inline and the other is display block. it would probably be a good idea to make it consistent and stick with display inline though.

and as for the sort of breaking change, i think folks can continue to use display: inline-flex. i don't believe the changes here will affect that. the only thing that they'll run into is that if they do choose to render their datepicker in a rtl locale, it will be in the incorrect order.

updating the docs and stories here is to reflect the changes needed for rtl locales to display in the right order. that way, those who do end up copying the docs + styles afterwards will have the proper styling for it to work correctly.

unfortunately, i don't think there's a way to have rtl locales render their date/time segments correctly using display flex, so it might require updates on the users side.

ok, so long as inline-flex doesn't break anything, I'm fine with this
we should add a little blurb to the Accessibility section for the docs

yeah, that's a good idea to call it out in our docs somewhere. i think maybe the Internationalization section instead?

rspbot · 2025-01-22T02:12:47Z

Build successful! 🎉

rspbot · 2025-01-22T18:35:06Z

Build successful! 🎉

snowystinger · 2025-01-23T02:39:50Z

packages/@react-aria/datepicker/src/useDatePickerGroup.ts

+      let update = () => {
+        if (ref.current) {
+          // TODO: For now, just querying this list of elements. However, it's possible that either through hooks or RAC that some users may include other focusable items that they would want to able to keyboard navigate to. In that case, we might want to utilize focusableElements in isFocusable.ts
+          let editableSegments: NodeListOf<Element> | undefined = ref.current?.querySelectorAll('span[role="spinbutton"], span[role="textbox"], button');


because this checks for span in all the selectors, will this break people who are using divs like we used to?

oh good point. hmm in that case, we could update it so that it also checks for divs as well?

although, my guess is that people aren't using this in rtl locales given that the time/date formats were so off.

yihuiliao · 2025-01-24T01:13:40Z

packages/@react-spectrum/datepicker/test/DateField.test.js


      // Not needed in aria-described by because each segment has a label already, so this would be duplicative.
      let group = getByRole('group');
      expect(group).not.toHaveAttribute('aria-describedby');

-      expect(getByText('month / day / year')).toBeVisible();


i removed this line since now each individual segment is in its own span and it would be hard to query those. we have a chromatic story to test this so it is covered. i kept the rest of the test since it would be good to continue checking the aria stuff.

(and the same goes for DatePicker.test and DateRangePicker.test)

rspbot · 2025-01-24T01:21:18Z

Build successful! 🎉

snowystinger

Noticed that "Delete" likely needs the same logical movement treatment that the arrow keys got. Otherwise, in Hebrew I start deleting from the year -> month -> day and then it stops, it never moves to the time field element

I'm ok for this to be in follow up, otherwise looks good

yihuiliao · 2025-01-24T18:27:33Z

Noticed that "Delete" likely needs the same logical movement treatment that the arrow keys got. Otherwise, in Hebrew I start deleting from the year -> month -> day and then it stops, it never moves to the time field element

I think this is actually the correct behavior. "Delete" causes the focus to shift to the previous node. If you're deleting from year -> month -> day and then try deleting again, the focus never moves because there is no node before day.

It's the same in LTR. In en-US, if you start deleting year -> day -> month, you'll also never move to the timefield. It just feels like you should in RTL because we're reading the entire field as LTR because that's what we're used to.

devongovett · 2025-01-25T01:28:34Z

packages/@react-aria/datepicker/src/useDatePickerGroup.ts

+
+      update();
+
+      let observer = new MutationObserver(update);


Do we need the MutationObserver? When would the segments change order without the date picker itself re-rendering?

hmm good point,,,i think it should be fine if we were to get rid of it

it was for if users added more buttons to control the field, Yihui had an example with a << and < button for jumping month and year I think

I don't know if those buttons would ever be removed or added dynamically

Ah right, I think it was from the example in this issue #7645

devongovett · 2025-01-28T23:32:06Z

packages/@react-stately/datepicker/src/useDateFieldState.ts

+      isEditable
+    } as DateSegment;
+
+    if (segment.type === 'hour') {


We should add some comments to this function describing how it works and what the unicode characters do. Will be hard for future readers/contributors to understand what's going on if they aren't familiar with it. 😄

yihuiliao added 2 commits November 21, 2024 14:09

bdo on timefield, reverse segments on timefield in datefield

bbe6191

fix lint

0716ad5

adobe deleted a comment from rspbot Nov 21, 2024

make things inline

76b8d0e

use unicode character to wrap segments

47e2af5

yihuiliao added 3 commits December 4, 2024 14:11

fix test

ef0b637

append unicode to text in hooks, update rac

b3e2f70

Merge branch 'main' into bidi-override

7b77f1e

yihuiliao added 6 commits December 17, 2024 12:53

add comment

fd5a902

Merge branch 'main' into bidi-override

d9b69a8

skip failing test for now

a9f732e

update keyboard nav

8df5ad6

update logic of how unicode is applied

679e355

fix spacing

bedf713

adobe deleted a comment from rspbot Jan 7, 2025

yihuiliao added 5 commits January 7, 2025 11:13

add comments

c2df442

update tests

9080188

undo some previous changes

2b5fb13

wrap time segments in lri, wrap fields in unicode isolate

91173c3

fix ssr test

79a8da3

adobe deleted a comment from rspbot Jan 9, 2025

yihuiliao added 2 commits January 9, 2025 17:57

fix spacing

b923904

fix css logic

ae03f3b

yihuiliao changed the title ~~wip: fix incorrect date/time order in rtl~~ fix incorrect date/time order in rtl Jan 17, 2025

yihuiliao changed the title ~~fix incorrect date/time order in rtl~~ fix: ensure date/time segments are ordered correctly in RTL Jan 17, 2025

yihuiliao marked this pull request as ready for review January 17, 2025 23:18

snowystinger reviewed Jan 18, 2025

View reviewed changes

packages/@react-aria/datepicker/src/useDatePickerGroup.ts Outdated Show resolved Hide resolved

packages/@react-aria/datepicker/src/useDatePickerGroup.ts Outdated Show resolved Hide resolved

snowystinger reviewed Jan 19, 2025

View reviewed changes

yihuiliao added 4 commits January 21, 2025 10:13

remove style tests

83d2727

fix lint

9e74c07

update to uselayouteffect and update keyboard nav test

e48425b

make date input more consistent with using display inline

8e10996

update timefield docs css to use display inline

13d47a9

snowystinger reviewed Jan 23, 2025

View reviewed changes

yihuiliao added 5 commits January 23, 2025 16:17

fix showFormatHelpText

4d1adff

small change

9acdf22

fix lint

c02ec95

add divs to keyboard navigation so it works with older versions

e564b9c

fix lint + fix tests

77a6bb9

yihuiliao commented Jan 24, 2025

View reviewed changes

yihuiliao changed the title ~~fix: ensure date/time segments are ordered correctly in RTL~~ fix: correctly format date/time in RTL Jan 24, 2025

adobe deleted a comment from rspbot Jan 24, 2025

snowystinger approved these changes Jan 24, 2025

View reviewed changes

devongovett approved these changes Jan 28, 2025

View reviewed changes

yihuiliao added this pull request to the merge queue Jan 29, 2025

Merged via the queue into main with commit e228ed8 Jan 29, 2025
31 checks passed

yihuiliao deleted the bidi-override branch January 29, 2025 01:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: correctly format date/time in RTL #7423

fix: correctly format date/time in RTL #7423

yihuiliao commented Nov 21, 2024 •

edited

Loading

rspbot commented Nov 21, 2024

sadeghbarati commented Dec 2, 2024 •

edited

Loading

yihuiliao commented Dec 2, 2024

rspbot commented Dec 4, 2024

rspbot commented Dec 6, 2024

rspbot commented Dec 20, 2024

rspbot commented Jan 8, 2025

rspbot commented Jan 17, 2025

snowystinger Jan 19, 2025

yihuiliao Jan 21, 2025

snowystinger Jan 19, 2025

yihuiliao Jan 21, 2025 •

edited

Loading

snowystinger Jan 23, 2025

yihuiliao Jan 24, 2025

rspbot commented Jan 22, 2025

rspbot commented Jan 22, 2025

snowystinger Jan 23, 2025

yihuiliao Jan 24, 2025

yihuiliao Jan 24, 2025 •

edited

Loading

rspbot commented Jan 24, 2025

snowystinger left a comment

yihuiliao commented Jan 24, 2025

devongovett Jan 25, 2025

yihuiliao Jan 29, 2025 •

edited

Loading

snowystinger Jan 29, 2025 •

edited

Loading

yihuiliao Jan 30, 2025

devongovett Jan 28, 2025 •

edited

Loading

fix: correctly format date/time in RTL #7423

fix: correctly format date/time in RTL #7423

Conversation

yihuiliao commented Nov 21, 2024 • edited Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

rspbot commented Nov 21, 2024

sadeghbarati commented Dec 2, 2024 • edited Loading

yihuiliao commented Dec 2, 2024

rspbot commented Dec 4, 2024

rspbot commented Dec 6, 2024

rspbot commented Dec 20, 2024

rspbot commented Jan 8, 2025

rspbot commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuiliao Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rspbot commented Jan 22, 2025

rspbot commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuiliao Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

rspbot commented Jan 24, 2025

snowystinger left a comment

Choose a reason for hiding this comment

yihuiliao commented Jan 24, 2025

Choose a reason for hiding this comment

yihuiliao Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

snowystinger Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devongovett Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

yihuiliao commented Nov 21, 2024 •

edited

Loading

sadeghbarati commented Dec 2, 2024 •

edited

Loading

yihuiliao Jan 21, 2025 •

edited

Loading

yihuiliao Jan 24, 2025 •

edited

Loading

yihuiliao Jan 29, 2025 •

edited

Loading

snowystinger Jan 29, 2025 •

edited

Loading

devongovett Jan 28, 2025 •

edited

Loading