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

Columns slider: Number of columns reset to 1 when using Tab+Shift #34363

Closed
2 tasks done
TeBenachi opened this issue Aug 27, 2021 · 6 comments · Fixed by #35020
Closed
2 tasks done

Columns slider: Number of columns reset to 1 when using Tab+Shift #34363

TeBenachi opened this issue Aug 27, 2021 · 6 comments · Fixed by #35020
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@TeBenachi
Copy link
Contributor

Description

After adding the columns using the columns slider in the sidebar, the columns slider resets to 1 when moving the focus back using Tab + Shift key.

The number of columns is expected not to change.

Step-by-step reproduction instructions

  1. Place focus on the Color section on the side bar. (the color panel should stay closed)
  2. Press Tab key once to move the focus on the Columns slider
  3. Use the arrow key to add columns
  4. Use Tab + Shift to move the focus back to the Color
  5. Columns slider reset to 1

Screenshots, screen recording, code snippet

columns-slider

Environment info

No response

Pre-checks

  • I have searched the existing issues.
  • I have tested with all plugins deactivated except Gutenberg.
@RajkiranBagal
Copy link

Hey @TeBenachi ,

I checked this issue at my end & this seems to be fixed.

Reference - https://d.pr/v/WEWD3B

Can you please check again?

Let me know if I am missing something.

Regards
Rajkiran

@TeBenachi
Copy link
Contributor Author

TeBenachi commented Aug 29, 2021

Hi @RajkiranBagal

Thank you for checking. Having the focus move to the number box does not cause to reset, however
navigating back to the color section directly from the slider still seem to reset.

@annezazu annezazu added [a11y] Keyboard & Focus [Block] Navigation Affects the Navigation Block [Type] Bug An existing feature does not function as intended labels Aug 30, 2021
@talldan talldan added [Block] Columns Affects the Columns Block and removed [Block] Navigation Affects the Navigation Block labels Sep 1, 2021
@stokesman
Copy link
Contributor

I tried to reproduce this with WP 5.8 and could not. I used Brave (Version 1.28.105 Chromium: 92.0.4515.131) on macOS 10.12.6.

@TeBenachi, it would be helpful if we could know the OS and browser you encounter this issue with. It could also help to know the versions of OS/browser and WordPress too.

@stokesman
Copy link
Contributor

stokesman commented Sep 9, 2021

I can reproduce this in Firefox on macOS. Probably Firefox on any OS will be the same. I think what is happening is that in the RangeControl component, holding Shift changes the step attribute to 10 making any value that's not a multiple of 10 invalid and as stated on the MDN docs for <input[type="range">:

Note: When the data entered by the user doesn't adhere to the stepping configuration, the user agent may round to the nearest valid value, preferring numbers in the positive direction when there are two equally close options.

So it appears that Firefox is rounding to the nearest valid value upon blur.

To avoid this we'll probably have to change how we apply the shiftStep in RangeControl. Instead of changing the step attribute while Shift is pressed and relying on the default UA behavior we could prevent the default and do it ourselves (a precedent for such is in NumberControl). I've got a bit of work to do on RangeControl and had been considering proposing such a change. I'll try it out and if it resolves this I'll be sure to link this issue.

@stokesman stokesman self-assigned this Sep 9, 2021
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Sep 9, 2021
@stokesman stokesman added [Package] Components /packages/components and removed [Block] Columns Affects the Columns Block labels Sep 10, 2021
@ciampo
Copy link
Contributor

ciampo commented Sep 14, 2021

I think what is happening is that in the RangeControl component, holding Shift changes the step attribute to 10 making any value that's not a multiple of 10 invalid and as stated on the MDN docs for <input[type="range">

Since the max is 6, why doesn't the value get clamped to 6 when shift-increasing?

@stokesman
Copy link
Contributor

While step is 10 it makes the max of 6 an invalid step so the UA won't go there. That can be tested with a plain HTML <input type="range" min="0" max="6" step="10">. So it's another consequence of altering the step attribute (and another thing the PR I've linked resolves 😁 ).

I may go ahead and re-title this issue as it can actually happen with any almost any RangeControl. Some examples will follow.

Storybook

Also showing that you can recreate it withShift + click):

range-control-shift-tab-reset-storybook.mp4

Spacer blocks height control

range-control-shift-tab-reset-spacer-height.mp4

Cover blocks overlay opacity control:

range-control-shift-tab-reset-cover-opacity.mp4

@ciampo ciampo mentioned this issue Oct 1, 2021
62 tasks
@priethor priethor added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
7 participants