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 caret mode w and e movements #4632

Merged
merged 4 commits into from
Feb 13, 2025
Merged

Conversation

philg-dev
Copy link
Contributor

@philg-dev philg-dev commented Feb 12, 2025

Description

Note: I've decided to close PR #4630 and create this PR instead which fixes more issues that are in the exact same place in the code and thus would only create merge conflicts if I were to commit them separately based on the master. Should be much easier to review this way too.

Fix SyntaxError on Selection.modify() due to invalid granularity parameter 'vimword' due to missing return statements

Fix 'w' and 'e' not working in Caret Mode

Since forward movements for 'word' and 'vimword' use char-by-char custom logic which extends the selection range, we need to collapse the selection to the end, if we're in Caret Mode instead of Visual Mode.

Also removed dead code section for 'backward vimword', as there is no binding that ever uses that and the implemented logic is equal to 'backward word' ('b' key).

Bug analysis regarding the SyntaxError (commit c0c6f49):

  • just for reference - valid parameters for Selection.modify():
  • Vimium differentiates word and vimword
    • forward word is triggered with e to jump forward to the end of a word
    • backward word is triggered by b to jump back to the start of a word
    • forward vimword is triggered with w to jump forward right before the start of the next word (therefore including non-word characters after the original word's end)
    • there's no way to trigger backward vimword, but there is a code section that handles that case (and does the same as backward word) (P.S.: I've removed this section in the follow-up commit that fixes caret mode behavior)
  • missing return statements in several places in content_scripts/mode_visual.js in function Movement.runMovement lead to the SyntaxError
    • the section handling vimword functionality runs properly
      • forward vimword advances char-by-char and thus doesn't rely on modify's granularity parameter
      • backward vimword has no binding, but would work fine, since it replaces the granularity value by word when handing it over as a parameter to Selection.modify()
      • neither of those code sections have return statements after their work is done
      • this leads to the else section at the very end being called for the vimword cases which have already been handled

Implementation notes regarding the SyntaxError

  • I've added return statements to the vimword code sections
  • I've also added return statements to the remaining if else blocks to avoid this problem in case the function gets extended in the future
  • I've added some short comments to highlight the differences in the two code blocks for vimword and word since they look confusingly similar at the first glance
  • optionally we could completely get rid of the else sections and have the code as consecutive if() { doSomething(); return; } blocks, since each block is meant to return when done
  • in one of the cases the existing code returned the result of .modify() despite the function not having a return type, so I moved the return to a separate line instead

- fix SyntaxError on Selection.modify() due to invalid granularity parameter 'vimword' due to missing return statements
- fixes philc#3075
- fixes philc#3772
- fixes philc#4615

Since forward movements for 'word' and 'vimword' use char-by-char custom logic which extends the selection range, we need to collapse the selection to the end, if we're in Caret Mode instead of Visual Mode.

Also removed dead code section for 'backward vimword', as there is no binding that ever uses that and the implemented logic is equal to 'backward word' ('b' key).
@philg-dev philg-dev changed the title Fix caret mode w e Fix caret mode w and e movements Feb 12, 2025
@philg-dev
Copy link
Contributor Author

I've noticed a different kind of error that Firefox throws on granularity values that would generally be valid, but are unsupported in Firefox. For that I would rather do a separate PR at some other time.

More details can be found in #4633

Copy link
Owner

@philc philc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. When writing comments, uppercase the first letter of your sentence, and put a period at the end. This is documented in the coding style section of CONTRIBUTING.md.

Also fix this typo: "extent" is used in a few places where "extend" is meant.

@philg-dev
Copy link
Contributor Author

philg-dev commented Feb 13, 2025

Thanks a lot for your time. I've fixed the comments accordingly. (P.S.: sorry, I missed one of the typos on the first commit...)

@philc philc merged commit a023a65 into philc:master Feb 13, 2025
@philc
Copy link
Owner

philc commented Feb 13, 2025

Excellent, thanks!

As a nice followup, it would be good to re-enable the commented-out visual mode related tests in dom_tests.js, get them working, and ensure there's coverage for this PR's bugfix. The tests were disabled sometime ago because I think they'd fallen into disrepair. They should be rewritten as needed.

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