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

fix: No longer blocks text selection in contenteditable #98

Closed
wants to merge 1 commit into from
Closed

fix: No longer blocks text selection in contenteditable #98

wants to merge 1 commit into from

Conversation

jamesthomsondev
Copy link

This fix addresses issue #97

There are 2 parts to this fix.

  1. Instead of blocking the selectstart event with preventDefault, we use a CSS value placed on the document body. This allows for the same behaviour (so that text on the web page isn't selected), but no longer blocks the event from contenteditable elements.
  2. preventDefault now only occurs if allowTouch is true as these aren't necessary on desktop and contributed to preventing the plugin from working well with selectable text in inputs & contenteditable elements.

Note: These changes haven't been heavily tested on mobile

@simonwep
Copy link
Owner

Thank you for your PR - but this seems to be more of a "hack" - you're altering the DOM without the developer knowing. Also I really don't like the setTimeout(..., 0) part - this reminds me of the days of hacky jQuery to do stuff.

And as I mentioned in #97, the selectstart event needs to be prevented.

@simonwep
Copy link
Owner

Okay, again - thank you very much for spending time on this! I definetely helped me shed a light on this ^^ I'll got with simply removing the line where selectstart gets prevented - it's no longer required. See my comment on #97.

I'll close this for now :) Hope it's fixed for you in v2.0.2!

@simonwep simonwep closed this Mar 14, 2021
@jamesthomsondev
Copy link
Author

@simonwep

you're altering the DOM without the developer knowing.

I suppose so, but to play devils advocate, you were blocking events without the developer knowing...

Also I really don't like the setTimeout(..., 0) part - this reminds me of the days of hacky jQuery to do stuff.

This is a pretty standard technique in JS to push the event to the end of the event queue. It may not look pretty, but there's nothing hacky about it.

Removing selectstart has definitely helped and I can use the event hooks to workaround the other issues. Thanks for looking into this.

@jamesthomsondev
Copy link
Author

@simonwep I spoke too soon. The evt.preventDefault(); // Prevent swipe-down refresh block any possibility of selecting text within a contenteditable.

@simonwep
Copy link
Owner

I suppose so, but to play devils advocate, you were blocking events without the developer knowing...
Haha, I guess so :D

This is a pretty standard technique in JS to push the event to the end of the event queue. It may not look pretty, but there's nothing hacky about it.

Yeah, the "pretty thing" - that's what I'm worried about ^^

I spoke too soon. The evt.preventDefault(); // Prevent swipe-down refresh block any possibility of selecting text within a contenteditable.

Hahaha, so much legacy code I was afraid to remove in v2 ^^ #99 Looks promising, I will take a look at it later this week :)

@jamesthomsondev
Copy link
Author

jamesthomsondev commented Mar 15, 2021

Yeah, the "pretty thing" - that's what I'm worried about ^^

JavaScript is not a very pretty language, so don't worry about it too much ;) The important part, IMO, is that it works in an expected manor.

Hahaha, so much legacy code I was afraid to remove in v2 ^^ #99 Looks promising, I will take a look at it later this week :)

Great! Hopefully it's the final piece to these issues and improves your plugin. It's been very useful, great job!

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.

3 participants