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

Font Size Picker: Remove Custom option from FontSizePickerSelect dropdown #69038

Conversation

himanshupathak95
Copy link
Contributor

@himanshupathak95 himanshupathak95 commented Feb 5, 2025

What?

Closes #69029

Removes the 'Custom' option from the FontSizePicker's select dropdown while maintaining custom font size functionality through the settings icon button.

Why?

The current implementation includes a 'Custom' option in the font size dropdown when there are more than 5 font sizes. This creates two issues:

  • Semantically incorrect: A select/listbox control should contain values, not interactive controls that change the UI
  • Accessibility violation: When using keyboard navigation, reaching the 'Custom' option unexpectedly changes the entire UI context and loses focus
  • Redundant functionality: The same custom size feature is already available through the settings icon button

Testing Instructions

  • Use a theme with more than 5 font sizes (e.g., modify Twenty Twenty-Four's theme.json to add extra font sizes)
  • Open the block editor
  • Select a Paragraph block
  • Click on the Font size picker in the inspector panel and verify the "Custom" option is removed

Testing Instructions for Keyboard

  • Use a theme with more than 5 font sizes (e.g., modify Twenty Twenty-Four's theme.json to add extra font sizes)
  • Open the block editor
  • Use Tab to focus on the font size and select dropdown
  • Use arrow keys to navigate through options
  • Verify that no unexpected UI changes occur

Screencast

Before -

Screen.Recording.2025-02-05.at.10.45.04.mov

After -

Screen.Recording.2025-02-05.at.10.32.17.mov

@himanshupathak95
Copy link
Contributor Author

himanshupathak95 commented Feb 5, 2025

This PR currently just removes the "Custom" option from the dropdown for the reasons mentioned by @afercia in #69029 (comment)

This is a WIP and the following things can be implemented after discussion -

  • Opening of the dropdown and persistence while navigating options through keystrokes
  • Autofocus on the custom sizes control

This is a screencast of the current changes -

Before -

Screen.Recording.2025-02-05.at.10.45.04.mov

After -

Screen.Recording.2025-02-05.at.10.32.17.mov

After discussing and refining the approach, I will take care of the failing test cases.

@afercia
Copy link
Contributor

afercia commented Feb 5, 2025

@himanshupathak95 thanks for the PR.
I'd suggest to keep the changes into the scope of the issue and not proceed with the additional points.
When you have a chance, please add the What, Why, How, Testing Instructions to the PR description so it can be reviewed 🙏🏻

@himanshupathak95 himanshupathak95 marked this pull request as ready for review February 11, 2025 09:13
Copy link

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: himanshupathak95 <abcd95@git.wordpress.org>
Co-authored-by: afercia <afercia@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@himanshupathak95
Copy link
Contributor Author

Thanks for the suggestions @afercia 🙌🏻

I have removed the unit tests that accounted for the "Custom" option, and I have also updated the PR description. Please take a look whenever time permits and let me know if it tests well for you 🙇🏻

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components labels Feb 11, 2025
Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @himanshupathak95
Looks good to me. The only thing left is adding a changelog entry in packages/components/CHANGELOG.md

Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

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

LGTM

@afercia afercia merged commit 9182d59 into WordPress:trunk Feb 12, 2025
61 checks passed
@github-actions github-actions bot added this to the Gutenberg 20.3 milestone Feb 12, 2025
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 [Type] Bug An existing feature does not function as intended
Projects
Development

Successfully merging this pull request may close these issues.

Font size picker: unexpected change of context when selecting Custom option from select via the keyboard
2 participants