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

Go back/forward keybinding should not traverse history on key release #20478

Closed
cbrewster opened this issue Mar 30, 2018 · 7 comments
Closed

Go back/forward keybinding should not traverse history on key release #20478

cbrewster opened this issue Mar 30, 2018 · 7 comments

Comments

@cbrewster
Copy link
Member

@cbrewster cbrewster commented Mar 30, 2018

When using the go forward/back keybindings to traverse the session history, the constellation is flooded with traversal messages as long as the key binding is held down. We should just send a single message for a key press or at least have an interval of a second or so between subsequent messages.

I am using macOS, not sure if this is the case on other platforms.

@cbrewster cbrewster added the A-input label Mar 30, 2018
@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Mar 30, 2018

I think it's not repeating too fast. But we get 2 events for key down and key up. So the event is sent twice.

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Mar 30, 2018

Note to whoever might work on this: if we send reload/goForward/… messages on KeyState::Pressed, we need to figure out if we need to send key window events to Servo on KeyState::Released key events.

@cbrewster cbrewster changed the title Go back/forward keybinding traverses history all at once Go back/forward keybinding should not traverse history on key release Mar 30, 2018
@cbrewster
Copy link
Member Author

@cbrewster cbrewster commented Apr 10, 2018

We should only send the Navigation event on key pressed or key repeated and not on key released.
To fix this, check the state in both these branches and make sure it is not equal to key released.

(CMD_OR_ALT, None, Key::Right) | (KeyModifiers::NONE, None, Key::NavigateForward) => {
if let Some(id) = self.browser_id {
let event = WindowEvent::Navigation(id, TraversalDirection::Forward(1));
self.event_queue.push(event);
}
}
(CMD_OR_ALT, None, Key::Left) | (KeyModifiers::NONE, None, Key::NavigateBackward) => {
if let Some(id) = self.browser_id {
let event = WindowEvent::Navigation(id, TraversalDirection::Back(1));
self.event_queue.push(event);
}
}

@cbrewster cbrewster added the E-easy label Apr 10, 2018
@highfive
Copy link

@highfive highfive commented Apr 10, 2018

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@piotr-szpetkowski
Copy link
Contributor

@piotr-szpetkowski piotr-szpetkowski commented Apr 12, 2018

Hi.
I don't have much experience in Rust, but it seems like an easy thing to do, so I'll try to tackle it.

@highfive: assign me

@highfive
Copy link

@highfive highfive commented Apr 12, 2018

Hey @piotr-szpetkowski! Thanks for your interest in working on this issue. It's now assigned to you!

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jul 31, 2018

#21250 fixed this.

@paulrouget paulrouget closed this Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.