search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.52k stars 4.21k forks source link

Font Library: maintain focus order within nested modal #54431

Closed colorful-tones closed 8 months ago

colorful-tones commented 1 year ago

Description

When testing the work in #53884 it was found that the focus order sequencing fails within the nested modal.

Basically, the Font Library opens in a modal. You can focus on each font in the library. Once you click or enter into a font in the library, you go into a nested context screen that lists the weight variations of that font: 100, 200, 400, etc.

The issue lies when you enter into a font’s nested context screen. The focus shifts to the first variation’s checkbox, which allows the builder to enable or disable that variant.

However, the first focus should go to the ‘Back’ button, which would take the builder back to the Font Library overview, and then the next focus would be the first checkbox for the first variant.

This was initially discussed in the Make #accessibility channel.

Step-by-step reproduction instructions

I believe the only means to test the Font Library: Frontend [Stage 1] work is by checking out PR #53884 as the work has not been merged yet.

Hopefully, the detailed description (above) will guide users on how to recreate this bug along with the video (below).

Screenshots, screen recording, code snippet

https://github.com/WordPress/gutenberg/assets/405912/c3810f8e-1c65-4c86-bae4-30242ccd29ec

Proposed actionable items to address this issue

Environment info

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

colorful-tones commented 1 year ago

I wanted to note that @getdave is doing work on #54296 which may impact this issue, but I have not really peeked at the code or components that are being affected by this. Therefore, I'm not sure if there is overlap with the overall <Modal> component enhancements he is working on, or not. I just wanted to note the work being done to address #54106 could have some interplay here.

I do intend to try and dig deeper, but will likely not be until next week. 😅

t-hamano commented 1 year ago

I think this problem can be solved by using the Navigator components, similar to the implementation in the global style sidebar. This component seems to manage focus well when switching states.

54296 is related to what happens when a Modal component is mounting. I think this issue is not affected by #54296 because this issue is a problem in the content after the Modal component is mounted.

https://github.com/WordPress/gutenberg/assets/54422211/0fffedbb-2915-4c67-b034-e9ac9e6603bf

colorful-tones commented 1 year ago

It would be worth clarifying the expectations here. Perhaps @alexstine @joedolson may have some insights as to what the best experience might be.

Current experience of focus order

Right now, the user opens the modal, which I'm calling the Font Library Overview. They see a list of fonts available in the Font Library Overview. They can tab through each font family item in the list, and choose the enter into that particular font's family, which I'll call the Font Variant Overview. This Font Variant Overview is a nested context within the overall Font Library modal. It lists the chosen Font Family's variants, which could potentially be up to almost a dozen variations: 100, 100 italic, 200, 200 italic, 300, 300 italic, 400, 400 italic, and all the way up to 900.

Right now, when the user enters into the Font Variant Overview nested context screen the first focus goes to the checkbox next to the first Font Variant. This checkbox allows the user to enable or disable that variant. The next focus goes to the next checkbox, and so on.

It is worth noting that the return key does not work on the checkbox, which I believe should allow the user the toggle the state of the checkbox. This may be an additional bug worth fixing.

After going through all the variant checkboxes it goes to the 'Delete' button, which would delete the entire Font Family.

Next focus can go to the 'Update' button, which would save the updates the user made to enable and/or disable particular variants. However, this focus is only necessary when the 'Update' button is enabled. Meaning there have been changes made in variant selection. Otherwise, the focus on the 'Update' button is skipped. Great work there!

Then, it goes to the 'Close' button for the overall modal. Then to the 'Library' tab, which in theory should take the user back to the Font Library Overview, but currently has no actions at all, but still receives focus. Also, likely another bug worth fixing.

Finally, the focus goes to the 'Back' button, which takes the user back to the Font Library Overview area in the modal.

Suggested experience enhancements

  1. The 'Back' button should receive the first focus when entering into the Font Variant Overview. This would allow the user to go back immediately and skip over the variant list altogether.
  2. After the 'Back' button is focused the variant checkboxes will receive focus.
  3. The 'Library' tab should have an actionable 'return' to take the user back to the main Font Library Overview.
matiasbenedetto commented 1 year ago

Thanks for the detailed review. I linked it in the Font Library Stage 3 tracking issue.

carolinan commented 10 months ago

@colorful-tones Hi, even though I am logged in, I am unable to open the slack archive link, the link only sends me to the first page of Slack, can you see if perhaps it is incorrect?

afercia commented 10 months ago

