Graphite icon indicating copy to clipboard operation
Graphite copied to clipboard

Prevent browser forward/backward navigation keys from crashing editor

Open skoriop opened this issue 1 year ago • 8 comments

This adds the BrowserBack and BrowserForward browser control keys to ModifierKeys.

Closes #1628.

skoriop avatar Feb 28 '24 05:02 skoriop

I'm thinking that the Rust backend never has a use for browser state, since that's a concept that doesn't apply outside the browser context (like when we have a desktop app through Tauri). Since it won't be used, I'd prefer taking the approach of blocking such events from ever being sent to the Rust backend while it's still in the JS context, as it should remain a purely JS concern. Could you please try updating your approach to do that and test that it still indeed fixes the crash? Thanks!

Keavon avatar Feb 28 '24 06:02 Keavon

I should also mention that I wasn't able to reproduce this crash when I just tested it. (See my comment from just now in the issue.) Were you able to reproduce it?

Keavon avatar Feb 28 '24 06:02 Keavon

I should also mention that I wasn't able to reproduce this crash when I just tested it. (See my comment from just now in the issue.) Were you able to reproduce it?

No, not really, I thought it was an issue with my mouse

skoriop avatar Feb 28 '24 16:02 skoriop

I disagree that we should discard the forwards and backwards buttons as you previously mentioned that they should be used for undoing changes in selection @Keavon.

I can trivially reproduce on chrome by creating a new document and then pressing the forwards button.

0HyperCube avatar Feb 28 '24 16:02 0HyperCube

@0HyperCube wait, are we talking about mouse buttons or the clickable buttons left off the URL bar in the browser? I was interpreting this to be the latter.

Keavon avatar Feb 28 '24 18:02 Keavon

Ok, I misunderstood, now I get what's going on with it being a mouse button. This will also be part of #706.

Disregard my earlier statement about blocking it in the JS, sorry for the confusion. However I don't think it's accurate for these to be considered modifier keys. In the JS, the necessary code should be used to interpret it (in all browsers, since apparently this particular one is specific to Chrome?) and send it as a new NavigateBack and NavigateForward key which can be added to the Key enum in input_keyboard.rs.

Keavon avatar Feb 28 '24 21:02 Keavon

I'm not sure you understand that I'm referring to mouse buttons (i.e. buttons on the mouse). These are not keyboard keys and thus should not be classified as such. As per the aforementioned link to https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/buttons#value we can see that the mouse buttons are standardised and therefore it would not make sense to overcomplicate this issue by trying to re-invent keyboard input systems.

0HyperCube avatar Feb 28 '24 21:02 0HyperCube

We currently extend the W3C keyboard button spec with the mouse buttons (here) in our backend input handling code, which lets us treat a button press on the mouse the same way we'd treat a button press on the keyboard. We'll also be able to use buttons on a stylus, a 3D mouse, a joystick or gamepad, a VR controller, or any other exotic input device the same way (some of these could be used to drive real time animation in the future, perhaps). In other words, our backend (at least based on its current architecture) doesn't care what physical device the buttons are part of, we just care that it's a physical button that can be pressed and released (as opposed to something fundamentally different like an axis input, a scroll wheel, fingers on a touchscreen, a gesture, etc.).

Keavon avatar Feb 28 '24 21:02 Keavon