allow arrow keys to rotate and pitch the map when focused on the NavigationControl compass button #8953
Conversation
…gationControl compass button
src/ui/control/navigation_control.js
Outdated
| @@ -102,6 +102,37 @@ class NavigationControl { | |||
| this._handler = new DragRotateHandler(map, {button: 'left', element: this._compass}); | |||
| DOM.addEventListener(this._compass, 'mousedown', this._handler.onMouseDown); | |||
| DOM.addEventListener(this._compass, 'touchstart', this._handler.onMouseDown, {passive: false}); | |||
| DOM.addEventListener(this._compass, 'keydown', (e) => { | |||
| const bearingStep = 15, | |||
There was a problem hiding this comment.
I've made the implementation here since the zoom in/out button clicks are performed here, and the KeyboardHandler seems focused on handling keyboard events on the map, not the NavigationControl. This does lead to these constants defined twice, is that okay or should they be defined once and them imported into both the KeyboardHandler and NavigationControl?
There was a problem hiding this comment.
Sorry, which constants are defined twice here?
We don't have a clean separation of focus and input handling, probably KeyboardHandler is a bit misnamed if we have multiple controls that need to handle the keyboard but KeyboardHandler just represents the focused map.
There was a problem hiding this comment.
bearingStep and ptichStep are also defined at
mapbox-gl-js/src/ui/handler/keyboard.js
Lines 8 to 9 in 2377134
There was a problem hiding this comment.
Ah thank you. I think it's good to have them defined once and imported. I don't think we'd want to modify these values separately. Maybe it's a good time to start a src/ui/constants.js? I am having a hard time finding other examples in the project of a shared constants file but it seems reasonable to me.
Do you have an idea how you would test this? I'd be really appreciative if you drafted some simple ones to get us started. I don't have strong opinions on how to unit test this. I personally like supporting accessibility features but as with many of our smaller features we're a bit challenged right now adequately retesting these things. Having some basic tests to verify regressions would help a lot with adding more small improvements like this to things that fall outside our main development path. |
invoke the key event and then compare the map rotation and pitch to before the key event was invoked. I'll do some more work to add in unit tests. |
src/ui/control/navigation_control.js
Outdated
| @@ -102,6 +102,37 @@ class NavigationControl { | |||
| this._handler = new DragRotateHandler(map, {button: 'left', element: this._compass}); | |||
| DOM.addEventListener(this._compass, 'mousedown', this._handler.onMouseDown); | |||
| DOM.addEventListener(this._compass, 'touchstart', this._handler.onMouseDown, {passive: false}); | |||
| DOM.addEventListener(this._compass, 'keydown', (e) => { | |||
| const bearingStep = 15, | |||
There was a problem hiding this comment.
Ah thank you. I think it's good to have them defined once and imported. I don't think we'd want to modify these values separately. Maybe it's a good time to start a src/ui/constants.js? I am having a hard time finding other examples in the project of a shared constants file but it seems reasonable to me.
|
It might be useful to still keep this functionality even if #9036 goes ahead since shift+arrow is hard to discover, whereas the arrow keys when focused on the compass might be more discoverable? I'm not sure. |
|
Hi @andrewharvey, first of all thanks for all your work on this - didn't mean to derail it with #9036 but I do think we need to examine whether this functionality is necessary. That said, as I mentioned in #9036 (comment) I'm not sure that implementing #9036 means we shouldn't still consider merging this - I agree with your last comment:
I'm not sure either - I can see both arguments:
@andrewharvey do you have any feedback/data points from users or other devs that the shift-arrow control is too hard to discover and that focusing the compass and then using arrows would be more user-friendly? That would make sense to me but just based on speculation as I don't really have any data to go off of - wondering if you have data points on that or how we could go about getting some. I could go either way on this - if we have reason to believe that this will provide the best experience for keyboard-only/keyboard-preferred users, I'm fine with adding this regardless of #9036. Either way, I would love to merge in the test(s) you added, they're sorely needed! cc @ahk @ryanhamley |
Oh not at all, I agree that it's important to re-consider if the current UX is worth retaining going forward.
Not really, I'm not sure any particular method is better 🤷♂️ . I think this PR is quite low priority so happy to leave it sit here while we see how #9036 pans out, and in case there is any other user feedback.
The unit tests I did were just for the functionality added in this PR, so no harm done if you're working on some too. I'm not fussed, if you have some ready to ship that's great! |
|
Looks like this is stale — let's close, and possibly reopen when we're ready to work on landing this again. |
Launch Checklist
closes arrow keys to change bearing when bearing button is active #8459
given you can rotate and pitch with the mouse on the compass button, the same actions should be achievable with the keyboard only.
there were no existing tests for the NavigationControl so I haven't added any, should I go ahead and add some unit tests for the NavigationControl or does this control not need unit tests?