Skip to content

/ servo

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

make it possible to disable default keybindings #8121

Merged
merged 1 commit into from Oct 22, 2015
Merged

make it possible to disable default keybindings #8121

merged 1 commit into from Oct 22, 2015

Conversation

@paulrouget
Copy link
Contributor

paulrouget commented Oct 21, 2015

For browser.html, we want to let the top level webpage handle these keybindings.

Review on Reviewable
@paulrouget paulrouget mentioned this pull request Oct 21, 2015
Closed
62 of 92 tasks complete
@jdm
Copy link
Member

jdm commented Oct 21, 2015

This feels like an inefficient way to go about this to me. Have you tried adding key listeners to browser.html and calling preventDefault for the bindings you want Servo to ignore?
@jdm jdm self-assigned this Oct 21, 2015
@paulrouget
Copy link
Contributor Author

paulrouget commented Oct 21, 2015

This feels like an inefficient way to go about this to me. Have you tried adding key listeners to browser.html and calling preventDefault for the bindings you want Servo to ignore?

Aren't DOM events not even created at this point?

Should we propagate the events and later on check if preventDefault was called?

Also - is zooming with ctrl-scroll really a thing we need?
@jdm
Copy link
Member

jdm commented Oct 21, 2015

We prevent backspace from navigating the page when a text input is focused on a page; all key events are first sent to the focused script task, processed, and if they're not prevented they're returned to the compositor for further processing. I would be ok with removing zooming via the mousewheel if it makes your life easier.
@paulrouget
Copy link
Contributor Author

paulrouget commented Oct 21, 2015

We prevent backspace from navigating the page when a text input is focused on a page; all key events are first sent to the focused script task, processed, and if they're not prevented they're returned to the compositor for further processing

I see. What about escape?
@paulrouget
Copy link
Contributor Author

paulrouget commented Oct 21, 2015

If we can preventDefault() the quit-on-escape-key behavior and remove the ctrl-scroll binding, that would be enough for me.

I'll look at how to do that.
@jdm
Copy link
Member

jdm commented Oct 21, 2015

That would require updating http://mxr.mozilla.org/servo/source/ports/glutin/window.rs#173 to match the others, and ensuring that handle_key has a way to indicate that the app should exit.
@paulrouget
Remove zoom-on-ctrl-scroll and make quit-on-escape preventable
Loading status checks…
e60f02c
@paulrouget paulrouget force-pushed the paulrouget:disableControls branch from 0122f78 to e60f02c Oct 22, 2015
@jdm
Copy link
Member

jdm commented Oct 22, 2015

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented Oct 22, 2015

📌 Commit e60f02c has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Oct 22, 2015

⌛ Testing commit e60f02c with merge 0d4641b...
bors-servo pushed a commit that referenced this pull request Oct 22, 2015
bors-servo
Auto merge of #8121 - paulrouget:disableControls, r=jdm
Loading status checks…
0d4641b 
make it possible to disable default keybindings

For browser.html, we want to let the top level webpage handle these keybindings.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8121)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 22, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo mentioned this pull request Oct 22, 2015
Merged
@bors-servo bors-servo merged commit e60f02c into servo:master Oct 22, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@jdm jdm mentioned this pull request Oct 27, 2015
Merged
mrobinson added a commit to mrobinson/servo that referenced this pull request Oct 27, 2015
@mrobinson
Make executing synthesized pinch zoom more similar to zoom
798bde3 
Synthesized pinch zoom was removed in servo#8121 so that this combination of
mouse wheel and key press can be handled by the page. I mistakenly
re-implemented it servo#8215. In order to preserve synthesized pinch zoom,
which is very useful for testing on desktop computers, have it work
similarly to page zoom except with the ALT key pressed.
@mrobinson mrobinson mentioned this pull request Oct 27, 2015
Merged
mrobinson added a commit to mrobinson/servo that referenced this pull request Oct 27, 2015
@mrobinson
Make executing synthesized pinch zoom more similar to zoom
4955037 
Synthesized pinch zoom was removed in servo#8121 so that this combination of
mouse wheel and key press can be handled by the page. I mistakenly
re-implemented it servo#8215. In order to preserve synthesized pinch zoom,
which is very useful for testing on desktop computers, have it work
similarly to page zoom except with the ALT key pressed.
bors-servo added a commit that referenced this pull request Oct 27, 2015
@bors-servo
Auto merge of #8223 - mrobinson:pinch-zoom-2, r=jdm
a65680a 
Make executing synthesized pinch zoom more similar to zoom

Synthesized pinch zoom was removed in #8121 so that this combination of
mouse wheel and key press can be handled by the page. I mistakenly
re-implemented it #8215. In order to preserve synthesized pinch zoom,
which is very useful for testing on desktop computers, have it work
similarly to page zoom except with the ALT key pressed.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8223)
<!-- Reviewable:end -->
mrobinson added a commit to mrobinson/servo that referenced this pull request Oct 27, 2015
@mrobinson
Make executing synthesized pinch zoom more similar to zoom
f71f7ba 
Synthesized pinch zoom was removed in servo#8121 so that this combination of
mouse wheel and key press can be handled by the page. I mistakenly
re-implemented it servo#8215. In order to preserve synthesized pinch zoom,
which is very useful for testing on desktop computers, have it work
similarly to page zoom except with the ALT key pressed.
bors-servo added a commit that referenced this pull request Nov 3, 2015
@bors-servo
Auto merge of #8223 - mrobinson:pinch-zoom-2, r=pcwalton
8dd11cc 
Make executing synthesized pinch zoom more similar to zoom

Synthesized pinch zoom was removed in #8121 so that this combination of
mouse wheel and key press can be handled by the page. I mistakenly
re-implemented it #8215. In order to preserve synthesized pinch zoom,
which is very useful for testing on desktop computers, have it work
similarly to page zoom except with the ALT key pressed.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8223)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Nov 3, 2015
@bors-servo
Auto merge of #8223 - mrobinson:pinch-zoom-2, r=pcwalton
Loading status checks…
526a3c8 
Make executing synthesized pinch zoom more similar to zoom

Synthesized pinch zoom was removed in #8121 so that this combination of
mouse wheel and key press can be handled by the page. I mistakenly
re-implemented it #8215. In order to preserve synthesized pinch zoom,
which is very useful for testing on desktop computers, have it work
similarly to page zoom except with the ALT key pressed.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8223)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@paulrouget @jdm @bors-servo @highfive
You can’t perform that action at this time.