Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pickers] Add PageUp and PageDown support for time components #14812

Merged
40 changes: 40 additions & 0 deletions packages/x-date-pickers/src/DigitalClock/DigitalClock.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { DIGITAL_CLOCK_VIEW_HEIGHT } from '../internals/constants/dimensions';
import { useControlledValueWithTimezone } from '../internals/hooks/useValueWithTimezone';
import { singleItemValueManager } from '../internals/utils/valueManagers';
import { useClockReferenceDate } from '../internals/hooks/useClockReferenceDate';
import { getFocusedListItemIndex } from '../internals/utils/utils';

const useUtilityClasses = (ownerState: DigitalClockProps<any>) => {
const { classes } = ownerState;
Expand Down Expand Up @@ -115,6 +116,7 @@ export const DigitalClock = React.forwardRef(function DigitalClock<TDate extends

const containerRef = React.useRef<HTMLDivElement>(null);
const handleRef = useForkRef(ref, containerRef);
const listRef = React.useRef<HTMLUListElement>(null);

const props = useThemeProps({
props: inProps,
Expand Down Expand Up @@ -294,6 +296,42 @@ export const DigitalClock = React.forwardRef(function DigitalClock<TDate extends
utils.isEqual(option, valueOrReferenceDate),
);

const handleKeyDown = (event: React.KeyboardEvent) => {
switch (event.key) {
case 'PageUp': {
if (!listRef.current) {
return;
}
Comment on lines +302 to +304
Copy link
Member

@oliviertassinari oliviertassinari Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is impossible to reach?

Suggested change
if (!listRef.current) {
return;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory listRef.current can be null, so this is meant to assure null safety on the lines below (and prevent TS complaints 😅)

Copy link
Member

@oliviertassinari oliviertassinari Oct 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory listRef.current can be null

Only if the ref is not applied to the right element. This sounds unlikely to happen in a future refactoring, not with our test suite.

I think that using listRef.current! so it gets transpiled to listRef.current is much better. No extra bundle size or runtime time spent on this. If done at scale, I would hope this brings more than a +1% improvement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my suggestion to go the safer route and prevent cases where DOM could be tampered with.
But given most other cases, where <x>Ref.current is accessed, we could indeed simplify to listRef.current!.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense ! I will add it to this PR
Thanks for the suggestion 🙏

const newIndex = getFocusedListItemIndex(listRef.current) - 5;
const children = listRef.current?.children;
const newFocusedIndex = Math.max(0, newIndex);

const childToFocus = children[newFocusedIndex];
if (childToFocus) {
(childToFocus as HTMLElement).focus();
}
event.preventDefault();
break;
}
case 'PageDown': {
if (!listRef.current) {
return;
}
const newIndex = getFocusedListItemIndex(listRef.current) + 5;
const children = listRef.current?.children;
const newFocusedIndex = Math.min(children.length - 1, newIndex);

const childToFocus = children[newFocusedIndex];
if (childToFocus) {
(childToFocus as HTMLElement).focus();
}
event.preventDefault();
break;
}
default:
}
};

return (
<DigitalClockRoot
ref={handleRef}
Expand All @@ -302,9 +340,11 @@ export const DigitalClock = React.forwardRef(function DigitalClock<TDate extends
{...other}
>
<DigitalClockList
ref={listRef}
role="listbox"
aria-label={translations.timePickerToolbarTitle}
className={classes.list}
onKeyDown={handleKeyDown}
>
{timeOptions.map((option, index) => {
if (skipDisabled && isTimeDisabled(option)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
digitalClockHandler,
formatFullTimeValue,
} from 'test/utils/pickers';
import { screen } from '@mui/internal-test-utils';
import { fireEvent, screen } from '@mui/internal-test-utils';

describe('<DigitalClock />', () => {
const { render } = createPickerRenderer();
Expand Down Expand Up @@ -92,6 +92,79 @@ describe('<DigitalClock />', () => {
});
});

describe('Keyboard support', () => {
it('should move focus up by 5 on PageUp press', () => {
const handleChange = spy();
render(<DigitalClock autoFocus onChange={handleChange} />);
const options = screen.getAllByRole('option');
const lastOptionIndex = options.length - 1;

const firstElement = options[0];
const lastElement = options[lastOptionIndex];

fireEvent.keyDown(firstElement, { key: 'End' }); // moves focus to last element
fireEvent.keyDown(lastElement, { key: 'PageUp' });
arthurbalduini marked this conversation as resolved.
Show resolved Hide resolved

expect(handleChange.callCount).to.equal(0);
expect(document.activeElement).to.equal(options[lastOptionIndex - 5]);

fireEvent.keyDown(options[lastOptionIndex - 5], { key: 'PageUp' });
expect(handleChange.callCount).to.equal(0);
expect(document.activeElement).to.equal(options[lastOptionIndex - 10]);
});

it('should move focus to first item on PageUp press when current focused item index is among the first 5 items', () => {
const handleChange = spy();
render(<DigitalClock autoFocus onChange={handleChange} />);
const options = screen.getAllByRole('option');

// moves focus to 4th element using arrow down
[0, 1, 2].forEach((index) => {
fireEvent.keyDown(options[index], { key: 'ArrowDown' });
});

fireEvent.keyDown(options[3], { key: 'PageUp' });
expect(handleChange.callCount).to.equal(0);
expect(document.activeElement).to.equal(options[0]);
});

it('should move focus down by 5 on PageDown press', () => {
const handleChange = spy();
render(<DigitalClock autoFocus onChange={handleChange} />);
const options = screen.getAllByRole('option');

fireEvent.keyDown(options[0], { key: 'PageDown' });

expect(handleChange.callCount).to.equal(0);
expect(document.activeElement).to.equal(options[5]);

fireEvent.keyDown(options[5], { key: 'PageDown' });

expect(handleChange.callCount).to.equal(0);
expect(document.activeElement).to.equal(options[10]);
});

it('should move focus to last item on PageDown press when current focused item index is among the last 5 items', () => {
const handleChange = spy();
render(<DigitalClock autoFocus onChange={handleChange} />);
const options = screen.getAllByRole('option');
const lastOptionIndex = options.length - 1;

const firstElement = options[0];
const lastElement = options[lastOptionIndex];

fireEvent.keyDown(firstElement, { key: 'End' }); // moves focus to last element
// moves focus 4 steps above last item using arrow up
[0, 1, 2].forEach((index) => {
fireEvent.keyDown(options[lastOptionIndex - index], { key: 'ArrowUp' });
});
fireEvent.keyDown(options[lastOptionIndex - 3], { key: 'PageDown' });

expect(handleChange.callCount).to.equal(0);
expect(document.activeElement).to.equal(lastElement);
});
});

it('forwards list class to MenuList', () => {
render(<DigitalClock classes={{ list: 'foo' }} />);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
DIGITAL_CLOCK_VIEW_HEIGHT,
MULTI_SECTION_CLOCK_SECTION_WIDTH,
} from '../internals/constants/dimensions';
import { getFocusedListItemIndex } from '../internals/utils/utils';

export interface ExportedMultiSectionDigitalClockSectionProps {
className?: string;
Expand Down Expand Up @@ -187,13 +188,50 @@ export const MultiSectionDigitalClockSection = React.forwardRef(

const focusedOptionIndex = items.findIndex((item) => item.isFocused(item.value));

const handleKeyDown = (event: React.KeyboardEvent) => {
switch (event.key) {
case 'PageUp': {
if (!containerRef.current) {
return;
}
const newIndex = getFocusedListItemIndex(containerRef.current) - 5;
const children = containerRef.current?.children;
const newFocusedIndex = Math.max(0, newIndex);

const childToFocus = children[newFocusedIndex];
if (childToFocus) {
(childToFocus as HTMLElement).focus();
}
event.preventDefault();
break;
}
case 'PageDown': {
if (!containerRef.current) {
return;
}
const newIndex = getFocusedListItemIndex(containerRef.current) + 5;
const children = containerRef.current?.children;
const newFocusedIndex = Math.min(children.length - 1, newIndex);

const childToFocus = children[newFocusedIndex];
if (childToFocus) {
(childToFocus as HTMLElement).focus();
}
event.preventDefault();
break;
}
default:
}
};

return (
<MultiSectionDigitalClockSectionRoot
ref={handleRef}
className={clsx(classes.root, className)}
ownerState={ownerState}
autoFocusItem={autoFocus && active}
role="listbox"
onKeyDown={handleKeyDown}
{...other}
>
{items.map((option, index) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
adapterToUse,
multiSectionDigitalClockHandler,
} from 'test/utils/pickers';
import { screen } from '@mui/internal-test-utils';
import { fireEvent, screen, within } from '@mui/internal-test-utils';

describe('<MultiSectionDigitalClock />', () => {
const { render } = createPickerRenderer();
Expand Down Expand Up @@ -105,4 +105,82 @@ describe('<MultiSectionDigitalClock />', () => {
expect(onChange.lastCall.firstArg).toEqualDateTime(new Date(2019, 0, 1, 15, 30));
});
});

describe('Keyboard support', () => {
it('should move item focus up by 5 on PageUp press', () => {
const handleChange = spy();
render(<MultiSectionDigitalClock autoFocus onChange={handleChange} />);
const hoursSectionListbox = screen.getAllByRole('listbox')[0]; // get only hour section
const hoursOptions = within(hoursSectionListbox).getAllByRole('option');
const lastOptionIndex = hoursOptions.length - 1;

const firstElement = hoursOptions[0];
const lastElement = hoursOptions[lastOptionIndex];

fireEvent.keyDown(firstElement, { key: 'End' }); // moves focus to last element
fireEvent.keyDown(lastElement, { key: 'PageUp' });

expect(handleChange.callCount).to.equal(0);
expect(document.activeElement).to.equal(hoursOptions[lastOptionIndex - 5]);

fireEvent.keyDown(hoursOptions[lastOptionIndex - 5], { key: 'PageUp' });

expect(handleChange.callCount).to.equal(0);
expect(document.activeElement).to.equal(hoursOptions[lastOptionIndex - 10]);
});

it('should move focus to first item on PageUp press when current focused item index is among the first 5 items', () => {
const handleChange = spy();
render(<MultiSectionDigitalClock autoFocus onChange={handleChange} />);
const hoursSectionListbox = screen.getAllByRole('listbox')[0]; // get only hour section
const hoursOptions = within(hoursSectionListbox).getAllByRole('option');

// moves focus to 4th element using arrow down
[0, 1, 2].forEach((index) => {
fireEvent.keyDown(hoursOptions[index], { key: 'ArrowDown' });
});

fireEvent.keyDown(hoursOptions[3], { key: 'PageUp' });
expect(handleChange.callCount).to.equal(0);
expect(document.activeElement).to.equal(hoursOptions[0]);
});

it('should move item focus down by 5 on PageDown press', () => {
const handleChange = spy();
render(<MultiSectionDigitalClock autoFocus onChange={handleChange} />);
const hoursSectionListbox = screen.getAllByRole('listbox')[0]; // get only hour section
const hoursOptions = within(hoursSectionListbox).getAllByRole('option');

fireEvent.keyDown(hoursOptions[0], { key: 'PageDown' });

expect(handleChange.callCount).to.equal(0);
expect(document.activeElement).to.equal(hoursOptions[5]);

fireEvent.keyDown(hoursOptions[5], { key: 'PageDown' });

expect(handleChange.callCount).to.equal(0);
expect(document.activeElement).to.equal(hoursOptions[10]);
});

it('should move focus to last item on PageDown press when current focused item index is among the last 5 items', () => {
const handleChange = spy();
render(<MultiSectionDigitalClock autoFocus onChange={handleChange} />);
const hoursSectionListbox = screen.getAllByRole('listbox')[0]; // get only hour section
const hoursOptions = within(hoursSectionListbox).getAllByRole('option');
const lastOptionIndex = hoursOptions.length - 1;

const firstElement = hoursOptions[0];
const lastElement = hoursOptions[lastOptionIndex];

fireEvent.keyDown(firstElement, { key: 'End' }); // moves focus to last element
// moves focus 4 steps above last item using arrow up
[0, 1, 2].forEach((index) => {
fireEvent.keyDown(hoursOptions[lastOptionIndex - index], { key: 'ArrowUp' });
});

fireEvent.keyDown(hoursOptions[lastOptionIndex - 3], { key: 'PageDown' });
expect(handleChange.callCount).to.equal(0);
expect(document.activeElement).to.equal(lastElement);
});
});
});
8 changes: 8 additions & 0 deletions packages/x-date-pickers/src/TimeClock/Clock.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,14 @@ export function Clock<TDate extends PickerValidDate>(inProps: ClockProps<TDate>)
handleValueChange(viewValue - keyboardControlStep, 'partial');
event.preventDefault();
break;
case 'PageUp':
handleValueChange(viewValue + 5, 'partial');
LukasTy marked this conversation as resolved.
Show resolved Hide resolved
event.preventDefault();
break;
case 'PageDown':
handleValueChange(viewValue - 5, 'partial');
event.preventDefault();
break;
case 'Enter':
case ' ':
handleValueChange(viewValue, 'finish');
Expand Down
2 changes: 0 additions & 2 deletions packages/x-date-pickers/src/TimeClock/TimeClock.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -185,13 +185,11 @@ export const TimeClock = React.forwardRef(function TimeClock<TDate extends Picke
utils.setMinutes(valueOrReferenceDate, timeValue),
'minutes',
);

case 'seconds':
return !shouldDisableTime(
utils.setSeconds(valueOrReferenceDate, timeValue),
'seconds',
);

default:
return false;
}
Expand Down
40 changes: 40 additions & 0 deletions packages/x-date-pickers/src/TimeClock/tests/TimeClock.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,46 @@ describe('<TimeClock />', () => {
expect(reason).to.equal('partial');
});

it('should increase hour selection by 5 on PageUp press', () => {
const handleChange = spy();
render(
<TimeClock
autoFocus
value={adapterToUse.date('2019-01-01T22:20:00')}
onChange={handleChange}
/>,
);
const listbox = screen.getByRole('listbox');

fireEvent.keyDown(listbox, { key: 'PageUp' });

expect(handleChange.callCount).to.equal(1);
const [newDate, reason] = handleChange.firstCall.args;
expect(adapterToUse.getHours(newDate)).to.equal(3);
expect(adapterToUse.getMinutes(newDate)).to.equal(20);
expect(reason).to.equal('partial');
});

it('should decrease hour selection by 5 on PageDown press', () => {
const handleChange = spy();
render(
<TimeClock
autoFocus
value={adapterToUse.date('2019-01-01T02:20:00')}
onChange={handleChange}
/>,
);
const listbox = screen.getByRole('listbox');

fireEvent.keyDown(listbox, { key: 'PageDown' });

expect(handleChange.callCount).to.equal(1);
const [newDate, reason] = handleChange.firstCall.args;
expect(adapterToUse.getHours(newDate)).to.equal(21);
expect(adapterToUse.getMinutes(newDate)).to.equal(20);
expect(reason).to.equal('partial');
});

[
{
keyName: 'Enter',
Expand Down
Loading
Loading