New issue
Advanced search Search tips

Issue 725970 link

Starred by 8 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 27
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug



Sign in to add a comment

Trackpad Pinch zoom on Mac no longer works in webviews

Project Member Reported by birunt...@mohanathas.com, May 24 2017

Issue description

See test case here: https://gist.github.com/anonymous/697b715f7abb3539b3d4980eeeb5c8d0

Or just run:

  git clone https://gist.github.com/697b715f7abb3539b3d4980eeeb5c8d0.git webview-pinch-zoom
  cd webview-pinch-zoom
  /path/to/chrome --load-and-launch-app=$PWD

Pinch zooming over the Google Maps webview should zoom the page on macOS, but this no longer works. This worked in Chromium 56 and regressed in either 57 or 58.
 
Cc: wjmaclean@chromium.org mcnee@chromium.org
Owner: wjmaclean@chromium.org
Just to be clear, this is trackpad pinch-zoom, right?

WebView doesn't actually accept browser-based GesturePinch events. In the case of touch-screen, maps.google.com has JS that processes touch events to create zoom.

So if this is not working on Mac, it's likely the case that some other event type is failing to be transmitted to the WebView ...

We'll triage this to see what's happening.


>Just to be clear, this is trackpad pinch-zoom, right?

Yep!
Labels: Needs-Feedback
I tried this with M56 stable (56.0.2924.87) and trackpad pinch-zoom did not work on this version. Given the following code I don't think I ever would have expected it to work:

https://cs.chromium.org/chromium/src/extensions/browser/app_window/app_web_contents_helper.cc?rcl=6552fc43089472109de62975dbb91553d7dc19df&l=36

I.e. we don't send GesturePinch events (touchscreen *or* trackpad) to apps.

Can you give me an *exact* version number where this works?
Labels: -Needs-Feedback
We actually discovered this issue in Electron after the Chromium 58 upgrade: https://github.com/electron/electron/issues/9578

Usually issues in the Electron <webview> are upstream Chromium issues and shared by the Chrome Apps <webview>. I seem to have incorrectly assumed that to be the case here.

We haven't overridden PreHandleGestureEvent in Electron, but I just tried a build with the following just to be sure:

  bool PreHandleGestureEvent(
      content::WebContents* source,
      const blink::WebGestureEvent& event) override { return false; }

In Electron, the pinch zoom gesture zooms the entire window even when it is over a webview. It seems like it is not being routed to the webview render process. In the previous version of Electron based on Chromium 56, it zoomed the webview.

I'm not sure if this is a Chromium regression or if there has e.g. been some API change between Chromium 56 and Chromium 58 that Electron hasn't taken into consideration.
Ahhhh, ok, then that makes more sense.

Could you try reverting the following CL on your local branch: https://codereview.chromium.org/2745153002/

My hypothesis is this: since your branch allows GesturePinch events to go to the app, they go to the embedder's renderer, and there they encounter 

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebViewImpl.cpp?rcl=d4d25fc7cc5cebdee0a46b5f679c7a0fe9732c96&l=963

WebViewImpl::HandleSyntheticWheelFromTouchpadPinchEvent()

This exists to give pages the option of disabling touchpad pinch via a wheel handler ... if this function marks the synthetic wheel event as consumed (which BrowserPlugin *used to* but no longer does, then that would cancel the pinch event.

Cancelling the pinch event is *exactly* what maps wants to do, so it can replace the generic browser-pinch-zoom experience, which zooms the entire page, with their own custom pinch-zoom that only zooms the map content and not the UI elements.

If I'm correct, there are two possibilities here. The safest would be to route the GesturePinchEvents via RenderWidgetHostInputEventRouter, in which case maps' touch handler would get access to the synthetic mousewheels in the normal way. (At present Mac always sends GesturePinch events to the top-level renderer ...) The second possibility is to have BrowserPlugin detect if the guest has a wheel event handler, and if it does it should forward the wheels to the guest while reporting to the embedder it consumed them. But this is problematic in that if the guest's page *doesn't* consume the wheel, then we've incorrectly reported them as consumed.
Cc: -kbr@chromium.org
>Could you try reverting the following CL on your local branch: https://codereview.chromium.org/2745153002/

Yep, reverting that fixed the issue. We'll revert it Electron for now, thank you!
Just reverting this change is not the ultimate solution ... if you refer to the bug underlying the CL you see it fixes another problem.

We should consider routing the GesturePinch events from RenderWidgetHostViewMac to get a solution that can be upstreamed.
Summary: Trackpad Pinch zoom on Mac no longer works in webviews (was: Pinch zoom no longer works in webviews)
FYI, we landed a change (https://chromium-review.googlesource.com/c/chromium/src/+/559650) that routes gesture pinch events on Mac, so you may be able to undo your local work-around on Electron and get the desired behaviour now.
Status: Assigned (was: Untriaged)
Owner: mcnee@chromium.org
Now that we have  crbug.com/739213#c7  , I think we can get the desired behaviour here as well.
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86

commit be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86
Author: Kevin McNee <mcnee@chromium.org>
Date: Thu Aug 23 15:10:26 2018

Consistent touchpad pinch behaviour for app windows and webviews

Currently, app windows (possibly containing webviews):
1) suppress all pinch events which also suppresses the
   synthetic wheel event, but not touchscreen touch events, and
2) allow "smart zoom" on Mac which can change the page scale even
   though other means of changing the page scale are suppressed.

