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

Enabled toolbar focusing and cycling across all editors and features #12064

Merged
merged 83 commits into from
Aug 22, 2022

Conversation

oleq
Copy link
Member

@oleq oleq commented Jul 14, 2022

Suggested merge commit message (convention)

Feature (core): Enabled toolbar focusing (and cycling) across all editor implementations and features. Closes #10368. Closes #5146. Closes #9906. Closes #10025.

BREAKING CHANGE (ui): The enableToolbarKeyboardFocus() helper has been removed. Please use the EditorUI#addToolbar() method instead to enable accessible toolbar navigation (and focusing) using Alt+F10 and Esc keystrokes (see #10368).


Additional information

TODOs

  • Tests:
    • At least two tests for every editor type:
      • one to make sure Alt+F10 works for simple navigation (root->main toolbar->back to root),
      • another to make sure this works when the selection was on an image and the image toolbar was visible (e.g. image->image toolbar->main toolbar->image toolbar->back to the image).
    • Tests for changes in EditorUI
      • Tests for toolbars with different priorities (selected widget's toolbar > global toolbar, balloon toolbar > block toolbar, etc.)
    • Tests for the Alt+F10 in source editing textarea.
    • Tests for WidgetToolbarRepository to make sure it registers things in EditorUI
    • Same for BalloonToolbar.
  • Code refactoring (I left lots of comments in the code):
    • There's a risk of infinite loop in EditorUI#focusNextFocusableToolbarDefinition (see the code to learn more)
    • Helper functions in EditorUI#_initFocusTracking should probably become private methods of EditorUI.
    • Remove unnecessary comments.
    • EditorUI#registerFocusableEditingArea() has an obsolete support of UI views that we probably don't need at this stage.
    • Remove console.logs in EditorUI
  • QA
    • I didn't do anything to the multi root guide (example). I didn't check how it works but this is the right time to figure it out.
  • Docs:
    • API docs for the new code.
    • I suppose we should update some guides (which guides?) about developing new editors with the new EditorUI API.
    • There's a focus tracking guide. Maybe we should mention some (which?) of the changes there?
    • Migration guide (removal of enableToolbarKeyboardFocus() and new API).
  • TBD warning in enableToolbarKeyboardFocus() pointing to the new EditorUI API.
    • Decision: The helper is removed.

Follow-ups

  • I don't think this is the right place to do this but I think we should definitely put "Press Alt+F10 to focus the toolbar"/"Press Alt+F10 to focus the next toolbar" in aria-labels here and there. Alt+F10 does not look like an industry standard to me. Right now, the user does not get any information about a toolbar being displayed and how to access it. This should be confirmed but:
    • All root editables should have these labels.
    • All widgets that have toolbars registered in WidgetToolbarRepository should have these labels (image, table, etc.).
      • All widgets with nested editables that keep the widget toolbar visible should have labels (e.g. image caption).
    • Source editing textarea should have the label.
    • All toolbars should have a label explaining how to move to another toolbar.
  • My gut tells me we should use FocusCycler to move around the UI (cycling the toolbars) but I didn't do this in this PR.  I suppose FocusCycler#_getFocusableItem() is too simple to make decisions I implemented in this PR (toolbars have priorities and deciding which to focus is a lot harder). But I suppose we could unify everything down the road.
  • There's an optional follow-up about re-writing some pieces of the SourceEditing that I first decided to handle here but then I figured things were already getting complex so I extracted the code to a separate PR. Long story short, this is about SourceEditing using ckeditor5-ui to bring its UI (textarea) and reduce internal redundancy.

@oleq oleq marked this pull request as ready for review August 9, 2022 12:27
@oleq
Copy link
Member Author

oleq commented Aug 10, 2022

Use-case to fix: when pressing Alt+F10 in a toolbar when there's no other toolbar to focus:

  • nothing should change (expected)
  • focus moves to the first item in the dropdown (actual)

@oleq
Copy link
Member Author

oleq commented Aug 10, 2022

Use-case to fix (low priority): when pressing Alt+F10 in a toolbar when some dropdown is open and the focus moves to another toolbar, the dropdown should close.

@Acrophost
Copy link

Use-case to fix: when pressing ALT+F10 more than once in Balloon Editor when only one toolbar is available to focus:

  • focus moves to balloon toolbar and stays there for next ALT+F10 presses (expected)
  • focus moves to balloon toolbar and on next ALT+F10 goes missing (actual)
balloon.editor.focus.lost.2022-08-16.at.14.17.19.mov

@Acrophost
Copy link

Acrophost commented Aug 16, 2022

Another use-case to fix: pressing ALT+F10 on an image sitting in a table:

  • focus toggles between regular toolbar and image toolbar (expected)
  • focus jumps from image toolbar to table toolbar and after that toggles between regular toolbar and table toolbar (actual)

Videos:

  • Balloon block editor:
balloon.block.editor.2022-08-16.at.15.14.15.mov
  • Classic editor - select whole table and then image before ALT+F10 (can appear without these steps by adding extra balloon toolbar):
classic.editor.2022-08-16.at.15.33.10.mov

@oleq
Copy link
Member Author

oleq commented Aug 19, 2022

but it will fail when the table cell content was focused first before an image:

@Acrophost Could test the toolbars again to verify if my recent fixes do not introduce new regressions? Thanks.

@Acrophost
Copy link

@oleq everything works much better and I don't see any more issues besides the one you have already mentioned :)

@oleq oleq merged commit 1499585 into master Aug 22, 2022
@oleq oleq deleted the ck/10368-source-editing-focus-toolbar-v2 branch August 22, 2022 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants