Fix backspace to shift focus to previous element in useDateSegment #5715

ryo-manba · 2024-01-18T14:51:24Z

Closes #5261 , #5223

✅ 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:

Test 1: Re-entering Data in the Time Field

Visit the Date Field Example in Storybook.
Enter "45" into the Number Field for minutes.
Clear the entry using the backspace key.
Enter a new value, such as "30", for minutes.
Confirm that you can correctly re-enter the minutes.

Test 2: Backspace Focus Shift on Empty DateSegment

Visit the Date Field Example in Storybook.
Enter "45" into the Number Field for minutes.
Use the backspace key to clear the minutes entered.
Press backspace again when the field is empty (showing placeholder).
Confirm that the focus shifts to the previous field.
Refocus on the minutes field and enter a new value, such as "30".
Confirm that the minutes can be correctly re-entered.

🧢 Your Project:

LFDanLu · 2024-01-22T19:28:14Z

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

@@ -98,6 +98,7 @@ export function useDateSegment(segment: DateSegment, state: DateFieldState, ref:
      let parsed = parser.parse(newValue);
      if (newValue.length === 0 || parsed === 0) {
        state.clearSegment(segment.type);
+        focusManager.focusPrevious();


Personally, I feel like focus should remain in the field. I'm used to deleting the entire segment to type in my new input, but open to other opinions from the team

I think if the segment is already cleared, it's improved UX if backspace moves focus to the previous segment. We just should ensure the focus doesn't wrap (i.e. don't move focus if focus is already on the first segment.)

Yeah, I think I agree with Daniel on this one. Personally, if I backspace to clear out a segment, that means I want to enter a new value for that same segment, not go back to the previous segment.

Just sharing my thoughts here - in the current implementation of DateSegment, it automatically moves to the next field once the input is complete, but it doesn’t revert to the previous field when using the backspace key. This is aligned with Chrome’s behavior.

However, thinking about consistency, it somehow feels more intuitive if the backspace would actually take us back to the previous field.

In the case of Safari, the focus doesn’t automatically move to the next field after input, nor does it revert to the previous one with a backspace. This can be confirmed at the following link.
https://developer.mozilla.org/docs/Web/HTML/Element/input/date

If everyone is on board with this behavior, I will make the adjustment as suggested by @reidbarber, ensuring that the focus doesn't move back if it's already on the first segment.

I feel like our current implementation is fine. It seems more natural for the focus to shift to the previous input field when the final value is deleted, not just when the current segment is empty. For instance, if the focus is on a placeholder and backspace moves the focus back, that feels like too much. What do you all think?

IMO I feel like auto moving focus once the input segment is cleared is too much because it assumes the user is wants to move to the previous date segment when they could just be trying to input another value in the original input segment. Auto moving focus forward when a segment is filled in on the other hand feels fine because there isn't another operation that the user might be doing once the segment is filled in

I concur with @LFDanLu +1

Okay, I certainly understand that reasoning. In that case, let's proceed with adjusting the behavior to move focus back with the Backspace key only when the date segment is empty.

I have fixed it, please review when you have time!

snowystinger · 2024-02-11T23:10:41Z

GET_BUILD

rspbot · 2024-02-11T23:24:03Z

Build successful! 🎉

snowystinger

Thanks for the PR, looks like it's working well. Tested in Chrome/Safari/FF on MacOS

LFDanLu · 2024-02-16T19:25:52Z

GET_BUILD

LFDanLu · 2024-02-16T20:07:52Z

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

      let newValue = segment.text.slice(0, -1);
+      newValue = newValue === '0' ? '' : newValue;
      let parsed = parser.parse(newValue);


Did some digging and I think we'll need to change this bit of logic to something like

Suggested change

let newValue = segment.text.slice(0, -1);

newValue = newValue === '0' ? '' : newValue;

let parsed = parser.parse(newValue);

let newValue = segment.text.slice(0, -1);

let parsed = parser.parse(newValue);

newValue = parsed === 0 ? '' : newValue;

The reason for this is that the segment text is not always to be Arabic numerals (e.g. 0, 1, 2, 3 etc). If you go to this story locally and change the locale in the story to "Arabic (Egypt)" and log out what newValue is, you'll notice that it will return values like ٤. If you then switch your keyboard into an Arabic format so pressing 1 would return ١, you would then see the bug still exists where the focus auto advances after clearing the minutes time field. If we parse the value first, we can get around this issue

Lemme know if that makes sense or if you need help reproducing !

Ah, I see. I had overlooked that perspective, thanks for pointing it out! I've tried it locally and I understand the issue now.

It looks like we can also fix the bug where input becomes impossible after using backspace to clear the minutes field in cases other than Arabic numerals, like with Arabic (Egypt).

I'll be adopting the commit you suggested. Thanks!

Co-authored-by: Daniel Lu <[email protected]>

LFDanLu · 2024-02-20T19:26:02Z

GET_BUILD

rspbot · 2024-02-20T19:41:30Z

Build successful! 🎉

rspbot · 2024-02-20T19:41:32Z

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

LFDanLu

LGTM, thanks for the PR!

Fix focus shift on clearing minute fields

73e3cb4

ryo-manba mentioned this pull request Jan 18, 2024

Second digit not working on time field after previous data is cleared #5223

Closed

fix: time field focus

47e8b54

ryo-manba changed the title ~~Fix focus shift on clearing minute fields~~ Fix focus shift on useDateSegment Jan 18, 2024

ryo-manba changed the title ~~Fix focus shift on useDateSegment~~ Fix backspace to shift focus to previous element in useDateSegment Jan 18, 2024

add: test for focusing previous segment when backspacing date input

5c5e6d1

LFDanLu reviewed Jan 22, 2024

View reviewed changes

ryo-manba added 3 commits February 9, 2024 22:09

fix: return to previous field if segment is empty

524a0f3

revert unnecessary test modifications

6ffe217

Merge branch 'main' into fix/date-segment-backspace

d2a429f

ryo-manba requested review from snowystinger, reidbarber, LFDanLu and yihuiliao February 9, 2024 13:45

ryo-manba mentioned this pull request Feb 9, 2024

DateInput splits up two digit numbers #5846

Closed

snowystinger previously approved these changes Feb 11, 2024

View reviewed changes

Merge branch 'main' into fix/date-segment-backspace

d11dea1

LFDanLu requested changes Feb 16, 2024

View reviewed changes

Update packages/@react-aria/datepicker/src/useDateSegment.ts

6378256

Co-authored-by: Daniel Lu <[email protected]>

ryo-manba dismissed snowystinger’s stale review via 6378256 February 17, 2024 05:46

ryo-manba requested review from LFDanLu and snowystinger February 17, 2024 05:46

snowystinger and others added 2 commits February 19, 2024 16:46

Merge branch 'main' into fix/date-segment-backspace

26b7339

Merge branch 'main' into fix/date-segment-backspace

7f463a1

LFDanLu approved these changes Feb 20, 2024

View reviewed changes

snowystinger approved these changes Feb 20, 2024

View reviewed changes

snowystinger merged commit fd3f09c into adobe:main Feb 20, 2024
26 checks passed

ryo-manba deleted the fix/date-segment-backspace branch February 21, 2024 00:22

ryo-manba mentioned this pull request Feb 22, 2024

Unable to correct value in day segment on useDateField #4087

Closed

ryo-manba mentioned this pull request Mar 10, 2024

Unexpected behavior with DateField auto tabbing #4174

Closed

ttsirkia mentioned this pull request May 2, 2024

Usability issues in DateField/DatePicker #5364

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix backspace to shift focus to previous element in useDateSegment #5715

Fix backspace to shift focus to previous element in useDateSegment #5715

ryo-manba commented Jan 18, 2024 •

edited

Loading

LFDanLu Jan 22, 2024

reidbarber Jan 22, 2024

yihuiliao Jan 22, 2024

ryo-manba Jan 23, 2024

ryo-manba Jan 23, 2024

ryo-manba Jan 30, 2024 •

edited

Loading

LFDanLu Jan 30, 2024

snowystinger Feb 5, 2024

ryo-manba Feb 6, 2024

ryo-manba Feb 9, 2024 •

edited

Loading

snowystinger commented Feb 11, 2024

rspbot commented Feb 11, 2024

snowystinger left a comment

LFDanLu commented Feb 16, 2024

LFDanLu Feb 16, 2024

ryo-manba Feb 17, 2024

LFDanLu commented Feb 20, 2024

rspbot commented Feb 20, 2024

rspbot commented Feb 20, 2024

LFDanLu left a comment

Fix backspace to shift focus to previous element in useDateSegment #5715

Fix backspace to shift focus to previous element in useDateSegment #5715

Conversation

ryo-manba commented Jan 18, 2024 • edited Loading

✅ Pull Request Checklist:

📝 Test Instructions:

Test 1: Re-entering Data in the Time Field

Test 2: Backspace Focus Shift on Empty DateSegment

🧢 Your Project:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryo-manba Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryo-manba Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

snowystinger commented Feb 11, 2024

rspbot commented Feb 11, 2024

snowystinger left a comment

Choose a reason for hiding this comment

LFDanLu commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LFDanLu commented Feb 20, 2024

rspbot commented Feb 20, 2024

rspbot commented Feb 20, 2024

LFDanLu left a comment

Choose a reason for hiding this comment

ryo-manba commented Jan 18, 2024 •

edited

Loading

ryo-manba Jan 30, 2024 •

edited

Loading

ryo-manba Feb 9, 2024 •

edited

Loading