Enhance accessibility of autocomplete

[Comandeer]

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches that change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

• Unit tests
• Manual tests

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

• PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#4617](https://github.com/ckeditor/ckeditor4/issues/4617): Fixed: [Autocomplete](https://ckeditor.com/cke4/addon/autocomplete) is not accessible in inline editors.

What changes did you make?

I've added appropriate attributes to the editor's editable ([aria-autocomplete="list"], [aria-controls], [aria-activedescendant], [aria-expanded]) and to the autocomplete and its items (mainly roles – listbox for the autocomplete itself and option for its items). Thanks to that screen readers far better support the autocomplete:

• NVDA announces the presence of the autocomplete on editor focus and reads options from the listbox,
• JAWS announces the presence of the autocomplete on editor focus if the editor is empty (it's probably connected with the default verbosity settings) and reads options from the listbox,
• VoiceOver does not announce the presence of the autocomplete, however it reads options from the listbox.

I've also ensured that these changes aren't applied to iframe-based editors.

Which issues does your PR resolve?

Closes #4617.
Closes #4653.

[sculpt0r]

Rebase on newest master.

[sculpt0r]

@Comandeer

I added a few inline comments, but not finish the entire review since we have red CI for a11y tests (https://travis-ci.com/github/ckeditor/ckeditor4/jobs/501340888). I re-run it once for both Chrome builds with the same result.

Only the second Chrome build is red. I run a11y manually on my Chrome & FF (also on BS Chrome & FF) and it looks good 🤔 Also that is a case with unexpected errors in CI logs (#4620).

Could you take a look at a11y tests?

[Comandeer]

I've updated methods' names.

As for tests: they work for me 🤔 I don't know why they are failing on CI.

[sculpt0r]

Thank you for the previous changes.

I made some research in a11y unit tests and here are my results:

• first of all, those tests are not independent. If one fails, then another may also fail. I found it when I try to add assertion with spy on the method which updates aria attributes on editable. I put it on a separate branch based on yours: 9878696 The spy assertion fails on my PC, but it causes another test to fail... You can verify it even by adding a simple assert.isTrue( false );. Finally, I realized that if an assertion fails, then autoComplete.destroy(); is not invoked... So I made a fast modification here: 82d5147 and it leads to only two failing tests on CI (was four before): https://travis-ci.com/github/ckeditor/ckeditor4/jobs/501698276

Could you make a little deeper digging with those two? Also please make sure, that created autoComplete will be destroyed after each test. We won't have a situation when one test influences another.

Back to 9878696 - I supposed that sinon.spy( autoComplete, 'updateAriaAttributesOnEditable' ), and assertion assert.isTrue( ariaUpdateSpy.calledWith( false ) ); whould pass after ESC key press simulation, but it doesn't...

So we have only two failing tests on CI. The code behavior seems to be valid (especially it pass on local machines). So it must be something 'deeper'. Based on the above, maybe it's test construction? It's definitely a problem only with those new tests - so again, please dig a little big deeper.

[Comandeer]

It seems that tests fail due to #4653 – change-isActive is incorrectly handled as evt.data is set to false.

[f1ames]

I guess it's now unblocked since #4652 was fixed?

[Comandeer]

Rebased onto latest master.

[Comandeer]

Ok, it seems that fix for #4652 fixed issues with tests. I've also added a small test for #4653.

[sculpt0r]

@Comandeer
It's nice that you solved an issue with failing CI - which actually was a manifestation of another issue with our events 👍 Now we have an even better code base after all these years :)

Screen readers announce autocomplete in a correct way (according to manual test) 👍

I left a few inline comments - mostly very small details.

The casual full test run passed on my FF and CI Chrome: 👍
However, we have two failing security tests now due to id and role attribute was added to li elements.

Could you also adjust those two?

[Comandeer]

Adjusted tests, all should be green now.

[sculpt0r]

Good job 🎉 LGTM 🎉

I have to remove a single quote from the unit test message, which was... my suggestion before 🙈

I verified corrected unit tests and they pass 👍