search

cksource / ckeditor4-plugin-a11ychecker

Accessibility checker for CKEditor 4
Other
19 stars 12 forks source link

Space key not handled in close button #200

Open mlewand opened 8 years ago

mlewand commented 8 years ago
  1. Open Accessibility Checker manual test.
  2. Open AC by pressing it's icon.
  3. Press shift + tab twice to move the focus on close button.
  4. Press space.

    Expected

AC gets closed.

Actual

Key event does not get handled, and default browser behavior apply (scrolling the page).

mlewand commented 8 years ago

@f1ames could you please review this one?

f1ames commented 8 years ago

@Tade0 There are still unanswered comments from @mlewand, I also added one myself, please take a look there - https://github.com/cksource/ckeditor-plugin-a11ychecker/pull/206.

Tade0 commented 8 years ago

A curious thing occurred to me: the listeners that I've been manipulating seem not to be the ones that actually do the job. Investigating now what's happening here.

Tade0 commented 8 years ago

There's an issue with this issue, take a look at these two branches: https://github.com/cksource/ckeditor-plugin-a11ychecker/tree/t/200-redundant https://github.com/cksource/ckeditor-plugin-a11ychecker/tree/t/200-broken

If you comment out the listeners in ui/Viewer, you can still use Return/ESC/click to close AC. If you, in turn, comment the listener in ui/ViewerController the Return/click behaviour is changed and although it closes the panel, it doesn't really close AC.

This could be easily fixed with breaking ecapsulation/separation of concerns by making the Viewer aware of the Controller, but I think this is not the way to go here.

My proposition is to implement an "end"(as in: "close AC") event on the Viewer and listen on it in the ViewerController the events in Viewer would fire that event and the ViewerController would act accordingly.

What do you think @mlewand , @f1ames ?

f1ames commented 8 years ago

It looks like a redundant code indeed. But I am not sure if there wasn't a reason for that. I would check to be sure if hiding the balloon panel (that's what happens in the Viewer: // Hide the panel...) is same as closing the whole a11ychecker (the comment in ViewerController seems to confirm that it is the same, duplicated behavior).

Apart from that, I think the approach with emitting close/end event inView and listening to it in ViewController is a way to go.

mlewand commented 8 years ago

@Tade0 I've reviewed #215 solution, find my notes in PR ticket.

f1ames commented 8 years ago

@Tade0 I added a comment to your PR ticket.