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

better support for contenteditables in iframes (ckeditor, tinymce, etc) #252

Merged
merged 2 commits into from May 27, 2016

Conversation

kjhangiani
Copy link
Contributor

@kjhangiani kjhangiani commented May 25, 2016

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:

  • 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 to correctly reference parent window instead of assuming it to be document
  • altered positioning logic for situation where autocompleter is located in an iframe

- 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) {
Copy link
Owner

@yuku yuku May 26, 2016

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?

Copy link
Contributor Author

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 :)

@yuku
Copy link
Owner

yuku commented May 26, 2016

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.

@amenk
Copy link
Contributor

amenk commented May 26, 2016

Looks like I did something similar today ... anyways, thanks :-) 9a61b19

@kjhangiani kjhangiani force-pushed the iframe-support branch 3 times, most recently from 27b6ee0 to 6141da3 Compare May 27, 2016 01:07
- 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
@kjhangiani
Copy link
Contributor Author

kjhangiani commented May 27, 2016

@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:
soxhub@d757e6d

@yuku yuku merged commit d8b97dc into yuku:master May 27, 2016
@yuku yuku mentioned this pull request May 27, 2016
@yuku
Copy link
Owner

yuku commented May 27, 2016

Thanks for your contribution! v1.3.5 is out

@kjhangiani kjhangiani deleted the iframe-support branch May 27, 2016 08:26
@namaljayathunga
Copy link

Hi Developers, Can you please answer to this questions? #285 Thanks

@yuku yuku added the DEPRECATED jquery-textcomplete Issues associated to jquery-textcomplete (was DEPRECATED) label Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEPRECATED jquery-textcomplete Issues associated to jquery-textcomplete (was DEPRECATED)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants