Redesign on state in menu and dropdowns

[Mati365]

Suggested merge commit message (convention)

Fix (ui): Improve accessibility of menu bar toggle items by marking them as menucheckbox aria roles.
Feature (ui): Redesign of menu items styling.

Closes #16572.

Additional information

Commercial PR: https://github.com/cksource/ckeditor5-commercial/pull/6339

Before

After

[scofalik]

Below are conclusions from manual testing I did (some relate to commercial features PR):

1. Following menu items are not getting "on" state: Link, Block quote, Comments archive, Accessibility dialog, Find and replace. Additionally, AI Assistant gets incorrect styling (it is all grayed, instead of getting a checkbox). Finally, Comments archive doesn't get "on" state in toolbar either -- it's not directly related to this ticket but we could fix it as it's the same part of the code.
2. Switch toggle menu items have incorrect styling. When they are on, they are blue. See track changes toolbar dropdown.
3. Focus do not follow mouse hovering on the top-level menu items ("Insert", "View", etc.) IF the keyboard interaction was when the menu was not open. To test this focus menu using Alt+F9, then click right arrow, then hover other items.
4. In revision history, in sidebar, for a selected revision (blue), if open the revision menu (click tripe-dot) the items inside have white font color.
5. Format -> Highlight -> options, when one of them is selected it "loses its colors".

Some other thoughts that are not directly related to this PR and can be done as a follow-up:

1. In toolbar dropdowns, focus is not following the mouse hovers. I guess this one can be solved together / after "new dropdowns" PR.
2. The mockups for the "checked menu items" has not only changes related to colors and checkboxes but also to paddings AFAIR. We should also introduce these changes at some point, but we can do it separately.
3. "None" option in border dropdown in table properties and table cell properties components is not "on" when it is chosen. This is not a regression.
4. Since you can now close a tooltip using ESC, you need double ESC to close Format -> Bulleted List -> dropdown. If you wait a while, the first ESC will close the tooltip (e.g. "Circle" tooltip), then only the second ESC will close the dropdown. I found it counterintuitive and unexpected (cc @oleq).
5. Icons in lists dropdowns do not get focus when hovered with mouse (cc @oleq).

[Mati365]

@scofalik Thx! Will be fixed.

[scofalik]

One more thing @Comandeer @oleq.

When I click the top-level menu item (like "Insert") it gets focus and the focus will follow the mouse hover. I wonder about this behavior. On one hand, it seems correct to show the focus when I clicked the top-level menu item. On the other hand, it is weird that the focus follows the mouse. But it would be also weird if it didn't. So I wonder if we have to show the focus on click and if we can hide it when hovering other items. It is also inconsistent with hovering items inside such menu. E.g. if I hover&click "Insert" it shows the focus but then I hover down and the focus is not shown anymore. But if I hover left&right, the focus is kept. See video.

Screen.Recording.2024-06-24.at.16.19.27.mov

[scofalik]

Other than what I commented, please try to use properties and methods name as short as possible. Maybe you'll find some room for improvement. I also dislike maybeXXX approach. I get your point, but that's not really necessary if we have typed code and if the method is very short or you immediately early return like this:

const listView = pickListView();

if ( !listView ) {
    return;
}

[Mati365]

@scofalik

Other than what I commented, please try to use properties and methods name as short as possible. Maybe you'll find some room for improvement.

I tend to use the most descriptive names as I can, even if they are longer, so we can read them as a recipe step that shortly describes what the method does without looking at docs or types. Although it's my personal opinion, I'm perfectly fine with renaming. Can you point me which methods and variables should be renamed (and what is a preferable way to name them?). It'd helpful and save my time.

I also dislike maybeXXX approach. I get your point, but that's not really necessary if we have typed code and if the method is very short or you immediately early return like this:

Sorry, I forgot about our discussions about these maybe prefixes, and I renamed these maybeXXX variables. I used to do them while using optional monodic result of functions to quickly determine without looking at the type if type might be nullable.

[Comandeer]

Replying to #16573 (comment):

"Focus visible" SC requires focus only during keyboard operations. I would argue that it allows us to skip showing the focus indicator if the user hovers over a top-level menu item with a mouse. Skipping the indicator there would make the whole experience consistent.

So:

1. If the user operates a top-level menu item with a mouse, do not add focus indicator.
2. As soon as the user starts using keyboard, show the focus indicator.

[scofalik]

Sorry, I forgot about our discussions about these maybe prefixes, and I renamed these maybeXXX variables. I used to do them while using optional monodic result of functions to quickly determine without looking at the type if type might be nullable.

👍

I tend to use the most descriptive names as I can, even if they are longer, so we can read them as a recipe step that shortly describes what the method does without looking at docs or types.

Of course, this is a good rule to follow. But sometimes these names get a bit too long and reading them line after line is tiring. Sometimes they do not fit convention. Some things I'd think about from this PR are:

• Does it have to be CheckHolder or maybe Check is enough?
• _shouldRenderCheckHolder -> _hasCheck
• hasReservedCheckHolderSpace -> hasCheckSpace or requiresCheckSpace or addCheckSpace

Do you have to change these names? Honestly, these ones are deep and used slightly, so it isn't a critical thing. But I think you have slight tendency to overdescribe the names, so you can treat it as a general advice.

[Mati365]

@scofalik Replaced 👍🏻

[scofalik]

One more example: _preallocateCheckSpacerInItemsIfNeeded. I am not a big fan of this name too 😄. First, of course we do this only if needed, we don't do stuff if it is not needed. If you feel it is important to explain what exactly happens, we can add proper comment in API doc. I'd probably go with something like _setItemsCheckSpace.

[scofalik]

Almost there. One bug left.

[oleq]

• Since you can now close a tooltip using ESC, you need double ESC to close Format -> Bulleted List -> dropdown. If you wait a while, the first ESC will close the tooltip (e.g. "Circle" tooltip), then only the second ESC will close the dropdown. I found it counterintuitive and unexpected (cc @oleq).

Since this is the only place that acts this way, I wouldn't be worried. There are pros and cons of this approach.

5. Icons in lists dropdowns do not get focus when hovered with mouse (cc @oleq).

AFAIR they are custom-made and don't share the same logic that makes up the menu bar. I don't know how to address this issue yet, though. Also, I suppose the same goes with in-toolbar dropdowns in general because they are different UI components and they are missing this mouse-to-keyboard logic.

[Mati365]

Since you can now close a tooltip using ESC, you need double ESC to close Format -> Bulleted List -> dropdown. If you wait a while, the first ESC will close the tooltip (e.g. "Circle" tooltip), then only the second ESC will close the dropdown. I found it counterintuitive and unexpected

@scofalik imo, it's correct behavior. There could be situations where you focus an item using keyboard, like in multi-level lists, then tooltip shows and it overlaps another items. To get of rid of them, without closing dropdown, ESC should be used.

AFAIR they are custom-made and don't share the same logic that makes up the menu bar. I don't know how to address this issue yet, though. Also, I suppose the same goes with in-toolbar dropdowns in general because they are different UI components and they are missing this mouse-to-keyboard logic.

@oleq I created a separated issue about that: #16612

[scofalik]

Regarding tooltips: sometimes it is correct, sometimes not. I understand why we introduced closing tooltips on ESC and I don't discuss it - I agree that was an improvement. But in this case it was a UX degradation from my point of view and how I used it, I noticed it and wrote about it. There's probably no way to have both, so I am fine with how it is.

It wouldn't be a problem if the list components were better integrated with the menu bar, i.e., if I was able to leave the list types grid by pressing arrow left or arrow up when in left column / top row.