It would be worth clarifying the expectations here.

What I would expect here is to not use a modal dialog for highly dynamic content that gets updated on the fly. As mentioned in other issues an PRs, the Modal component comes with built-in features to manage focus that work okay when the modal content is static but they are fragile when the modal content changes and focusable elements are added / removed / disabled on the fly. See https://github.com/WordPress/gutenberg/issues/57904

That said, I see that once more a relevant new user interfaces has been added without considering deeplu keyboarde accessibility and without actually testing with keyboard.

Right now, when the user enters into the Font Variant Overview nested context screen the first focus goes to the checkbox next to the first Font Variant

What I see on current trunk appears to be slighy different. When pressing Enger on one of the font items, focus doesn't land anywhere and appears to be lost. I wouldn't be surprised there is a full focus loss as the element that had focus (the font item) gets removed from the DOM. Only after pressing the Tab key, focus moves to the first checkbox.

To solve this issue:

It is worth noting that the return key does not work on the checkbox, which I believe should allow the user the toggle the state of the checkbox. This may be an additional bug worth fixing.

Native checkboxes can be checked and unchecked with the Space bar. The Enter key submits the form (assuming there is a form). I don't see a good reason to change the native behavior.

colorful-tones commented 10 months ago

even though I am logged in, I am unable to open the slack archive link, the link only sends me to the first page of Slack, can you see if perhaps it is incorrect?

@carolinan I recommend utilizing Slack's search field, copy and paste the link into the search field and it should come up with the conversation. Sorry for the confusion and please let me know if you're having issues accessing it.

colorful-tones commented 9 months ago

I'm going to go through the latest Font Library progress on trunk and see if/how this is still an issue with focus and will report back.

joedolson commented 9 months ago

Looking at this, I'm in agreement with @afercia that a disclosure pattern would be a clearer, more efficient pattern here. Visually, this has the appearance of an accordion, and I don't see any sound reason that it can't behave like one. If clicking on a font expanded a disclosure with the font variants, then there wouldn't be any need for a focus change; focus would stay on the font name, toggling state to expanded, then the user can navigate forward with the keyboard normally with no change of context.

This reduces mouse clicks for mouse users, as well, so I think it would just be a cleaner, more effective UX.

colorful-tones commented 9 months ago

The issue still persists here.

I wonder if we're looking for something more along these lines (forgive my attempt at design) with the discussion of progressive disclosure pattern? 🤔

Basically, we would likely need to convert using the inner nested TabPanel component usage to utilize the Panel component. I'm sure there is some tricky considerations to make sure there are no regressions for the existing tabs that we want to keep in place though, e.g. "Library", "Upload", "Installed Fonts"

Figma design snapshot of progressive disclosure selection of font in Font Library

Screenshot highlighting view source and inner Font Library tabPanel component

alexstine commented 9 months ago

Disclosure widget sounds nicer over modal here.

andrewhayward commented 9 months ago

I'm also in agreement that a disclosure pattern would be a clearer, more efficient pattern for this, and if we were approaching this afresh would definitely be my recommendation too.

With that said, pragmatically we need to unblock this fonts feature, and ideally get something shipped in 6.5. I don't think the current concept is wrong, even if we generally recognise that it could be improved, so if we're able to iron out the kinks and ensure focus is more logical (and definitely not lost!), it seems like a reasonable first pass.

Ideally we can revisit the implementation after this coming release, with a bit more discussion around how/why we got to this current version, and any points that might impact future iterations. We haven't for example discussed how to delete fonts.

colorful-tones commented 9 months ago

We discussed this a bit with @jasmussen and @t-hamano in Slack today.

It seems that what @t-hamano expressed above is likely worth exploring:

I think this problem can be solved by using the Navigator components, similar to the implementation in the global style sidebar. This component seems to manage focus well when switching states.

Also, it seems that #54296 is not a blocker for the mounted modal.

https://github.com/WordPress/gutenberg/pull/54296 is related to what happens when a Modal component is mounting. I think this issue is not affected by https://github.com/WordPress/gutenberg/pull/54296 because this issue is a problem in the content after the Modal component is mounted.

I attempted to explore integrating the Navigator component, but quickly got in over my head.

I don't think the current concept is wrong

I don't believe anybody is indicating things are wrong, but we're advocating for the Font Library to be useful to as many people as possible. There is certainly a delicate balance between introducing new features and making sure they're ready for the majority of WordPress users.

t-hamano commented 9 months ago

