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

Normal mode isolation #176

Merged
merged 5 commits into from
Jul 31, 2020
Merged

Normal mode isolation #176

merged 5 commits into from
Jul 31, 2020

Conversation

nbelzer
Copy link
Collaborator

@nbelzer nbelzer commented Jul 27, 2020

Resolves #170, #168, #148, #110

Normal mode is the mode where all Vimari bindings work, however as mentioned in for example #168 and #148 these can overlap with the keybindings set on the webpage. This can be a rather frustrating experience to the end user.

Insert mode is the mode where Vimari bindings are disabled and you can interact with the website. Currently this mode is automatically enabled when you are editing a text field.

This PR brings back the isolation between website and Vimari as you would experience in similar extensions such as Vimium. It works as follows:

  • By default a website is loaded in normal mode.
  • When enabled, Vimari no longer allows you to interact with the underlying website (in normal mode).
  • You can enable insert mode as always by pressing i. It can be disabled using esc.
  • In insert mode all keybindings from Vimari are disabled and instead you can use the keybindings available from the underlying website.

Changes

  • To implement this I found that it was required to change a piece of code in the Mousetrap library. This change (shown below) allows us to capture events before they reach the underlying website and therefore block them in case we are in normal mode.
  function _addEvent(object, type, callback) {
      if (object.addEventListener) {
-          object.addEventListener(type, callback, false);
+          // VIMARI CUSTOMISATION:
+          // We set the useCapture to true such that events are handled before
+          // being dispatched to any EventTarget beneath it in the DOM tree.
+          object.addEventListener(type, callback, true);
          return;
      }
  • Because of the need to alter the Mousetrap library I decided that it would be best to update to the latest version of mousetrap as well (1.3.5 to 1.6.5). This was a minor update and should therefore not cause any issues. However I'll keep looking out for weird behaviour.

Whats next?

  • I believe that Vimari could be more transparent to the user when insert mode is enabled. Vimium shows a little status bar with "Insert Mode" at the bottom when enabled. Vimari already has a similar implementation (press f to see it pop up), so something similar could be implemented.

nbelzer added 4 commits July 25, 2020 18:26
Upon manual testing this did not break any functionally. Since this
updates the Minor from 3 to 6 there should not be any breaking changes.

The library is upgraded such that a minor modification can be made,
allowing us to properly isolate website behaviour from Vimari (normal
mode). I figured it best to perform this modification on the latest
version of Mousetrap such that possible new features are included.
To be able to stop the propagation of events to the website but still
capture them in Vimari we need to listen to the keyEvents using the
onCapture flag. Something that has not yet been implemented in
Mousetrap (see ccampbell/mousetrap#400).
@nbelzer nbelzer requested a review from danielcompton July 27, 2020 11:58
@nbelzer nbelzer self-assigned this Jul 27, 2020
@nbelzer nbelzer changed the title Implement normal vs insert mode isolation Normal mode isolation Jul 27, 2020
@nbelzer nbelzer mentioned this pull request Jul 30, 2020
Copy link
Member

@danielcompton danielcompton left a comment

Choose a reason for hiding this comment

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

Great, thanks! Happy for you to merge when ready.

@@ -1,5 +1,9 @@
// Custom behaviour for Vimari implemented at line 185. Be aware of this
Copy link
Member

Choose a reason for hiding this comment

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

Good callout.

@danielcompton danielcompton mentioned this pull request Jul 31, 2020
@gaisin
Copy link

gaisin commented Sep 11, 2020

Thanks a lot for this feature — might be super useful!

Do you have any advices on disabling ESC to exit Safari's full screen?
I've found this topic on Stack Overflow and tried to remap ESC with Option+ESC, but then Vimari will also not be able to catch ESC pressed.
So basically my problem is that when I'm trying to exit Vimari's Insert mode Safari will exit full screen mode.

@nbelzer
Copy link
Collaborator Author

nbelzer commented Sep 11, 2020

@gaisin Next to ESC you can also use CTRL + [ to exit insert mode which would solve your problem I think. Might look in to making these bindings (enter and exit insert mode) configurable as wel in the future.

@gaisin
Copy link

gaisin commented Sep 12, 2020

@nbelzer Thanks, that works for me!

@Hultner
Copy link

Hultner commented Sep 17, 2020

@nbelzer CTRL+ [ is quite difficult to use on nordic keyboards since "[" is hidden behind modifiers, significantly harder than Esc.

On previous Safari versions where TamperMonkey still worked I had my own solution for this, basically back then I did it by adding an event-listener on ESC preventing default behaviour like this, which was sufficient. Having this in vimari would rid my #1 annoyance with Safari.

const ESC_KEY = 27;
const KEYPRESS_EVENT = "keypress";

window.addEventListener(KEYPRESS_EVENT, 
    event => event.keyCode === ESC_KEY && event.preventDefault()
);

@Hultner
Copy link

Hultner commented Sep 17, 2020

@nbelzer Tested CTRL+[ on nordic keyboard and it doesn't work at all, it does nothing if I enter insert-mode, I can only exit using ESC which now always exits fullscreen. This makes the insert-mode/normal-mode completely broken for nordic users, and probably a lot of other non english speaking users like German, French, and Spanish.

@nbelzer
Copy link
Collaborator Author

nbelzer commented Sep 17, 2020

@Hultner Thanks for the detailed response. At the moment I'm quite busy with University again but hopefully I'll have some time to look deeper into the issue this weekend. It should be possible to either make the bindings variable or implement something similar to the logic you presented to avoid this problem completely.

I believe the issue here to be similar to what is described in #200 so I'll continue discussion there.

@Hultner
Copy link

Hultner commented Sep 17, 2020

@nbelzer Thanks for the swift reply, and that is of course very understandable, university is of course more important and I don't expect you to put time into this unless it's something you want to do and feel you have the time and capacity to do.

With that said if you do end up looking into the issue (over this weekend or other time), do feel free to reach out to me if I can assist in any way.

I think the sweetest option would be if there was a setting to always preventDefault for escape, as well as rebinding, for instance some may want to rebind to ^C which works in normal vim but also works on pretty much all keyboards since it doesn't use any special characters. For me using Esc (actually Caps Lock rebound) is so ingrained in my muscle memory that I end up hitting it all the time whenever I interact with text even if I'm not using vim. At this point it's like the period at the end of a sentence, a reflex deeply ingrained in my reptilian brain.

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.

Support for insert mode
4 participants