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: Skip text processing when range is null #598

Merged
merged 4 commits into from Jul 17, 2021
Merged

fix: Skip text processing when range is null #598

merged 4 commits into from Jul 17, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jul 15, 2021

Avoid raising errors when range is null. Calling caretRangeFromPoint when "x or y are negative, outside viewport, or there is no text entry node" will return null. One common example is hovering over a scrollbar.

Fixes #386

Avoid raising errors when range is null. Calling caretRangeFromPoint when "x or y are negative, outside viewport, or there is no text entry node" will return null. One common example is hovering over a scrollbar.

Fixes #386
@melink14
Copy link
Owner

This looks good and thanks for the contribution! I'll probably wait til I merge the #561 PR statck since that adds testing framework that I want to start using for incoming changes now that I finally have all TS and lint errors fixed. (I got behind on code reviews so the stack has been growing...)

I'll probably do that tonight or this weekend JST and then add a quick test to your PR and merge it.

If you're interested you could retarget your PR to that branch chain and then add a quick test yourself after looking at the examples in my PRs but otherwise merging shouldn't take too long I hope (famous last words)

@melink14 melink14 changed the title fix: skip text processing when range is null fix: Skip text processing when range is null Jul 16, 2021
@melink14
Copy link
Owner

I caught up on pull requests for now so I'll check this out in more detail this weekend and then merge dictionary PR which will trigger store update!

I might want to keep the larger console issue open since it occurs to me that instead of printing client errors directly it would be better to report them to the background script for printing in that debug console. (assuming it still works with MV3)

@ghost
Copy link
Author

ghost commented Jul 16, 2021

Thanks! Not in any big rush, whenever you can get to it is fine. Unfortunately I don't think I'll have time to add unit tests in the next couple days, before you get around to reviewing/merging it anyways. If this is still open after the weekend I can take a stab at it though, for sure. I think it should be enough to test the tryUpdatePopup function, passing it a MouseEvent with negative clientX/clientY values.

@codecov
Copy link

codecov bot commented Jul 17, 2021

Codecov Report

Merging #598 (3c833f1) into main (992bdcc) will increase coverage by 1.36%.
The diff coverage is 96.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #598      +/-   ##
==========================================
+ Coverage   39.81%   41.18%   +1.36%     
==========================================
  Files          11       11              
  Lines        3104     3130      +26     
  Branches       48       67      +19     
==========================================
+ Hits         1236     1289      +53     
+ Misses       1868     1841      -27     
Impacted Files Coverage Δ
extension/rikaicontent.ts 35.93% <75.00%> (+2.42%) ⬆️
extension/test/rikaicontent_test.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 992bdcc...3c833f1. Read the comment docs.

- Adds simulant for easy event simulation
- Add `null` to `caretRangeFromPoint` type to make mistakes like this more obvious.
@melink14
Copy link
Owner

I added a test for the branch early return but coverage is penalizing me for not testing that the inverse of the branch as well. I'd like near 100% relative coverage if possible but in this case the missing branch is the happy path and so adding a test would be more code health work so I'll probably just leave it for now.

@mergify mergify bot merged commit ae55bff into melink14:main Jul 17, 2021
melink14 pushed a commit that referenced this pull request Jul 17, 2021
## [2.0.0](v1.2.6...v2.0.0) (2021-07-17)

### ⚠ BREAKING CHANGES

* This version includes optional chaining requiring Chrome >=80

### Features

* Force Google Docs to use HTML mode instead of canvas mode ([#596](#596)) ([94b60a6](94b60a6)), closes [#593](#593)
* **detection:** Ignore invisible nodes when extracting text under mouse ([#561](#561)) ([cb97f36](cb97f36)), closes [#159](#159) [#366](#366) [#159](#159)

### Bug Fixes

* **dict:** Update dictionaries to latest versions. ([#581](#581)) ([77189c3](77189c3))
* Skip text processing when range is null ([#598](#598)) ([ae55bff](ae55bff)), closes [#386](#386)

### Code Refactoring

* Migrate from webpack to snowpack for build step ([#583](#583)) ([1bdd3d3](1bdd3d3))
@melink14
Copy link
Owner

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@melink14
Copy link
Owner

@all-contributors please add @qkjosh for bug report and fix.

@allcontributors
Copy link
Contributor

@melink14

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

mergify bot pushed a commit that referenced this pull request Jul 17, 2021
Add @qkjosh as a contributor for bug.

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

(looks like natural language parsing wasn't good enough)
@all-contributors also add @qkjosh for code.

@allcontributors
Copy link
Contributor

@melink14

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

mergify bot pushed a commit that referenced this pull request Jul 17, 2021
Add @qkjosh as a contributor for code.

This was requested by melink14 [in this comment](#598 (comment))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove console logs from production build
1 participant