Skip to content

feat(ui5-range-slider): focus and keyboard handling implementation #2620

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

Merged
merged 23 commits into from
Jan 20, 2021

Conversation

ndeshev
Copy link
Contributor

@ndeshev ndeshev commented Dec 23, 2020

The Range Slider is now focusable, the elements that gets the focus when
the component is active are the progress bar and the two handles. A new private property
'focused' is added.

The full keyboard handling specifications are implemented:

  • [TAB] Forward navigation
    -- On entering, move focus to the progress bar.
    -- If the focus is on the progress bar move it to the left handle
    -- If the focus is on the left handle move it to the right handle
    -- If focus is on the right handle, leave Range Slider

  • [SHIFT] + [TAB] Backward navigation
    -- On entering, move focus to the right handle.
    -- If the focus is on the right handle move it to the left handle
    -- If the focus is on the left handle move it to the progress bar
    -- If focus is on the left handle, leave Range Slider

  • [RIGHT], [UP], or [+] Increases the value of the Range Slider by a small increment.
    -- The focused Range Slider handle moves to the right/ up by the according value.
    -- If the focused Range Slider handle is at the maximum value position, do nothing.
    -- If the focus is on the whole selected range move it with the offset by the according value.

  • [LEFT], [DOWN], or [-] Decreases the value of the Range Slider by a small increment
    -- The focused Range Slider handle moves to the left/ down by the according value.
    -- If the focused Range Slider handle is at the minimum value position, do nothing.
    -- If the focus is on the whole selected range move it with the offset by the according value.

  • [CTRL]+[RIGHT], [CTRL]+[UP], or [PAGE UP] Increases the value of the Range Slider by a large increment
    -- The focused Range Slider handle moves to the right/ up accordingly.
    -- If the focused Range Slider handle is at the maximum value position, do nothing.
    -- If the focus is on the whole selected range move it with the offset by the according value.

  • [CTRL]+[LEFT], [CTRL]+[DOWN], or [PAGE DOWN] Decreases the value of the Range Slider by a large increment.
    -- The focused Range Slider handle moves to the left/ down accordingly.
    -- If the focused Range Slider handle is at the minimum value position, do nothing.
    -- If the focus is on the whole selected range move it with the offset by the according value.


  • [HOME] Sets the minimum value. The Range Slider handles or progress bar move to the leftmost/ bottommost position.
  • [END] Sets the maximum value. The Range Slider handles or progress bar move to the rightmost/ topmost position.
  • [ESC] If focus is on a thumb, revert this thumb to the value which it had when it got the focus.

The Slider is now focusable, the element that gets the focus when
the component is active is the slider's handle. A new private property
'focused' is added.

The full keyboard handling specifications are implemented.
@ndeshev ndeshev marked this pull request as draft December 23, 2020 06:36
@ndeshev ndeshev force-pushed the rangeslider-focus-keyboard-handling branch from 5b8f2ff to 3c5a249 Compare December 30, 2020 07:53
@ndeshev ndeshev marked this pull request as ready for review December 30, 2020 08:00
@ndeshev ndeshev force-pushed the rangeslider-focus-keyboard-handling branch 3 times, most recently from d66f5f8 to 3e7dacc Compare January 4, 2021 05:10
The Range Slider is now focusable, the elements that gets the focus when
the component is active are the slider's handles and progress tracker.

The full keyboard handling specifications are implemented.
@ndeshev ndeshev force-pushed the rangeslider-focus-keyboard-handling branch from 3e7dacc to e7130cc Compare January 4, 2021 05:29
-- onEnterDom() moved from Slider.js to SliderBase.js
-- A couple of keyboard handling rare cases are fixed
-- The private 'focus' property is removed as it is no longer used
-- Keyboard interactions with inactive Slider (with step 0) are prevented
-- Refactoring of the logic about actions onto the selected range
@d3xter666
Copy link
Contributor

  1. When you start moving the focus with TAB and move the handles with the arrows, at some point the focus "disappears"

  2. I'm not sure which is the correct behavior, but ESC IMO does not work as expected. Currently, when you play with the slider and hit ESC, it resets just one of the handles. My expectation was that regardless of which handles (or both) you have moved or focused within a single slider, when you hit ESC, the whole slider would be reset. If there's no clear spec, probably this worths discusiing it with UX.

Copy link
Contributor

@fifoosid fifoosid left a comment

Choose a reason for hiding this comment

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

This review is related to the code of the change rather than to the behaviour of the component

}

get tabIndexProgress() {
return this.tabIndex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need a getter for a property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the upcoming accessibility commit implementing the aria attributes according to the standards we are removing both this getter and the tabindex private property because they will be be no longer needed in results of a couple template changes of the Slider and & Range Slider components.

Copy link
Contributor

@fifoosid fifoosid left a comment

Choose a reason for hiding this comment

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

LGTM, someone from @SAP/ui5-webcomponents-topic-rl should approve the behaviour

Copy link
Contributor

@d3xter666 d3xter666 left a comment

Choose a reason for hiding this comment

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

I'm fine with the current behaviour

@MapTo0 MapTo0 merged commit 8c71ca4 into SAP:master Jan 20, 2021
kineticjs pushed a commit to kineticjs/ui5-webcomponents that referenced this pull request Jan 25, 2021
alexandar-mitsev pushed a commit to alexandar-mitsev/ui5-webcomponents that referenced this pull request Feb 1, 2021
NHristov-sap pushed a commit to NHristov-sap/ui5-webcomponents that referenced this pull request Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants