Control scroll zoom and add gesture handler warning #10962

avpeery · 2021-08-24T17:57:33Z

In order to address the issue of incidentally zooming the map instead of scrolling the page, this PR introduces a new feature that allows for scroll-zoom control, by requiring pressing either CMD or CTRL key while scrolling in order to zoom. There is also an alert (that can be opted on of) that appears when scrolling over the map without pressing CMD or CTRL key. This feature has been implemented as a control that can be added on. With this control implemented, you can still zoom with pinching.

There is also an option to set HTML or text in the warning. There is also an option to add new CSS classes to allow for some customization. If the alert HTML is not set by the client, the default alert will be "⌘ + scroll to zoom the map" (for MacOS) or "CTRL + scroll to zoom the map".

Check out the debug page: zoom_control.html which is also accessible as a manual testing page.

You can add the control to the map like so:

const scrollZoomBlockerControl = new mapboxgl.ScrollZoomBlockerControl();
map.addControl(scrollZoomBlockerControl);

Here is a screen recording of the feature:

Screen.Recording.2021-09-10.at.9.56.02.AM.mov

Launch Checklist

briefly describe the changes in this PR
include before/after visuals or gifs if this PR includes visual changes
write tests for all new functionality
document any changes to public APIs
manually test the debug page
tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

…o add in later if makes sense

…re handler

…el event if scroll zoom control is on

rreusser

Just some thoughts and feedback after a read-through, though I'm glad to discuss in more detail!

rreusser · 2021-08-31T17:38:35Z

debug/zoom_control.html

+    zoom: 12.5,
+    center: [-77.01866, 38.888],
+    style: 'mapbox://styles/mapbox/streets-v10',
+    scrollZoom: {controlled: true}


What do you think about requireCtrl? My only concern with the word controlled is that it has React connotations ("controlled components") which might suggest this has something to do with externally controlling zoom input rather than how it interacts with keyboard events.

rreusser · 2021-08-31T17:44:48Z

src/ui/handler/scroll_zoom.js

