-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Try adding a keyboard-navigable font size selector with visual feedback #17418
Closed
+252
−21
Closed
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
466e116
Try addinig a keyboard navigatble font size selector with visual feed…
youknowriad a5f3f0c
The label of the button should not be updated
youknowriad f56c5c5
Fix label output and remove combobox role.
tellthemachines a1474aa
Return focus to element that opens dropdown.
tellthemachines 721b371
Handle setting value with arrow keys
tellthemachines 2152dd9
Change labelling and remove invalid aria.
tellthemachines 412facf
Fix styles.
tellthemachines 5c72293
Change e2e selectors.
tellthemachines 4294522
Add aria label and id to button control.
tellthemachines c043dff
Don't allow arrow key nav with dropdown closed.
tellthemachines File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| /** | ||
| * External dependencies | ||
| */ | ||
| import { map } from 'lodash'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { withInstanceId } from '@wordpress/compose'; | ||
| import { useRef, useCallback, useEffect } from '@wordpress/element'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { | ||
| Dashicon, | ||
| BaseControl, | ||
| Button, | ||
| NavigableMenu, | ||
| Dropdown, | ||
| } from '../'; | ||
|
|
||
| function FontSizePickerSelect( { | ||
| fontSizes = [], | ||
| instanceId, | ||
| onChange, | ||
| value, | ||
| } ) { | ||
| const currentFont = fontSizes.find( ( font ) => font.value === value ); | ||
| const currentFontLabel = currentFont ? currentFont.label : ''; | ||
|
|
||
| // Work-around to focus active item | ||
| const popoverRef = useRef( null ); | ||
| const focusActiveItem = useCallback( () => { | ||
| // Hack work-arounds to control focus timing... | ||
| window.requestAnimationFrame( () => { | ||
| const { current } = popoverRef; | ||
| if ( ! current ) { | ||
| return; | ||
| } | ||
| const node = current.querySelector( '[aria-selected="true"]' ); | ||
| node.blur(); | ||
| window.requestAnimationFrame( () => { | ||
| if ( node ) { | ||
| // Hack work-arounds to control focus timing... | ||
| node.focus(); | ||
| } | ||
| } ); | ||
| } ); | ||
| }, [] ); | ||
|
|
||
| useEffect( () => { | ||
| focusActiveItem(); | ||
| }, [ focusActiveItem, value ] ); | ||
|
|
||
| // Work around to manage + force open state outside of Dropdown | ||
| const handleOnToggle = ( nextOpen ) => { | ||
| if ( nextOpen ) { | ||
| focusActiveItem(); | ||
| } | ||
| }; | ||
|
|
||
| // Work around to prevent scrolling. | ||
| // Need to adjust navigable-content/container | ||
| // https://github.com/WordPress/gutenberg/blob/master/packages/components/src/navigable-container/container.js#L89 | ||
| const handleOnKeyDown = ( event ) => { | ||
| // event.preventDefault(); | ||
| const { key } = event; | ||
| switch ( key ) { | ||
| case 'ArrowDown': | ||
| event.preventDefault(); | ||
| break; | ||
| case 'ArrowUp': | ||
| event.preventDefault(); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| }; | ||
|
|
||
| // Improve voiceover consistency compared to native select | ||
| const ariaHasPopup = 'listbox'; | ||
| const ariaProps = { | ||
| 'aria-haspopup': ariaHasPopup, | ||
| }; | ||
|
|
||
| const id = `font-size-picker__select-${ instanceId }`; | ||
|
|
||
| return ( | ||
| <BaseControl className="components-font-size-picker__select"> | ||
| <Dropdown | ||
| className="components-font-size-picker__select-dropdown" | ||
| contentClassName="components-font-size-picker__select-dropdown-content" | ||
| position="bottom" | ||
| focusOnMount="container" | ||
| onToggle={ handleOnToggle } | ||
| renderToggle={ ( { onToggle } ) => ( | ||
| <label htmlFor={ id } className="components-font-size-picker__select-label"> | ||
| { __( 'Preset Size' ) } | ||
| <Button | ||
| aria-label={ __( 'Preset Size' ) } | ||
| className="components-font-size-picker__select-selector" | ||
| id={ id } | ||
| isLarge | ||
| onClick={ onToggle } | ||
| { ...ariaProps } | ||
| > | ||
| { currentFontLabel } | ||
| </Button> | ||
| </label> | ||
| ) } | ||
| renderContent={ () => { | ||
| return ( | ||
| <NavigableMenu | ||
| role="listbox" | ||
| aria-label="Choose Font Size" | ||
| onKeyDown={ handleOnKeyDown } | ||
| > | ||
| <div ref={ popoverRef }> | ||
| { map( fontSizes, ( option ) => { | ||
| const isSelected = value === option.value; | ||
| const optionLabel = isSelected ? `${ option.label } (Selected)` : option.label; | ||
| const itemId = `item-${ option.value }`; | ||
| const itemRole = 'option'; | ||
| const labelRole = 'presentation'; | ||
|
|
||
| return ( | ||
| <Button | ||
| key={ option.value } | ||
| onClick={ () => { | ||
| onChange( option.value ); | ||
| } } | ||
| className={ `is-font-${ option.value }` } | ||
| role={ itemRole } | ||
| id={ itemId } | ||
| aria-label={ optionLabel } | ||
| aria-selected={ isSelected } | ||
| > | ||
| { isSelected && <Dashicon icon="saved" /> } | ||
| <span | ||
| className="components-font-size-picker__select-dropdown-text-size" | ||
| style={ option.size ? { fontSize: option.size } : {} } | ||
| role={ labelRole } | ||
| > | ||
| { option.label } | ||
| </span> | ||
| </Button> | ||
| ); | ||
| } ) } | ||
| </div> | ||
| </NavigableMenu> | ||
| ); | ||
| } } | ||
| /> | ||
| </BaseControl> | ||
| ); | ||
| } | ||
|
|
||
| export default withInstanceId( FontSizePickerSelect ); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| .components-font-size-picker__select { | ||
| // Need to override a generic rule | ||
| // that can apply to all controls in the sidebar | ||
| // Ideally that generic rule shouldn't target | ||
| // this select in the first place. | ||
| margin-bottom: 0 !important; | ||
|
|
||
| .components-base-control__field { | ||
| margin-bottom: 0; | ||
| } | ||
| } | ||
|
|
||
| .components-font-size-picker__select-dropdown { | ||
| display: inline-block; | ||
| width: 80px; | ||
| } | ||
|
|
||
| .components-font-size-picker__select-dropdown-content button { | ||
| display: block; | ||
| width: 100%; | ||
| text-align: left; | ||
| position: relative; | ||
| padding: 10px 5px; | ||
| line-height: 1; | ||
| } | ||
|
|
||
| .components-font-size-picker__select-dropdown-content button:hover { | ||
| background: #eee !important; | ||
| } | ||
|
|
||
| .components-font-size-picker__select-dropdown-content button.highlighted { | ||
| background: #eee !important; | ||
| } | ||
|
|
||
| .components-font-size-picker__select-dropdown-content button svg { | ||
| position: absolute; | ||
| left: 5px; | ||
| top: 50%; | ||
| transform: translateY(-50%); | ||
| } | ||
|
|
||
| .components-font-size-picker__select-dropdown-text-size { | ||
| margin-left: 30px; | ||
| display: block; | ||
| } | ||
|
|
||
| .components-font-size-picker__select-selector.components-font-size-picker__select-selector { | ||
| background: #fff; | ||
| border: 1px solid $dark-gray-200; | ||
| border-radius: 4px; | ||
| box-shadow: none; | ||
| margin-top: $grid-size; | ||
| padding-left: $grid-size-small; | ||
| position: relative; | ||
| } | ||
|
|
||
| .components-font-size-picker__select-selector::after { | ||
| content: "▼"; | ||
| position: absolute; | ||
| right: $grid-size-small; | ||
| } | ||
|
|
||
| .components-font-size-picker__select-selector:focus { | ||
| border: 1px solid $blue-medium-focus !important; | ||
| outline: none !important; | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be interesting to know the reasoning behind removing this code. I'm not sure what it does, but I think it terms of documenting its removal some details in the PR description would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this seems dangerous for me. This means if we close a modal/popover programmatically (not by focusing something else explicitly) and the focus is outside of that modal/popover when we do so, the focus will return to the button that opened the popover) which IMO is not what we want right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What it currently does is return if no element within the wrapped component is focused, so that focus goes back to the document body. I can't think of any situation where we would want this to happen, especially when using a component the sole purpose of which is to return focus to the previously focused element when closing a modal or popover. So I'm particularly curious to know why it was added in the first place. I know it's part of this changeset but I can't reproduce the issue it fixes after my changes.
Note that this return doesn't get triggered when we deliberately move focus to a specific element, such as when we use the Block Navigator. It only gets triggered when focus is for some reason lost while we are still inside the wrapped component. This is something that, for the sake of accessibility, should never happen, so I'm not sure that we should be catering to it.
Removing this code fixes the issue of focus being lost when closing the font size dropdown because in the
focusActiveItemfunction focus is blurred from the dropdown just before closing. I'm not sure why we're managing focus with this function though; I thoughtNavigableMenuwould be enough to provide keyboard navigation with the arrow keys? Perhaps @youknowriad could shed some light on this.Happy to hear further feedback and suggestions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this check exists to address the issues that there might be several nested components that are wrapped with
withFocusReturnand all of them can respond when they get unmounted. I suspect, this is a way to ensure that it's handled only once in such a case. This should be confirmed though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I don't think that's what it does, because if we have nested
withFocusReturns (such as this font size picker nested in the sidebar) the parent keeps tracking focus even when it's inside the child component, so I wouldn't expect focus to be lost in the parent unless it were also lost in the child.We don't seem to have any instance of a parent popover/withFocusReturn-wrapped component that closes when a child popover is closed (that's probably an accessibility anti-pattern anyway), but I have verified that, with the current changes, when opening the font size picker and then closing the sidebar with it still open, focus is correctly transferred to the open sidebar button.