Focus should be on first menu item when user opens Context Menu

[Satya Minnekanti]

Steps to reproduce

1. Open nightly build ​http://nightly.ckeditor.com/17-01-18-07-08/full/samples/

2. Insert a Table. Keep cursor inside the table & press CTRL + SHIFT + F10 or Application key to open Context menu

Expected result

Context Menu opens & focus is on first menu item ( pls see Aria Authoring practices for menu here :[ ​https://www.w3.org/TR/wai-aria-practices/#menu])

Actual result : Context Menu opens but focus is not on menu item & user has to press Down arrow key to get focus on the menu. This is violation of Accessibility Checkpoint 4.1.2 Name, Role, Value)

[Satya Minnekanti]

Same issue happens when we open Colour Palette in Text or Background Colour. Focus should go to first or selected option in colour palette

[Marek Lewandowski]

Confirmed. I'm surprised to see that all other applications that I have tested use the approach that we're doing right now. Even though spec specifies what is said in Expect result section.

[Marek Lewandowski]

It's important to keep in mind that it should apply to context menu, and dropdowns from toolbar menu buttons (e.g. language plugin, scayt), where you could press down arrow to unfold the menu.

[Tade0]

Created a general solution (in the panel plugin) at the same time removing all the previous attempts to do the same(dating back to 2013).

Changes pushed to ​branch:t/16804.

[Tomasz Jakut]

Although the proposed solution is based on ARIA's best practices, I'm not sure if it's better thanks to that…

In our current solution if user opens some combobox (e.g. "Styles"), then the screenreader announces the name of the opened combobox and the selected element. In the proposed solution this information is ommited and the screenreader announces only the value of the first item in the opened combobox. Tested with VoiceOver.

The solution is working pretty well for context menus, but for richer UI's elements it doesn't convince me.

From the other things:

• it will be nice to have some unit test which checks if the first element is really focused.

[Tomasz Jakut]

Mind also that our rich combos are a distinctive widgets, listbox, which have ​their own best practices.

[Tade0]

Replying to t.jakut:

Although the proposed solution is based on ARIA's best practices, I'm not sure if it's better thanks to that…

In our current solution if user opens some combobox (e.g. "Styles"), then the screenreader announces the name of the opened combobox and the selected element. In the proposed solution this information is ommited and the screenreader announces only the value of the first item in the opened combobox. Tested with VoiceOver.

Tested this in JAWS and the name of the combobox is announced.

[Tomasz Jakut]

Yes, the name of the combobox is announced in JAWS, but the selected element is not announced. I'm not sure, but it's probably connected with the fact how the combobox currently works: the selection is strictly connected with focus, so moving focus inside the list means also moving selection.

Inside guidelines dedicated to listbox we could read that moving selection with focus is optional and this case is probably the best example why.

[Tade0]

Replying to t.jakut:

Yes, the name of the combobox is announced in JAWS, but the selected element is not announced. I'm not sure, but it's probably connected with the fact how the combobox currently works: the selection is strictly connected with focus, so moving focus inside the list means also moving selection.

Ok, I see the problem now. I think it's more connected with the crudeness of my implementation.

[Tade0]

Changed the focusing behaviour so that now the selection style takes priority.

Updated manual test.

There's one issue I noticed in the meantime: pressing the arrow key at first does nothing.

[Tade0]

Fixed the remaining issue, rebased with major.

Changes pushed to ​branch:t/16804.

[Tomasz Jakut]

The original issue is about "context menu", but there is no mention of it inside manual test (TBH context menu is not working there at all).

I'm also wondering if unit tests are simple to write in this case. They should be – as I understand from the code the whole test would just check one property of the context menu/listbox.

[Tade0]

Moved and expanded the manual tests.

Added also unit tests. They may look a little unorthodox, but the point was to test the code of the panel plugin, not its numerous sub-classes.

[Tomasz Jakut]

I've found one more issue: in case of color listbox the first option is always selected, even if color was already set.

I've also update manual test name and rebased ​branch:t/16804 to the latest major.

[Tade0]

Replying to t.jakut:

I've found one more issue: in case of color listbox the first option is always selected, even if color was already set.

Interestingly, the former behaviour was "don't select anything at all".

I'll need to implement selecting the appropriate color regardless of accessibility to continue.

[Tade0]

I have a rudimentary implementation of color selection but it needs more work, because for now if you select elements of multiple colors it will simply select one of them - not acceptable.

[Tade0]

Improved color selection, refactored a few parts.

Changes pushed to ​branch:t/16804.

[Tomasz Jakut]

There's a small issue in Internet Explorer 8 with color dialog. If it's opened for the first time, focus indicator is not shown. Everything else seems fine.

The only thing that bugs me in the code is ​refactoring of loops. Don't you think that it's odd to have loops with incrementation nearly everywhere in our code and suddenly one with decrementation?

[Tade0]

Replying to t.jakut:

There's a small issue in Internet Explorer 8 with color dialog. If it's opened for the first time, focus indicator is not shown. Everything else seems fine.

I don't seem to be able to reproduce this one. Could you describe how exactly you produce this error?

The only thing that bugs me in the code is ​refactoring of loops. Don't you think that it's odd to have loops with incrementation nearly everywhere in our code and suddenly one with decrementation?

This part is really a reduceRight with short-circuiting, but we don't have this implemented(yet), so I left it that way.

Note how reversing the loop allows to get rid of that one additional check.

[Tomasz Jakut]

I'm still able to reproduce issue in IE8 in manual test.

1. Open ​http://tests.ckeditor.dev:1030/tests/plugins/panel/manual/tollbara11y
2. Open color pallete.

The focus indicator is not visible.

[Tade0]

The key to reproducing this is creating a window so small that the color panel can't be in its default position.

[Tade0]

Additionally the browser needs to be restarted each time.

I guess the problem might be connected with focusing items in a panel that was just created synchronously.

Changed the call that marks the first displayed item to asynchronous in IE.

Updated unit tests.

Changes pushed to ​branch:t/16804.

[Tomasz Jakut]

LGTM. Merged with ​git:61a6108.