@@ -154,6 +168,10 @@ class ScrollZoomHandler {

    wheel(e: WheelEvent) {
        if (!this.isEnabled()) return;
+        if (this.isControlled() && !e.ctrlKey && !e.metaKey) {
+            const scrollZoomControl = new mapboxgl.ScrollZoomControl({closeButton: false}).setHTML();


A quick search for addControl suggests that AttributionControl is the only control automatically added to the map. Do you think this is best as an opt-in control the developer must add or as an opt-out control you'd have to remove?

+1 to this, I think its better to have an opt-in Control for this and not clutter the Map constructor params more. So it might be nice to add this in like this

const map = new mapboxgl.Map({..}); map.addControl(new mapboxgl.ZoomBlockerControl({}));

The reason I think this would be more ergonomic is because it would be easier for users to customize it. With something like this.

const map = new mapboxgl.Map({..}); const zoomBlocker = new mapboxgl.ScrollBlockerControl({..}) .setHTML(`<b>whatever content the user wants</b>`); map.addControl(zoomBlocker);

Then instead of having this logic here the blocking of the zoom event can be done in the onAdd method of the control, this way the rest of the codebase is unaware of the blocker control running.

rreusser · 2021-08-31T17:46:44Z

src/ui/control/scroll_zoom_control.js

+        const that = this;
+
+        if (this.options.closeFadeOut) {
+            setTimeout(() => {


Can we use CSS transitions to avoid a timing loop, e.g.:

...-container { opacity: 1; transition: opacity 300ms ease-in-out; }

and then either set the opacity directly or toggle a class as needed? (I don't think we have to worry about transitionend events since nothing happens at the end of the transition.)

arindam1993

thanks for starting this, this has been such a heavily requested feature and will be great addition.

My main suggestion is to go with making it opt-in and, trying to make the rest of the codebase unaware of it.

arindam1993 · 2021-08-31T18:04:59Z

src/ui/handler/scroll_zoom.js

@@ -154,6 +168,10 @@ class ScrollZoomHandler {

    wheel(e: WheelEvent) {
        if (!this.isEnabled()) return;
+        if (this.isControlled() && !e.ctrlKey && !e.metaKey) {
+            const scrollZoomControl = new mapboxgl.ScrollZoomControl({closeButton: false}).setHTML();


+1 to this, I think its better to have an opt-in Control for this and not clutter the Map constructor params more. So it might be nice to add this in like this

const map = new mapboxgl.Map({..}); map.addControl(new mapboxgl.ZoomBlockerControl({}));

The reason I think this would be more ergonomic is because it would be easier for users to customize it. With something like this.

const map = new mapboxgl.Map({..}); const zoomBlocker = new mapboxgl.ScrollBlockerControl({..}) .setHTML(`<b>whatever content the user wants</b>`); map.addControl(zoomBlocker);

Then instead of having this logic here the blocking of the zoom event can be done in the onAdd method of the control, this way the rest of the codebase is unaware of the blocker control running.

arindam1993 · 2021-08-31T18:05:35Z

src/ui/control/scroll_zoom_control.js

+ *     .setHTML("<h1>'Use ⌘ + scroll to zoom the map'</h1>")
+ *     .addTo(map);
+ */
+export default class ScrollZoomControl extends Evented {


Maybe a better name would be ScrollZoomBlockerControl ?

…ntrol.js, flow not working with event

… transition is flickering

ryanhamley

This is looking good. I had a few nits on the code, but I'm seeing some unexpected behavior when I use the debug page. I know this is a draft so apologies if you're already addressing these, but here's what I've noticed.

The text flickers just before the control fades out.
If you hold down CMD and really zoom in/out then let go of CMD, the warning pops up and the zoom ends. We should ensure that this control is deactivated for the length of the "zoom event", including inertia. There's no wheelend event or anything like that, but it looks like the scroll_zoom handler has some similar logic to handle successive events as a single "event".

ryanhamley · 2021-08-31T19:24:36Z

src/ui/control/scroll_zoom_control.js

+     * new mapboxgl.ScrollZoomControl().setHTML();
+     * map.addControl(scrollZoomControl);


Nit: should be const scrollZoomControl = new mapbox...

src/ui/control/scroll_zoom_control.js

…due to interference with fullscreen control

avpeery · 2021-09-09T16:38:32Z

@ryanhamley Ah, thanks for clarifying - I can see the bug now. In response to letting zoom happen while the warning is displayed, I have a new change (haven't pushed it yet) that shortens the time of the warning and the fade happens more quickly, so I think it'll be less in the way / make more sense to allow for zoom with CMD or CTRL while the warning is displayed.

… false in test

… other operating systems for corresponding alert

rreusser · 2021-09-13T18:46:43Z

src/css/mapbox-gl.css

+    display: flex;
+    align-items: center;
+    width: 100%;
+    height: 100%;


is height: 100% 100% of the height or is it 100% of the width? I thought it meant 100% of the width (even though it's the height property) and that you have to do bottom: 0 to get full height. Though maybe because it's absolute positioned, this does work.

Ah I did not know this, I originally had top: 0; left: 0; bottom: 0; right: 0; but based on a preliminary search it seems documented that height: 100% is 100% of the parent element height?

…default message

…mediately remove alert with ctrl/cmd key

…or what the method does

mourner

The behavior itself looks great and it's awesome that it's finally being addressed, but it seems like @arindam1993's suggestion to make this a Control had an unintended consequence of introducing more complexity that it could have if it were a scrollZoom handler option (as proposed e.g. in #10614):

It has to adhere to the Control interface, which relies on configurable corner positions, and for this PR led to creating a separate full-container position available for controls which feels like an awkward exception.
It adds the expectation of being highly configurable, with methods like add/remove/toggleClassName, setHTML with sanitization, etc., where this narrow interaction use case doesn't really require that flexibility.

So in the end it feels like there's more code in this PR than the use case needs. Personally I would prefer a more minimalistic approach, where:

There's just one unique class for the overlay without need to change it. Users will be able to use it for styling.
There's just one text for it defined in default_locale.js with the ability to change it via locale option like other labels.
There's only Ctrl for the behavior (not Cmd) so there's no need to use different labels for Mac and others, because Ctrl is universal.
Fade out delay and duration and whether the overlay is even visible can be fully controlled through CSS without any JS options.

This may feel like nitpicking, but it's hard to remove code like this once it's added, and configurability is overrated for "reasonable defaults" features like this, so I always try to minimize the amount of code where possible for new features.

avpeery · 2021-10-01T20:49:17Z

Closing this PR as #11029 is the preferred approach and has been merged into main.

avpeery added 2 commits August 24, 2021 09:39

initial commit to set up scroll zoom control with gesture handling ui

14d5fef

lint changes and added safari command key capability

01366a4

avpeery added the feature 🍏 label Aug 24, 2021

avpeery marked this pull request as draft August 24, 2021 17:57

Added blank space to debug file

e40c145

avpeery linked an issue Aug 24, 2021 that may be closed by this pull request

Enable pinch-to-zoom, disable scroll zooming #10227

Closed

avpeery added 10 commits August 24, 2021 12:28

removed option to set maxwidth on gesture handler. may be an option t…

dc1a380

…o add in later if makes sense

WIP: moving gesture handler to scroll zoom control

a52188e

Merge remote-tracking branch 'origin/main' into avpeery/pinch-to-zoom

cf7b29c

fixed issues with scroll zoom control transfer to controls from gestu…

951e6c0

…re handler

removed gesture_handler.js, replaced with scroll_zoom_control

46ca12c

implemented fade out, not compatible with flow

65d129e

fixed flow issue

f153868

updates to debug page

a0e5846

Removed unused code

81aa456

added debug page, moved scroll zoom control alert to be added for whe…

af17ab5

…el event if scroll zoom control is on

rreusser reviewed Aug 31, 2021

View reviewed changes

arindam1993 suggested changes Aug 31, 2021

View reviewed changes

avpeery added 4 commits August 31, 2021 11:12

moved add control to debug file

d5e8fa9

moved all scroll zoom control functionality to control/scroll_zoom_co…

2e72a3f

…ntrol.js, flow not working with event

removed hover opacity css and unused maxwidth default option, fadeout…

559bf53

… transition is flickering

added option to disable showing alert

7983e7f

ryanhamley reviewed Aug 31, 2021

View reviewed changes

avpeery added 7 commits August 31, 2021 15:33

removed close button option

2a21e82

Fixed fading transition for fadeout only

266d534

added manual testing page

81e7eba

added zoom_control.html to release pages

5f31156

fixed font size and improved debug page to scroll page

98df599

fixed appendChild node error

4a65a0e

Renamed css mapboxgl-ctrl-fullscreen to mapboxgl-ctrl-full-container …

93a6cb6

…due to interference with fullscreen control

avpeery added 2 commits September 9, 2021 10:53

shortened fade out transition

23fb2a4

removed commented out css

41a5bd5

SnailBones added this to the v2.5 milestone Sep 9, 2021

avpeery added 12 commits September 9, 2021 18:11

Added unit tests - WIP

1335336

added test to allows scroll zoom with cmd or ctrl keys are pressed

c8e6180

removed no options test, and showAlert options

06026d9

removed showAlert option in debug file, added showAlert option set to…

62afeb1

… false in test

removed required setHTML if using default alert, and check for mac vs…

c197452

… other operating systems for corresponding alert

fixes in the documentation

777262e

removed unneeded parent class evented

22ae662

fixed issue with onRemove, added classname tests and remove tests

3c044de

changed onremove to removecontrol for remove test

0649391

changed fade transition time

62c7b35

Merge remote-tracking branch 'origin/main' into avpeery/pinch-to-zoom

0d19102

add in windows/linux check, need to rethink feature support logic

94af585

asheemmamoowala removed this from the v2.5 milestone Sep 13, 2021

mocked window.navigator.platform for unit tests

263b05b

rreusser reviewed Sep 13, 2021

View reviewed changes

avpeery added 6 commits September 13, 2021 15:04

option with platorm detection and getMetaKeyName method

bb66815

removed metakey customization, CMD/CTRL + scroll to zoom the map for …

2940d30

…default message

adding in check this._map.isZooming() to scroll zoom prevent default

1f54def

added platform checked default messaging and improved css styling, im…

89af49e

…mediately remove alert with ctrl/cmd key

fixed lint issues

33d6b31

renamed _preventDefault to _handleScrollZoom as it makes more sense f…

92a900b

…or what the method does

mourner reviewed Sep 17, 2021

View reviewed changes

avpeery mentioned this pull request Sep 17, 2021

Add gestureHandling map option to implement scroll zoom blocker #11029

Merged

8 tasks

avpeery closed this Oct 1, 2021

avpeery deleted the avpeery/pinch-to-zoom branch October 5, 2022 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control scroll zoom and add gesture handler warning #10962

Control scroll zoom and add gesture handler warning #10962

avpeery commented Aug 24, 2021 •

edited

rreusser left a comment •

edited

rreusser Aug 31, 2021

rreusser Aug 31, 2021

arindam1993 Aug 31, 2021

rreusser Aug 31, 2021

arindam1993 left a comment

arindam1993 Aug 31, 2021

arindam1993 Aug 31, 2021

ryanhamley left a comment

ryanhamley Aug 31, 2021

avpeery commented Sep 9, 2021 •

edited

rreusser Sep 13, 2021

avpeery Sep 15, 2021

mourner left a comment

avpeery commented Oct 1, 2021

		* new mapboxgl.ScrollZoomControl().setHTML();
		* map.addControl(scrollZoomControl);

Control scroll zoom and add gesture handler warning #10962

Control scroll zoom and add gesture handler warning #10962

Conversation

avpeery commented Aug 24, 2021 • edited

Launch Checklist

rreusser left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arindam1993 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanhamley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avpeery commented Sep 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mourner left a comment

Choose a reason for hiding this comment

avpeery commented Oct 1, 2021

avpeery commented Aug 24, 2021 •

edited

rreusser left a comment •

edited

avpeery commented Sep 9, 2021 •

edited