Implemented focusing first item after opening a dropdown

[mlewand]

Suggested merge commit message (convention)

Fix (ui): Opening a dropdown should focus the first item (or the first active item) for easier navigation and accessibility. Closes #11838. Closes #2000. Closes #11568. Closes #3215. Closes #5859.

MINOR BREAKING CHANGE (ui): Until now, preventDefault() was called on the mousedown event to prevent the ButtonView from stealing the focus from the editing root. Starting from this editor version, to improve the accessibility of the editor, all instances of the ButtonView class (and child classes) will automatically focus in DOM when clicked. It is recommended that features that use buttons to manage the content call editor.editing.view.focus() after the button was executed in order to bring the DOM focus back to the editing root to allow users type right away. 

---

Additional information

To sum up it does two things:

• Brings the focus for the first dropdown item for generic dropdown buttons.
• Focuses the active item in list dropdowns (relying on isOn property).

Reason why it closes so many extra issues is that it forces focus to go into dropdown items when buttons are open thus, for example, pressing esc works as expected now.

[mlewand]

@mmotyczynska based on your branch I added behavior for toolbar dropdowns and to prioritize focusing the active option if possible.

There's some failing tests that probably were written on a little bit more integration rather than unit test side. So this needs to be addressed.

Furthermore, there's insufficient code coverage so that needs to be fixed too.

Finally, there are 2 known uncovered cases which should be extracted to a followup:

