Trackpad Pinch zoom on Mac no longer works in webviews |
||||||||
Issue descriptionSee 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.
,
May 24 2017
>Just to be clear, this is trackpad pinch-zoom, right? Yep!
,
May 24 2017
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?
,
May 25 2017
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.
,
May 25 2017
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.
,
May 25 2017
>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!
,
May 26 2017
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.
,
Jul 17 2017
,
Oct 26 2017
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.
,
Aug 1
,
Aug 14
Now that we have crbug.com/739213#c7 , I think we can get the desired behaviour here as well.
,
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
,
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
,
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
,
Aug 27
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 |
||||||||
Comment 1 by wjmaclean@chromium.org
, May 24 2017Owner: wjmaclean@chromium.org