As a result of (1), if we have, say, google maps in a webview, a
user can pinch zoom with a touchscreen, but not with a touchpad.

We now suppress gesture events to app windows based on whether they
cause a scale change. So now touchscreen and touchpad pinching are
consistent since the page has access to the events so that it may
implement custom pinch zoom behaviour, but unhandled events still
do not result in a scale change for the app window. Also, smart
zoom is suppressed to be consistent with other touchpad pinching.

Bug:  725970 ,  874132 
Change-Id: I03dd2048002d69dc7c8a822fc727140c67d64706
Reviewed-on: https://chromium-review.googlesource.com/1174933
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585489}
[modify] https://crrev.com/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86/chrome/browser/apps/platform_apps/app_browsertest.cc
[add] https://crrev.com/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86/chrome/test/data/extensions/platform_apps/touchpad_pinch/background.js
[add] https://crrev.com/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86/chrome/test/data/extensions/platform_apps/touchpad_pinch/manifest.json
[add] https://crrev.com/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86/chrome/test/data/extensions/platform_apps/touchpad_pinch/window.html
[add] https://crrev.com/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86/chrome/test/data/extensions/platform_apps/touchpad_pinch/window.js
[add] https://crrev.com/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/embedder.html
[add] https://crrev.com/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/embedder.js
[add] https://crrev.com/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/guest.html
[add] https://crrev.com/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/guest.js
[add] https://crrev.com/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/manifest.json
[add] https://crrev.com/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/test.js
[modify] https://crrev.com/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86/components/guest_view/browser/guest_view_base.cc
[modify] https://crrev.com/be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86/extensions/browser/app_window/app_web_contents_helper.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Aug 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8555b051b42e9df3700f80d3c5c2530ab3c8f924

commit 8555b051b42e9df3700f80d3c5c2530ab3c8f924
Author: Scott Little <sclittle@chromium.org>
Date: Thu Aug 23 20:58:42 2018

Revert "Consistent touchpad pinch behaviour for app windows and webviews"

This reverts commit be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86.

Reason for revert: Looks like this caused a test to start flaking, https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-rel

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-rel/12367

Original change's description:
> Consistent touchpad pinch behaviour for app windows and webviews
> 
> Currently, app windows (possibly containing webviews):
> 1) suppress all pinch events which also suppresses the
>    synthetic wheel event, but not touchscreen touch events, and
> 2) allow "smart zoom" on Mac which can change the page scale even
>    though other means of changing the page scale are suppressed.
> 
> As a result of (1), if we have, say, google maps in a webview, a
> user can pinch zoom with a touchscreen, but not with a touchpad.
> 
> We now suppress gesture events to app windows based on whether they
> cause a scale change. So now touchscreen and touchpad pinching are
> consistent since the page has access to the events so that it may
> implement custom pinch zoom behaviour, but unhandled events still
> do not result in a scale change for the app window. Also, smart
> zoom is suppressed to be consistent with other touchpad pinching.
> 
> Bug:  725970 ,  874132 
> Change-Id: I03dd2048002d69dc7c8a822fc727140c67d64706
> Reviewed-on: https://chromium-review.googlesource.com/1174933
> Reviewed-by: Ben Wells <benwells@chromium.org>
> Reviewed-by: James MacLean <wjmaclean@chromium.org>
> Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
> Commit-Queue: Kevin McNee <mcnee@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#585489}

TBR=benwells@chromium.org,ekaramad@chromium.org,wjmaclean@chromium.org,mcnee@chromium.org