• list style dropdown is not affected by this change (because it's implemented in a much custom way) - it needs a follow up issue
• font color (and font bg color) dropdown - also requires a followup

[mlewand]

Also, @ckeditor/qa-team can you test CF UIs with this change? To make sure that it doesn't break anything (less likely) and still make sense.

[dufipl]

cc @mlewand @mmotyczynska

Issue with selection outside of the editor after accepting/discarding suggestions in TC

Steps

1. Open http://localhost:8125/ckeditor5/external/collaboration-features/packages/ckeditor5-revision-history/tests/manual/revision-history-autosave.html
2. Enable Track Changes
3. Create a suggestion
4. Click on the TC dropdown and choose "Accept all suggestions"

Expected result:

Focus gets back to the editor, the user can write instantly, ctrl+z is possible without manually focusing on the editor:

Actual result:

Focus does not get back to the editor

Additional details

It is a regression compared to the master branch.
Issue reproduce on Chrome and Firefox. However, it does not reproduce on Safari.

[dufipl]

cc @mlewand @mmotyczynska simpler case:

Issue with selection outside of the editor after enabling TC

Steps

1. Open http://localhost:8125/ckeditor5/external/collaboration-features/packages/ckeditor5-revision-history/tests/manual/revision-history-autosave.html
2. Move focus to the editable area.
3. Enable Track Changes

Expected result:

Focus inside of the editor, the user can type after enablement.

Actual result:

Focus does not get back to the editor.
Focus moved to the toolbar element:

Additional details

It is a regression compared to the master branch.
Issue reproduce on Chrome and Firefox. However, it does not reproduce on Safari.

[mmotyczynska]

Failing tests have been addressed.

Code coverage fixed too.

As agreed I also changed the default behaviour of addToolbarToDropdown. It does not focus the first active element unless it is set as true in options param.

And one more change as agreed yesterday for Special Characters feature: when the dropdown is opened the focus is switched to character categories dropdown.

[oleq]

I think we're getting close with this task 🙂

I noticed you didn't take advantage of the new addToolbarToDropdown() API, though. I decided to give it a try. I wasn't sure if this was the right way thing to do so sorry for the poor communication.

Upon closer inspection I unified the focus-on-open logic under a helper that gets used internally in addToolbarToDropdown() and addListToDropdown() as well as in some features that need a custom handling (list style and font color). You can see how it looks in a fork of this PR. Feel free to merge it here if you find it acceptable (it will need tests, though).

Things left to do:

• I noticed a glitch when clicking one dropdown of the balloon toolbar and then another. The latter won't open and the toolbar hides. This must be addressed. My gut tells me this could be due to the fact that until this PR, there was no way to focus anything in this kind of toolbar. It could be that something was never registered in the focus tracker because if you attach this listener you'll find that the entire editor gets blurred in the process

editor.ui.focusTracker.on( 'change:isFocused', () =>{
    console.log( editor.ui.focusTracker.isFocused );
});

Update: I suppose the problem is somewhere else.

Add the following listeners:

window.addEventListener('focus', () => { console.log( 'activeElement', document.activeElement ) }, true)
window.addEventListener('blur', () => { console.log( 'activeElement', document.activeElement ) }, true)

and the following style:

*:focus {
    outline: 3px dashed red !important;
    outline-offset: -1px !important;
}

and see what it looks like on master and then in this PR. The thing is that on master, the focus always stays in the root as you click across the UI (e.g. open one dropdown and then another). In this PR however, if you already got one dropdown open it takes the focus, and when you click another, the click outside handler logic kicks in

ckeditor5/packages/ckeditor5-ui/src/dropdown/utils.js

Lines 275 to 286 in 1f40c3e

	function closeDropdownOnBlur( dropdownView ) {
	dropdownView.on( 'render', () => {
	clickOutsideHandler( {
	emitter: dropdownView,
	activator: () => dropdownView.isOpen,
	callback: () => {
	dropdownView.isOpen = false;
	},
	contextElements: [ dropdownView.element ]
	} );
	} );
	}

To put it shortly, this logic is responsible for closing the dropdown when somebody click outside of it (on mousedown). When the dropdown is open, it holds the focus of the entire editor. When you click on another dropdown (hold the button down to see this better), this logic kicks in and the dropdown closes. The focal point of the entire editor gets hidden as the panel goes off the screen and the browser, seeing that the thing that used to be focused (a button) is no longer visible, moves the focus to <body>. editor.ui.focusTracker seeing the focus went to <body> thinks the editor was blurred and all sorts of UI (including all balloons) hide upon that. That's why you can't click between dropdowns in the widget toolbar (or any ballon for what is worth) but it kinda works in the classic editor toolbar (although notice that the editor.focusTracker.isFocused goes in and out and the editing root's outline is blinking).

I'm not sure how to address this yet, though. Maybe we should move the focus somewhere else before dropdownView.isOpen = false;?

diff --git a/packages/ckeditor5-ui/src/dropdown/utils.js b/packages/ckeditor5-ui/src/dropdown/utils.js
index 78b80d6442..1ace5146bc 100644
--- a/packages/ckeditor5-ui/src/dropdown/utils.js
+++ b/packages/ckeditor5-ui/src/dropdown/utils.js
@@ -278,6 +278,7 @@ function closeDropdownOnBlur( dropdownView ) {
                            emitter: dropdownView,
                            activator: () => dropdownView.isOpen,
                            callback: () => {
+                                    dropdownView.buttonView.focus();
                                     dropdownView.isOpen = false;
                            },
                            contextElements: [ dropdownView.element ]

seems to help but it looks weird. I think it would we better if we allowed all toolbar buttons to be focusable on click (ATM we prevent that). There would be no interruption in the focus then. Yeah, removing this

ckeditor5/packages/ckeditor5-ui/src/button/buttonview.js

Lines 154 to 156 in 30286f7

	mousedown: bind.to( evt => {
	evt.preventDefault();
	} ),

helps. What was it for? See #2000 (comment) to learn more.

• (cc @Reinmar) I'm worried about integrators and their View instances that do not come with the focus() method. When in the DropdownView we call this.panelView.focus(); and the panel does this.children.first.focus(); we assume that the first child has the focus() method. If not, there will be a JS crash. And because this will happen upon opening of every dropdown (lots of points of failure), I see two approaches:
  1. Implement View#focus() in view.js that will logWarning() saying that ~"We were about to focus this view but it looks like it doesn't have the focus() method implementation.". No crash will follow. We'll have to disable the warning in case someone calls super() in their focus() implementation but I suppose something like if (this.focus === View.prototype.focus) {} should do the trick.
  2. Check if this.children.first.focus in the DropdownPanelView and childToFocus.focus in focusChildOnDropdownOpen() exist. It's cheaper and easier but it secures the dropdown ecosystem only. If we progress with the a11y we may find out there are more systems that require smart automatic focus movement and other views will throw because they don't have the focus() interface.

[oleq]

An update regarding 

I noticed a glitch when clicking one dropdown of the balloon toolbar and then another. [...]

Together with @Reinmar we decided to ditch

ckeditor5/packages/ckeditor5-ui/src/button/buttonview.js

Lines 154 to 156 in 30286f7

	mousedown: bind.to( evt => {
	evt.preventDefault();
	} ),

and make all buttons focusable. Let's see how it works during QA session (known issue: blinking outline of nested editables because they're not connected to the EditorUI#focusTracker).

---

An update regarding

I'm worried about integrators and their View instances that do not come with the focus() method

Together with @Reinmar we decided to have a check in DropdownPanelView#focus() to make sure the first child has the focus() method. If not, don't do anything but logWarning() telling the integrator that their view instance is missing some API. 

We'll come up with a more systemic solution when the migration to TypeScript is over. A FocusableView interface would make things a lot easier.

[oleq]

I added some docs and I did a minor code refactoring here and there. There are still some changes missing tests, though:

---

@ckeditor/qa-team Could you please take a look at this PR? The scope is as follows:

• Whenever a dropdown (any dropdown except maybe table insertion dropdown, there's a dedicated ticket for that) is open, the focus should move somewhere into its panel.
• When a dropdown contains a list (e.g. headings) or a grid (font color), etc., the focus should move to the active item straight away, depending on the selection in the editing root.
• Note: It does not matter whether the dropdown was open by keyboard or by clicking the face of the button, it should work all the same
• Note: To make this happen we made every single button (also: dropdown) in the editor get focused when clicked. This is an a11y standard across the industry but this could also have an impact on all UI related features (also: collaboration features). Please verify that the entire UI of the editor acts predictably and the focus is never lost 🙏 We don't want the editor to get blurred.
• Note: Like I said, ignore the table insertion dropdown for now. It's one of a kind 🙃.

[dufipl]

@oleq @mmotyczynska Generally it looks like working fine, I have however two comments:

1. TC enabling still (Implemented focusing first item after opening a dropdown #12003 (comment)) does not bring the focus back to the editor as it does before that change. From the UI perspective button looks the same as the highlight which brings the focus back to the editor

2. Bullet list styling does not allow to navigate through potential styles using the keyboard after pressing the dropdown:

even if the focus is being kept. I'm not sure if that is in the scope of this task, but this dropdown looks similar to others which offer such a possibility.

Besides that, I have not found any significant issues 👍

[Reinmar]

1. C enabling still (Implemented focusing first item after opening a dropdown #12003 (comment)) does not bring the focus back to the editor as it does before that change. From the UI perspective button looks the same as the highlight which brings the focus back to the editor

Followup ticket for TC. It's ok to extract it from the scope of the current work in this PR.

1. Bullet list styling does not allow to navigate through potential styles using the keyboard after pressing the dropdown:

Separate topic: #11851

[Reinmar]

@oleq Am I right that this PR is ready to merge?

[Reinmar]

@oleq, sorry, to merge this we need the minor BC description regarding removing the preventDefault() in button's mousedown listener. 

I think this should be accompanied with a recommendation to use editor.editing.view.focus() (or whathever's correct) in the execute() if needed.

It sounds like the scenario in the TC plugin that @dufipl mentioned falls to this category.

[Reinmar]

Aren't these two tickets reasolved as well?

• The selected language in code block dropdown is not visible #5859
• Improve accessibility of drop–downs #1037

[oleq]

Aren't these two tickets reasolved as well?

• The selected language in code block dropdown is not visible #5859
• Improve accessibility of drop–downs #1037

The first one 👍 As for the second one, I think it's more than this. For instance

Exiting the drop–down should also be explained by the text reader.

was not covered by this PR. I think we should refine this issue first to understand what is done from that list and what is left and what is really necessary.

[dufipl]

@Reinmar created a follow-up ticket for the TC issue: https://github.com/cksource/collaboration-features/issues/4668
cc @scofalik