Opened 21 months ago

Closed 20 months ago

Last modified 18 months ago

#16804 closed Bug (fixed)

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

Reported by: Satya Minnekanti Owned by: Tade0
Priority: Nice to have (we want to work on it) Milestone: CKEditor 4.7.0
Component: Accessibility Version: 4.7.0
Keywords: irina Cc: IBM

Description

Steps to reproduce

  1. Open nightly build http://nightly.ckeditor.com/17-01-18-07-08/full/samples/
  1. 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)

Change History (26)

comment:1 Changed 21 months ago by 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

comment:2 Changed 21 months ago by 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.

comment:3 Changed 21 months ago by Marek Lewandowski

Priority: NormalNice to have (we want to work on it)
Status: newconfirmed

comment:4 Changed 21 months ago by Marek Lewandowski

Milestone: CKEditor 4.7.0

comment:5 Changed 21 months ago by 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.

comment:6 Changed 21 months ago by Tade0

Owner: set to Tade0
Status: confirmedreview

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.

comment:7 Changed 21 months ago by Tomasz Jakut

Status: reviewreview_failed

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.

comment:8 Changed 21 months ago by Tomasz Jakut

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

comment:9 in reply to:  7 Changed 21 months ago by 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.

comment:10 Changed 21 months ago by 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.

comment:11 in reply to:  10 Changed 21 months ago by 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.

comment:12 Changed 21 months ago by 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.

comment:13 Changed 21 months ago by Tade0

Status: review_failedreview

Fixed the remaining issue, rebased with major.

Changes pushed to branch:t/16804.

comment:14 Changed 21 months ago by Tomasz Jakut

Status: reviewreview_failed

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.

comment:15 Changed 21 months ago by Tade0

Status: review_failedreview

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.

comment:16 Changed 21 months ago by Tomasz Jakut

Status: reviewreview_failed

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.

comment:17 in reply to:  16 Changed 21 months ago by 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.

comment:18 Changed 20 months ago by 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.

comment:19 Changed 20 months ago by Tade0

Status: review_failedreview

Improved color selection, refactored a few parts.

Changes pushed to branch:t/16804.

comment:20 Changed 20 months ago by Tomasz Jakut

Status: reviewreview_failed

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?

comment:21 in reply to:  20 Changed 20 months ago by Tade0

Status: review_failedreview

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.

comment:22 Changed 20 months ago by Tomasz Jakut

Status: reviewreview_failed

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.

comment:23 Changed 20 months ago by Tade0

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

comment:24 Changed 20 months ago by Tade0

Status: review_failedreview

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.

comment:25 Changed 20 months ago by Tomasz Jakut

Resolution: fixed
Status: reviewclosed

LGTM. Merged with git:61a6108.

comment:26 Changed 18 months ago by Marek Lewandowski

Component: GeneralAccessibility
Summary: Accessibility: Focus should be on first menu item when user opens Context MenuFocus should be on first menu item when user opens Context Menu
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy