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

Page with no text gets affected when rikaikun in enabled #1114

Closed
greggman opened this issue Jul 4, 2022 · 5 comments · Fixed by #1134
Closed

Page with no text gets affected when rikaikun in enabled #1114

greggman opened this issue Jul 4, 2022 · 5 comments · Fixed by #1134

Comments

@greggman
Copy link

greggman commented Jul 4, 2022

Describe the bug
Page with no text gets broken when rikaikun is enabled. Note: This is not a priority or a request for fixing/support. Rather, given that it was easy to reproduce I thought i'd report it as maybe such a simple case is an easy repo for some other harder to repo case.

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://jsgist.org/?src=51d9921bea2c7f7066e2592569a2c986
  2. Click on slide the slider (blue) all the way to the left and then back
  3. Let off the mouse button
  4. Slide it again

Expected behavior
The slider functions correctly (it's just a styled <input type="range">

Screenshots
A "0" appears on the left and the slider stop working. This only happens if rikaikun is enabled.

System details (OS, browser version, rikaikun version)

  • OS: MacOS 12.4
  • Browser Version: Chrome 103
  • rikaikun Version: 2.4.8

Additional context
Thanks so much for Rikaikun!!!! I've used it for years and it's invaluable! ❤️

@melink14
Copy link
Owner

melink14 commented Jul 5, 2022

Thanks @greggman for reporting! This is definitely an bug! cc @all-contributors

The most direct cause is this early return which doesn't remove fake div we use when we detect inputs:

if (range === null) {
return;
}

It probably doesn't come up much since it requires the mouse to be outside the viewport and a range slider which is near the edge of the frame is an easier way to repro than most real situations!

More than that though, I think we should limit the types of inputs we try to extract text from. It was originally added when <input> was just text but now it can be things like range, date, and number which aren't useful to try to get Japanese text from. We should probably add a whitelist for input types which rikaikun tries to process since it's a waste of cycles if nothing else!

Let's leave this bug for properly cleaning up the added <div> and I'll open another one for being more choosy in trying to process <input>s

@melink14
Copy link
Owner

melink14 commented Jul 5, 2022

@all-contributors please add @greggman for bugs

@allcontributors
Copy link
Contributor

@melink14

I've put up a pull request to add @greggman! 🎉

mergify bot pushed a commit that referenced this issue Jul 5, 2022
Add @greggman as a contributor for bug.

This was requested by melink14 [in this comment](#1114 (comment))
@melink14
Copy link
Owner

While debugging I found that sometimes the iframe of jsgist is a blob: url and so rikaikun doesn't work in it. jsfiddle works as well for testing though.

@mergify mergify bot closed this as completed in #1134 Jul 17, 2022
mergify bot pushed a commit that referenced this issue Jul 17, 2022
melink14 pushed a commit that referenced this issue Jul 18, 2022
## [2.4.11](v2.4.10...v2.4.11) (2022-07-18)

### Bug Fixes

* **dict:** Update dictionaries to latest versions ([#1140](#1140)) ([70de0be](70de0be))
* Remove fake div if returning early because `caretRangeFromPoint` is null ([#1134](#1134)) ([e7d40b8](e7d40b8)), closes [#1114](#1114)
@melink14
Copy link
Owner

🎉 This issue has been resolved in version 2.4.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

3 participants