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

Skip the editorInteraction event when there is no native selection. #303

Closed
wants to merge 1 commit into from

Conversation

dantman
Copy link
Contributor

@dantman dantman commented Jul 27, 2015

This fixes #302.

@ipeychev
Copy link
Contributor

Just started reviewing :)

:octocat: Sent from GH.

@ipeychev
Copy link
Contributor

There are other public functions, which depend on the native selection, so we should fix them too.
For that purpose, let's go with the following commit.

Thank you very much for the contribution!

@ipeychev ipeychev closed this Jul 28, 2015
@ipeychev ipeychev reopened this Jul 29, 2015
@ipeychev
Copy link
Contributor

Just started reviewing :)

:octocat: Sent from GH.

@ipeychev
Copy link
Contributor

Hey @dantman,

After the changes I committed, the exception does not appear anymore, but your PR actually could be useful in order to resolve an UX issue, so I will merge it. Basically, at least in Chrome and using the current master, if you make a selection (so the toolbars appear) and switches to another application using Cmd/Ctrl + Tab, the toolbars will appear outside of the editor. This is due to the default values we set to the selection data object.

Some notes about the commits we make in AlloyEditor:

  1. In general, we have a strict rule - a commit and especially bugfix should have tests. An exception of this rule is when we just cannot test it. I think this issue falls exactly in this category. I tried several ways, but I wasn't able to force all browsers to loose the native selection. If you can think of a way to test it, that would help a lot!

  2. All commit messages should include something like:

Fixes #303

so we will know what was the purpose of the commit and to which ticket it belongs.
I amended your commit message, this was just FYI.

Thanks again for your contribution, we really appreciate it!

Iliyan

@ipeychev ipeychev closed this in db977d1 Jul 29, 2015
ipeychev added a commit that referenced this pull request Jul 29, 2015
ipeychev added a commit that referenced this pull request Jul 29, 2015
@ipeychev
Copy link
Contributor

Thank you, pull request merged! See changes here.

:octocat: Sent from GH.

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

Successfully merging this pull request may close these issues.

Uncaught TypeError: Cannot read property 'createRange' of null
2 participants