Font Library: Fix focus loss when update/install button is pressed #58364
Conversation
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
t-hamano
added
[Type] Bug
|
Size Change: +18 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
@t-hamano nice addition, thank you.
Are these two bugs already tracked in other GitHub issues? |
|
@afercia Thanks for the review!
As far as I know, it hasn't been reported anywhere yet. I plan to submit both problems as issues later. |
afercia
requested changes
Jan 29, 2024
There was a problem hiding this comment.
Sorry I spoke too soon.
We also need to noop the buttons when they are aria-disabled. The pattern to use is similar to the one used for many other buttons e.g. the Save button. something like
onClick={ (condition to disable the button) ? undefined : callback }
afercia
reviewed
Jan 29, 2024
| @@ -299,7 +300,7 @@ function Footer( { handleInstall } ) { | |||
| variant="primary" | |||
| onClick={ handleInstall } | |||
There was a problem hiding this comment.
The button should be noop'ed when isDisabled || isInstalling
afercia
reviewed
Jan 29, 2024
| @@ -186,7 +186,7 @@ function Footer( { shouldDisplayDeleteButton, handleUninstallClick } ) { | |||
| ) } | |||
| </div> | |||
| <Button | |||
| disabled={ ! fontFamiliesHasChanges } | |||
| aria-disabled={ ! fontFamiliesHasChanges } | |||
| variant="primary" | |||
| onClick={ saveFontFamilies } | |||
There was a problem hiding this comment.
The button should be noop'ed when ! fontFamiliesHasChanges
Yes, I just noticed that too, so I updated it 😅 |
|
Oops, I noticed that the following issue has already been reported.
|
afercia
approved these changes
Jan 30, 2024
|
@afercia Thanks for the review! As seen in this comment on slack, it seems that the test failure also occurred on trunk. This problem is now resolved, so I merged the trunk again. All tests should now pass. |
|
Thanks @t-hamano. Yes Inoticed other PRs failing today and I rebased them. I think the preferred flow in this project is to rebase on top of trunk and not merge? |
|
I don't think there is a strict rule as to whether we should use merge or rebase. It seems that different people prefer different things. Personally, when I want to import trunk changes, I follow the following rules:
If I already have a comment tree hanging in a PR, I feel like a force push will interrupt that timeline. However, Gutenberg uses squash merge by default, so no matter how many commits are made to one PR, they will be aggregated into one commit in the trunk. So in the end, I think either one is fine 👍 |
Right, thanks. |
|
Thank you @t-hamano for working on this! As a small follow-up, would you mind using the Instead of using |
All right 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
This PR fixes an issue where the font library loses focus when updating or installing fonts.
Why?
Once the font is updated and installed, the button will have a disabled attribute added to it. If the button has this attribute, it is not possible to focus on the button and the button loses focus. Users who only use the keyboard may have a hard time returning focus to the Font Library.
How?
I used
aria-disabled="true"instead ofdisabled. This allows the button to maintain focus even if it is inactive. This is the approach also used by the Post Editors and the Site Editor.Also, in the current implementation, the footer itself is not displayed when no font is selected to be installed. This PR displays the footer even when no font is selected and renders an inactive button.
Testing Instructions for Keyboard
Note
Unlike installing fonts, updating fonts does not notify the screen reader of any information, so users cannot understand what happened. This is also an issue with trunk, so it needs to be fixed separately. Additionally, focus is also lost when the font is removed, so this also needs to be fixed separately.
Screenshots or screencast
c341c5efacd1bbf292fba6e8d3644717.mp4