Based on the discussion so far, I think it might be better to proceed as follows. What do you think?

  1. First, we will ship #58794, a PR that replaces infinite scrolling with pagination. This PR contains changes to many components and will serve as the basis for further work moving forward.
  2. When you click on a font, instead of going to the font variations page, display the font variations as accordion content. This will resolve focus loss between fonts and variations. If this could be implemented, there would effectively be no "Back" button in each tab's content, so the refactoring using Navigator components suggested in this comment would be unnecessary.
  3. Resolves focus loss when deleting fonts reported in #58893. Perhaps this requires some kind of focus control.

In parallel with these efforts, we should also proceed with refactoring the font library itself.

andrewhayward commented 9 months ago

I attempted to explore integrating the Navigator component, but quickly got in over my head.

I threw together a very rough-and-ready draft to illustrate how it could be used in this scenario.

I don't think the current concept is wrong

I don't believe anybody is indicating things are wrong, but we're advocating for the Font Library to be useful to as many people as possible.

Yes, sorry, wasn't clear; didn't mean to imply that anyone in particular was taking such a strong stance.

I was trying to suggest that the current implementation does (conceptually) work, even if there's room for improvement, so rather than spend time now going back through the design process we ship what we've got (with appropriate fixes to focus, etc) in time for the next release. We can then spend proper time working on improvements, with everyone's input and involvement.

matiasbenedetto commented 9 months ago

Thanks, everyone, for testing the research and the inputs about this. It is very appreciated.

I don't know if the disclosure pattern will improve the functionality's readability or usability. Given that the list of font variants available can be long (i.e., 20+ variants), could that affect the readability of the entire list of fonts? Could listing font families and font faces in the same list hinder understanding the different nature of each? Where would the UI font-family-specific elements, such as the 'Delete' button, fit in the accordion?

Could removing the current navigation behavior prevent us from adding functionality around font families? Those features could be required soon, requiring extra space and UI elements. This functionality could be, for example, about editing all the font family parameters available in the theme.json schema, uninstalling font faces individually, adding custom demo text, etc.

Considering that these designs https://github.com/WordPress/gutenberg/issues/45271 were proposed a long time ago and have been tested for several months: Is changing them a few days before the beta the safest path to follow?

The solution suggested by @colorful-tones seems like solving the problem without adding new challenges in terms of UI/UX, so I lean to implement that:

Suggested experience enhancements The 'Back' button should receive the first focus when entering into the Font Variant Overview. This would allow the user to go back immediately and skip over the variant list altogether. After the 'Back' button is focused the variant checkboxes will receive focus. The 'Library' tab should have an actionable 'return' to take the user back to the main Font Library Overview.

colorful-tones commented 9 months ago

I see that this issue https://github.com/WordPress/gutenberg/issues/54431 is captured on the Font Library: WordPress 6.5 Required Tasks #55277 under Nice to Have. I propose we leave this issue in the Todo column in the WordPress 6.5 Editor Tasks project in hopes that it'll get enough progress to make it into WP 6.5. However, I do agree that it is a 'Nice to Have' and could be pushed to post-WP-6.5.

colorful-tones commented 9 months ago

Could removing the current navigation behavior prevent us from adding functionality around font families? Those features could be required soon, requiring extra space and UI elements. This functionality could be, for example, about editing all the font family parameters available in the theme.json schema, uninstalling font faces individually, adding custom demo text, etc.

@matiasbenedetto raises some good unknowns here.

Considering that these designs https://github.com/WordPress/gutenberg/issues/45271 were proposed a long time ago and have been tested for several months: Is changing them a few days before the beta the safest path to follow?

Yes, certainly something to consider.

The solution suggested by @colorful-tones seems like solving the problem without adding new challenges in terms of UI/UX, so I lean to implement that:

I agree that getting these adjustments in place for the 6.5 release would allow enable usability, while allowing us to continue to iterate.

I'm going to update the original description to contain actionable items based on this plan. I will be out of office next week, and I suspect that this may not get much attention (being the original issue author). But, if somebody feels like taking a pass at incorporating the following then go for it! ❤

Here is a list of addressable items (also in issue description 👆):

Note: I added the last item based on @afragen insight and unique callout:

Native checkboxes can be checked and unchecked with the Space bar. The Enter key submits the form (assuming there is a form). I don't see a good reason to change the native behavior.

Let me know if I missed anything. Thanks all!

afragen commented 9 months ago

Above was @afercia, not me 😉