Change-Id: I3a717517c60bbe22bbb3b93c64bea107ef191a5a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  725970 ,  874132 
Reviewed-on: https://chromium-review.googlesource.com/1187167
Reviewed-by: Scott Little <sclittle@chromium.org>
Commit-Queue: Scott Little <sclittle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585611}
[modify] https://crrev.com/8555b051b42e9df3700f80d3c5c2530ab3c8f924/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/8555b051b42e9df3700f80d3c5c2530ab3c8f924/chrome/browser/apps/platform_apps/app_browsertest.cc
[delete] https://crrev.com/529b158da2b374d372bdf69b42d06910585c19f4/chrome/test/data/extensions/platform_apps/touchpad_pinch/background.js
[delete] https://crrev.com/529b158da2b374d372bdf69b42d06910585c19f4/chrome/test/data/extensions/platform_apps/touchpad_pinch/manifest.json
[delete] https://crrev.com/529b158da2b374d372bdf69b42d06910585c19f4/chrome/test/data/extensions/platform_apps/touchpad_pinch/window.html
[delete] https://crrev.com/529b158da2b374d372bdf69b42d06910585c19f4/chrome/test/data/extensions/platform_apps/touchpad_pinch/window.js
[delete] https://crrev.com/529b158da2b374d372bdf69b42d06910585c19f4/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/embedder.html
[delete] https://crrev.com/529b158da2b374d372bdf69b42d06910585c19f4/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/embedder.js
[delete] https://crrev.com/529b158da2b374d372bdf69b42d06910585c19f4/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/guest.html
[delete] https://crrev.com/529b158da2b374d372bdf69b42d06910585c19f4/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/guest.js
[delete] https://crrev.com/529b158da2b374d372bdf69b42d06910585c19f4/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/manifest.json
[delete] https://crrev.com/529b158da2b374d372bdf69b42d06910585c19f4/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/test.js
[modify] https://crrev.com/8555b051b42e9df3700f80d3c5c2530ab3c8f924/components/guest_view/browser/guest_view_base.cc
[modify] https://crrev.com/8555b051b42e9df3700f80d3c5c2530ab3c8f924/extensions/browser/app_window/app_web_contents_helper.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Aug 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f7610825f03ca092acbf312b476a60c167671d97

commit f7610825f03ca092acbf312b476a60c167671d97
Author: Kevin McNee <mcnee@chromium.org>
Date: Fri Aug 24 15:52:47 2018

Reland "Consistent touchpad pinch behaviour for app windows and webviews"

This is a reland of be51aadb8aa5b5c7ee900c3c2721e69ec6b2ea86

Original change's description:
> Consistent touchpad pinch behaviour for app windows and webviews
> 
> Currently, app windows (possibly containing webviews):
> 1) suppress all pinch events which also suppresses the
>    synthetic wheel event, but not touchscreen touch events, and
> 2) allow "smart zoom" on Mac which can change the page scale even
>    though other means of changing the page scale are suppressed.
> 
> As a result of (1), if we have, say, google maps in a webview, a
> user can pinch zoom with a touchscreen, but not with a touchpad.
> 
> We now suppress gesture events to app windows based on whether they
> cause a scale change. So now touchscreen and touchpad pinching are
> consistent since the page has access to the events so that it may
> implement custom pinch zoom behaviour, but unhandled events still
> do not result in a scale change for the app window. Also, smart
> zoom is suppressed to be consistent with other touchpad pinching.
> 
> Bug:  725970 ,  874132 
> Change-Id: I03dd2048002d69dc7c8a822fc727140c67d64706
> Reviewed-on: https://chromium-review.googlesource.com/1174933
> Reviewed-by: Ben Wells <benwells@chromium.org>
> Reviewed-by: James MacLean <wjmaclean@chromium.org>
> Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
> Commit-Queue: Kevin McNee <mcnee@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#585489}

Bug:  725970 ,  874132 
Change-Id: I23420821c1166f7b641faf672798ddac0e928ad9
Reviewed-on: https://chromium-review.googlesource.com/1187345
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585856}
[modify] https://crrev.com/f7610825f03ca092acbf312b476a60c167671d97/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/f7610825f03ca092acbf312b476a60c167671d97/chrome/browser/apps/platform_apps/app_browsertest.cc
[add] https://crrev.com/f7610825f03ca092acbf312b476a60c167671d97/chrome/test/data/extensions/platform_apps/touchpad_pinch/background.js
[add] https://crrev.com/f7610825f03ca092acbf312b476a60c167671d97/chrome/test/data/extensions/platform_apps/touchpad_pinch/manifest.json
[add] https://crrev.com/f7610825f03ca092acbf312b476a60c167671d97/chrome/test/data/extensions/platform_apps/touchpad_pinch/window.html
[add] https://crrev.com/f7610825f03ca092acbf312b476a60c167671d97/chrome/test/data/extensions/platform_apps/touchpad_pinch/window.js
[add] https://crrev.com/f7610825f03ca092acbf312b476a60c167671d97/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/embedder.html
[add] https://crrev.com/f7610825f03ca092acbf312b476a60c167671d97/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/embedder.js
[add] https://crrev.com/f7610825f03ca092acbf312b476a60c167671d97/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/guest.html
[add] https://crrev.com/f7610825f03ca092acbf312b476a60c167671d97/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/guest.js
[add] https://crrev.com/f7610825f03ca092acbf312b476a60c167671d97/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/manifest.json
[add] https://crrev.com/f7610825f03ca092acbf312b476a60c167671d97/chrome/test/data/extensions/platform_apps/web_view/touchpad_pinch/test.js
[modify] https://crrev.com/f7610825f03ca092acbf312b476a60c167671d97/components/guest_view/browser/guest_view_base.cc
[modify] https://crrev.com/f7610825f03ca092acbf312b476a60c167671d97/extensions/browser/app_window/app_web_contents_helper.cc

Status: Fixed (was: Assigned)
The synthetic wheels of a touchpad pinch are now offered to webviews. If the pinch is not consumed, it is up to the embedder whether the page scale changes.

Sign in to add a comment