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

Discovery: research keyboard handling for grid components #11851

Closed
mlewand opened this issue May 30, 2022 · 8 comments · Fixed by #12202
Closed

Discovery: research keyboard handling for grid components #11851

mlewand opened this issue May 30, 2022 · 8 comments · Fixed by #12202
Assignees
Labels
domain:accessibility This issue reports an accessibility problem. squad:features Issue to be handled by the Features team. type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@mlewand
Copy link
Contributor

mlewand commented May 30, 2022

Provide a description of the task

Research best practices on how to handle grid navigation using the keyboard.

We have multiple grid-like UI components:

To make it clear: this issue is just discovery. It's meant just to figure out best way to implement it and see why we have it implemented the way we did. We also can make some PoC to see how much effort will it require to fix it.

Once we have figured how this should be solved, we can move on to each separate issue.

@mlewand mlewand added type:task This issue reports a chore (non-production change) and other types of "todos". domain:accessibility This issue reports an accessibility problem. squad:features Issue to be handled by the Features team. labels May 30, 2022
@mlewand
Copy link
Contributor Author

mlewand commented Jun 9, 2022

There's a curious thing in how we handle grids as some of them have not predefined columns.

For example, list styles have responsive grid, based only on CSS. However font colors do have predefined columns count.

@mlewand
Copy link
Contributor Author

mlewand commented Jun 9, 2022

As far as the implementation goes we might consider doing it as a helper rather than making this feature a UI component class.

An example of such composition-based approach is click outside handler:

export default function clickOutsideHandler( { emitter, activator, callback, contextElements } ) {

@oleq
Copy link
Member

oleq commented Jul 5, 2022

For the record, it looks like the table insertion grid should handle keyboard differently because when reaching the right boundary of the grid, the right arrow key should not move focus back to the first column (which would be expected in the color grid).

@mmotyczynska mmotyczynska self-assigned this Jul 13, 2022
@CKEditorBot CKEditorBot added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Jul 13, 2022
@mmotyczynska
Copy link
Contributor

mmotyczynska commented Jul 14, 2022

Yet another approach could be using a FocusCycler as it's already partly used in grid-based features. We'd have to enhance it for grids so e.g. the next/previous methods would 'know' that at some point a row of elements ends and we have to go to the beginning/end of the row instead of to the next/previous element. And of course we'd have to add vertical navigation as well for handling arrow up/down events.

We'd have to discuss if there should be one FocusCycler for everything or two FocusCyclers at the same time - one for main navigation and the second one for grid only.

@Reinmar
Copy link
Member

Reinmar commented Jul 15, 2022

BTW: #5772

@mmotyczynska
Copy link
Contributor

Navigation (tab vs arrow keys):

There should be two different navigation handled (they shouldn’t be mixed together):

  • Between sections in the dropdown (tab)
  • Between items of the grid (arrow keys)

In case we decided using 2 focusCyclers, they will have to operate on two different levels - we cannot mix the levels in one focusCycler, e.g. cycling through sections of the view with tab key should be in a different focusCycler than moving between grid items with arrow keys.

The approaches discussed regarding focusCycler:

  1. Extend focusCycler to support 2D navigation
    • Has to be backward compatible: navigation through a vector and through grid elements
    • No 3D navigation: for sections 1D navigation (with tab) and for grid elements 2D navigation (arrow keys)
    • This extension would require one additional parameter: number of columns - the focusCycler should automatically switch to 2D mode
    • This extension would require 2 additional methods (focusUp and focusDown as focusPrevious and focusNext already exist)
    • Actually focusUp and focusDown are not good names, as focusPrevious and focusNext could actually mean right and left as well as up and down in case of 1D vertical dropdown
  2. Create a class inheriting from FocusCycler (e.g. GridFocusCycler) that adds additional constructor parameter and methods for navigation

3 possible solutions (for this ticket):

Actually we came to the conclusion that there are 3 ways of handling this issue:

  1. Create a helper and make no changes in current grids
  2. Create a helper and change current grids so they work the same
    • Seems a good option to have GridView class and a helper inside (and also we could use this helper for inserting tables) - this would be still flexible solution with not so many changes regarding using this API
    • But there is no such class as GridView - and it could be a good moment to create one (but it would take some time)
  3. Create new component for grids and no helper

We agreed that the best option would be 2 (helper + GridView class). 

Notes about the implementation of the GridView class:

  • We have to create a GridView class, very simple, can work as a factory
  • Constructor would get a child constructor (e.g. ButtonView) or there could be a function that could create these children
  • GridView would know how to calculate navigation as there would be information about number of columns
  • GridView would also have a collection of children (can be linear)
  • GridView would also have this new behaviour (helper function that attaches listeners for arrow keys and handles navigation)
  • This way, Colors, ListProperties and SpecialCharacters could use this class and pass a custom callback function for creating children
  • This way probably a lot of code could be removed from current features because now it would be handled in GridView class
  • GridView would add keyboard navigation and maybe also some basic generic CSS for displaying elements as a grid; it would also get a number of columns to pass it to the helper and to use it in inline styles
  • To be checked: number of columns does not change when the browser width changes
  • There should be also a guide for UI regarding how to create grids

Summary:

  • This solution (GridView class + helper) is a preferred one but requires rather a lot of refactoring
  • To be on the safe side we should start with option 1 (creating a behaviour - a helper function) and add it to current features, also change the navigation in focusCyclers so it is consistent for all grids (tab vs arrow keys)
  • But we should create a follow-up tickets for option 2 (creating a GridView class):
    • Refactoring of grids so they use the same parent class
    • FocusCycler and a helper together to be used in GridView class
    • A separate ticket for inserting tables that could also use this solution

Decided:

  • At first we focus on the helper that is added to all four grids
  • Make the navigation consistent (tab vs arrow keys)
  • Finish the poc so it fully works (tests not needed).

@CKEditorBot CKEditorBot added status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Jul 25, 2022
@mmotyczynska
Copy link
Contributor

A follow-up issue for keyboard navigation in color picker in table properties: #12193

@CKEditorBot CKEditorBot added status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. and removed status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. labels Aug 1, 2022
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Aug 2, 2022
@mlewand

This comment was marked as outdated.

oleq added a commit that referenced this issue Aug 12, 2022
…r-grid-components-discovery-poc

Feature (ui): Introduced the `addKeyboardHandlingForGrid()` helper to handle grid keyboard navigation. Closes #11851.

Fix (font): Fixed focus order for color grid in font color and background drop-downs. Closes #11841.

Fix (list): Unified focus handling and keyboard navigation in list properties drop-downs. Closes #11041.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Aug 12, 2022
@mlewand mlewand added this to the iteration 56 milestone Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:accessibility This issue reports an accessibility problem. squad:features Issue to be handled by the Features team. type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants