-
Notifications
You must be signed in to change notification settings - Fork 306
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
better support for contenteditables in iframes (ckeditor, tinymce, etc) #252
Conversation
- added detection code in init to detect whether element is contained in an iframe (popular with ckeditor, tinymce, etc) - altered logic in contenteditable to use ownerDocument instead of document or window - altered positioning logic for situation where autocompleter is located in an iframe - built new dist files
@@ -78,7 +78,8 @@ | |||
throw new Error('textcomplete must be called on a Textarea or a ContentEditable.'); | |||
} | |||
|
|||
if (element === document.activeElement) { | |||
// use ownerDocument to fix iframe / IE issues | |||
if (element === this.$el[0].ownerDocument.activeElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my understanding, this.$el[0]
is identical to element
. Do you have any reason to avoid using element.ownerDocument.activeElement
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, no reason, I'll change it. Probably due to a find and replace. I wrote a lot of this stuff ages ago and once it was working I didn't look back at it :)
Hi @kjhangiani, thanks for sending the patch. Please check my comments. And please reset dist directory. I'll update the directory when I release the next version. |
Looks like I did something similar today ... anyways, thanks :-) 9a61b19 |
27b6ee0
to
6141da3
Compare
- changed references to `this.$el[0]` in content_editable.js to `this.el` since this is available - similar change made in completer.js - restored dist files to master
6141da3
to
d2997ae
Compare
@yuku-t I have made those changes and reset the dist directory. Also, while I haven't encountered this situation, I imagine a similar problem will exist if using a textarea with autocomplete in an iframe. I think that situation is very rare, but I imagine similar ownerDocument logic will be required there as well (and possibly positioning logic too). If I get some time this weekend I'll try and throw a test together and confirm if it has the same bug, but just thought I should mention. Thanks for the great addon! For anyone who stumbles here from google, here's a commit with the built dist files that can be used in bower until this is merged and released: |
Thanks for your contribution! v1.3.5 is out |
Hi Developers, Can you please answer to this questions? #285 Thanks |
Sorry for the extremely long delay in submitting this PR, I've had this fix on a forked copy for a while, but just recently had the time to update our version, and decided to re-submit the fix as an official PR.
This should result in no changes in behavior, but will now support textcomplete when used by contenteditables in an iframe. This is popular with plugins such as tinymce and ckeditor. We currently use this fix with tinymce and it works well, I haven't tested ckeditor but the fix should apply there as well.
It makes the following changes: