Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add close event as possible alternative to handling closing in the Viewer. #215

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Tade0
Copy link
Contributor

@Tade0 Tade0 commented Jun 15, 2016

This PR is related to this:
#200 (comment)
comment and should fix the problem in #200 without breaking encapsulation.

The idea is for the Viewer to implement the CKEDITOR.event interface and emit a close whenever the user wishes to close AC. The ViewerController can listen on that event and act accordingly.

@mlewand
Copy link
Contributor

mlewand commented Jun 30, 2016

close event seems to be a good idea.

Actually these two listeners are quite important as those explicitly blur the panel, meaning that in turn it will put the focus back to the editor. To test it just remove these listeners, and click the close button - it will close AC but if you type something, the editor won't get anything.

Actually esc key is handled globally by HotkeyManager so that's the reason why it's working when te listener was removed. Because of that we can even remove listening for esc here (on a side note, listener added in this PR would work only in close button, and it wouldn't blur the panel).

See my changes on top of the branch. Still there's an issue:

  • There are no manual tests.

@Tade0 Tade0 added the review? label Jul 6, 2016
@f1ames
Copy link
Contributor

f1ames commented Jul 26, 2016

I found that one test is failing (https://github.com/cksource/ckeditor-plugin-a11ychecker/blob/t/200a/tests/Controller/CheckingMode.js#L46) throwing Unexpected error: viewerPanel.blur is not a function.

I run tests with dev version using t/200a branch with CKEditor major branch. Is this a bug or some mismatch in my testing environment configuration?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants