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

Keyboard Handler #663

Merged
merged 11 commits into from Jul 27, 2012
Merged

Keyboard Handler #663

merged 11 commits into from Jul 27, 2012

Conversation

ericmmartinez
Copy link
Contributor

This implements a feature requested in issue #646.

Implementation tested in all browsers I have access to and I see no reason why it shouldn't work in additional browsers. The biggest obstacle is the event.keyCode for the various keys in different browsers. Implementation is done such that as new keyCodes are found in different browsers they can easily be supported by the handler. I would request additional browsers be tested. Not sure on the feasibility of support on mobile platforms.

Currently known to work on:

Mac OSX 10.6.8

  • Safari 5.1.1
  • Firefox 11
  • Chrome 18

Windows 7

  • IE 8
  • IE 9
  • Firefox 4
  • Chrome 18

Implementation adds a "keyboard" option for the L.Map configuration options. Default set to true, enables keyboard navigation when the most recent click was on the map. Can be set to false to disable this navigation.

Implementation supports variable panning distance and zoom level jumping by calling setPanOffset and setZoomOffset respectively. Both methods accept an integer parameter. In the former method the integer is the number of pixels to pan by when panning. In the latter offset the integer is the number of zoom levels to step in/out when zooming. Passing a negative integer to either method is supported and will effectively invert the controls. Note: A wrapper method could be added to the map for convenience. Currently a user would have to do something like the following to call these methods. (Seems unclean as-is)

// Pan by 100 pixels when an arrow key is pressed
map.keyboard.setPanOffset(100);

// Zoom in/out 2 zoom levels when +/- is pressed.
map.keyboard.setZoomOffset(2);

Default pan offset is 50 pixels. Default zoom offset is 1 zoom level.

@mourner
Copy link
Member

mourner commented May 4, 2012

Wow, this is awesome! Great work and really useful — will be merged for sure!

Several suggestions:

  1. for accessibility reasons, it's not enough to enable keyboard navigation after mouse clicking since user may not have access to mouse or navigates purely with keyboard, so I think it's better to implement L.Map should be able to acquire/lose focus #594 (ability for map to aquire focus) and also add keyboard hooks immediately, not after mouse click.
  2. I think it's better add pan and zoom offsets as Map options (like keyboard, e.g. keyboardZoomOffset) instead of setters for consistency.

@ghost ghost assigned mourner May 4, 2012
@ericmmartinez
Copy link
Contributor Author

I've implemented the changes you requested including creating a L.Map.Focus handler. This will now trigger "focus" and "blur" events by user tabbing in/out of the map as well as clicking on/off the map (respectively). The L.Map.Keyboard handler has been updated to make use of this new focus event. Others can listen for focus with:

map.on('focus', this.onMapFocus, this);
map.on('blur', this.onMapBlur, this);

The underlying focus/blur functionality was achieved by following this article. Only tested so far on Mac OSX 10.5.8 running Chrome 18, Firefox 6 and Safari 5. More testing is appreciated. Hopefully this at least makes a decent start at issue #594.

I also enabled map configuration options for keyboardPanOffset and keyboardZoomOffset.

@mourner
Copy link
Member

mourner commented Jul 12, 2012

@ericmmartinez Hi Eric, sorry for being slow about the pull... Going to merge this for 0.4. Could you please remove the Scale control fix? It was fixed in master in a simpler way, without the need to write custom calculation code. Also, it's a good practice to have a separate pull for each change so it can be discussed/acted upon independently.

@ericmmartinez
Copy link
Contributor Author

I've rolled back my changes to the L.Control.Scale so my pull request only includes the focus/keyboard stuff now.

@mourner
Copy link
Member

mourner commented Jul 12, 2012

Awesome, thanks!

mourner added a commit that referenced this pull request Jul 27, 2012
@mourner mourner merged commit e183237 into Leaflet:master Jul 27, 2012
@mourner
Copy link
Member

mourner commented Jul 27, 2012

Works like a charm, really great work Eric! I've cleaned up the code a bit to be more Leaflet-like, and also moved all Focus code to the Keyboard handler as I couldn't come up with a use case where you need focus but don't need keyboard navigation.

@mourner
Copy link
Member

mourner commented Aug 3, 2012

Hey Eric, please take a look #869 — I guess it's safe to remove the 0 keycode?

@rostik
Copy link

rostik commented Sep 13, 2012

when i use the keyboard to pan the map, it doesn't work with the setMaxBounds. it keeps scrolling. is there a way to make it work ?

@mourner
Copy link
Member

mourner commented Sep 13, 2012

@rostik known issue, will be fixed

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.

None yet

4 participants