PDF: Pinch zoom is changing the visual view-port and zooming into the document |
||||||||||
Issue descriptionChrome Version : 72.0.3626.30 OS Version: 11316.35.0 What steps will reproduce the problem? 1. Open a pdf, eg. http://foersom.com/net/HowTo/data/OoPdfFormExample.pdf 2. Touch the document with two fingers, and pinch-zoom to make the document larger What is the expected result? Zooming into the document. What happens instead of that? Zooming into the document AND the page -- the toolbar gets larger and can move off screen. Appears this was modified in issue 786419 but appears to have regressed since then -- I verified that it works correctly on another ChromeOS device on stable M71.
,
Jan 7
Bisected to https://chromium.googlesource.com/chromium/src/+log/b26b9d891ad497c3928e9b7c04567100efe9bbdd..82cc4869d96f7345e42e3114b4bb12e6f5c9a368. Specifically, https://chromium.googlesource.com/chromium/src/+/82cc4869d96f7345e42e3114b4bb12e6f5c9a368 The issue goes away with --disable-blink-features=PaintTouchActionRects
,
Jan 7
,
Jan 11
I have a WIP CL which updates the PDF pinch tests so that they could have caught this regression: https://chromium-review.googlesource.com/c/chromium/src/+/1407488
,
Jan 12
Having the touch-action applied to html instead of just the #plugin embed seems to produce suitable touch action regions. It makes more sense to have the touch-action on html anyway, so I'll add this to my CL.
,
Jan 12
,
Jan 12
Took me a while to track down the chromeos devices but I am able to reproduce this issue. This is a regression from PaintTouchActionRects where we dropped plugin touch actions in some cases. I have a patch up using the testcase in comment #4 (https://chromium-review.googlesource.com/c/chromium/src/+/1408215) but I am worried about merging this into release branches. In testing this I noticed an edge-case bug where, if the user pinch-zooms on the toolbar itself, it will zoom in. I think change to move touch-action from #plugin to html will fix that. The css change is also safe to merge. What do you think of landing the css patch and merging it while the more risky PaintTouchActionRect regression fix rolls out with the regular release process?
,
Jan 14
Re c#7: Sounds good.
,
Jan 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1289c6b4d058c790f2d69a2f307e603e18e5cca0 commit 1289c6b4d058c790f2d69a2f307e603e18e5cca0 Author: Philip Rogers <pdr@chromium.org> Date: Mon Jan 14 16:14:56 2019 Touch action hit test display items should count as drawable content This patch updates PaintLayer::HasBoxDecorationsOrBackground to return true if touch-action rects are painted in the background. This ensures GraphicsLayer::DrawsContent is true if touch-action rects paint so that the GraphicsLayer is included when ScrollingCoordinator collects touch action rects in UpdateLayerTouchActionRects. This fixes a bug where an embeded object's touch action rects were skipped. A unit test has been added and this also passes the pdf extension unit test in: https://chromium-review.googlesource.com/c/chromium/src/+/1407488/1 Bug: 919286 Change-Id: I74f58722509dfb826f3e0ca7701f58cb35194fd0 Reviewed-on: https://chromium-review.googlesource.com/c/1408215 Reviewed-by: Xianda Sun <sunxd@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/master@{#622468} [modify] https://crrev.com/1289c6b4d058c790f2d69a2f307e603e18e5cca0/third_party/blink/renderer/core/paint/compositing/composited_layer_mapping_test.cc [modify] https://crrev.com/1289c6b4d058c790f2d69a2f307e603e18e5cca0/third_party/blink/renderer/core/paint/paint_layer.cc [modify] https://crrev.com/1289c6b4d058c790f2d69a2f307e603e18e5cca0/third_party/blink/renderer/core/paint/paint_layer.h
,
Jan 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eb2d65feb0063fd3cc30b0eac7eeadad5fc975cf commit eb2d65feb0063fd3cc30b0eac7eeadad5fc975cf Author: Kevin McNee <mcnee@chromium.org> Date: Mon Jan 14 19:46:24 2019 PDF Viewer: Fix use of touch-action PaintTouchActionRects appears to have regressed touch-action with respect to the plugin element. However, touch-action still works if applied to the document element. Since it makes sense to have the touch-action on the document element for the PDF viewer anyway, we do so here. We also update the PDF pinch browser tests to check that the page scale doesn't change. The tests ensured that the viewer implements its own pinch zoom mechanism, but they did not test that the browser's native pinch zoom was prevented. We also lacked an end-to-end test for the touchscreen case. Bug: 919286 Change-Id: If9eb60031375cfd65d45eed8c56bb01e884fa8a2 Reviewed-on: https://chromium-review.googlesource.com/c/1407488 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: dsinclair <dsinclair@chromium.org> Commit-Queue: Kevin McNee <mcnee@chromium.org> Cr-Commit-Position: refs/heads/master@{#622554} [modify] https://crrev.com/eb2d65feb0063fd3cc30b0eac7eeadad5fc975cf/chrome/browser/pdf/pdf_extension_test.cc [modify] https://crrev.com/eb2d65feb0063fd3cc30b0eac7eeadad5fc975cf/chrome/browser/resources/pdf/index.css [modify] https://crrev.com/eb2d65feb0063fd3cc30b0eac7eeadad5fc975cf/chrome/test/data/pdf/gesture_detector_test.js [modify] https://crrev.com/eb2d65feb0063fd3cc30b0eac7eeadad5fc975cf/content/public/test/browser_test_utils.cc [modify] https://crrev.com/eb2d65feb0063fd3cc30b0eac7eeadad5fc975cf/content/public/test/browser_test_utils.h
,
Jan 15
Requesting merge of https://chromium.googlesource.com/chromium/src/+/eb2d65feb0063fd3cc30b0eac7eeadad5fc975cf to M72. Impact: Performing a touchscreen pinch to zoom a PDF will also zoom the ui elements (like the toolbar), causing them to go offscreen. Also, the gesture is not smooth.
,
Jan 15
This bug requires manual review: We are only 13 days from stable. Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 15
Thanks mcnee@ - how safe is this merge overall?have you already verified this in canary?
,
Jan 15
My CL is quite simple (just a PDF viewer specific css change). There shouldn't be any safety issues here. Note that pdr@ has a CL which addresses the underlying bug more generally, but as mentioned in c#7, that one may be risky to merge. My CL addresses the issue by simply making it so that the underlying bug no longer affects the PDF viewer. I've confirmed the behaviour is now correct on Windows canary, but note that I couldn't test my CL's effect in isolation on canary, as pdr@'s CL has also landed. Either of these CLs would fix the issue, but we only want to merge mine, since it's the safe change.
,
Jan 15
Approving https://chromium-review.googlesource.com/c/1407488 for M72. Branch:3626
,
Jan 16
(6 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9503f60d355eb3e6885189c56f38ff40a968b112 commit 9503f60d355eb3e6885189c56f38ff40a968b112 Author: Kevin McNee <mcnee@chromium.org> Date: Wed Jan 16 16:24:49 2019 M72: PDF Viewer: Fix use of touch-action TBR=dsinclair@chromium.org,avi@chromium.org PDF Viewer: Fix use of touch-action PaintTouchActionRects appears to have regressed touch-action with respect to the plugin element. However, touch-action still works if applied to the document element. Since it makes sense to have the touch-action on the document element for the PDF viewer anyway, we do so here. We also update the PDF pinch browser tests to check that the page scale doesn't change. The tests ensured that the viewer implements its own pinch zoom mechanism, but they did not test that the browser's native pinch zoom was prevented. We also lacked an end-to-end test for the touchscreen case. (cherry picked from commit eb2d65feb0063fd3cc30b0eac7eeadad5fc975cf) Bug: 919286 Change-Id: If9eb60031375cfd65d45eed8c56bb01e884fa8a2 Reviewed-on: https://chromium-review.googlesource.com/c/1407488 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: dsinclair <dsinclair@chromium.org> Commit-Queue: Kevin McNee <mcnee@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#622554} Reviewed-on: https://chromium-review.googlesource.com/c/1413785 Reviewed-by: Kevin McNee <mcnee@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#713} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/9503f60d355eb3e6885189c56f38ff40a968b112/chrome/browser/pdf/pdf_extension_test.cc [modify] https://crrev.com/9503f60d355eb3e6885189c56f38ff40a968b112/chrome/browser/resources/pdf/index.css [modify] https://crrev.com/9503f60d355eb3e6885189c56f38ff40a968b112/chrome/test/data/pdf/gesture_detector_test.js [modify] https://crrev.com/9503f60d355eb3e6885189c56f38ff40a968b112/content/public/test/browser_test_utils.cc [modify] https://crrev.com/9503f60d355eb3e6885189c56f38ff40a968b112/content/public/test/browser_test_utils.h
,
Jan 16
(6 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9503f60d355eb3e6885189c56f38ff40a968b112 Commit: 9503f60d355eb3e6885189c56f38ff40a968b112 Author: mcnee@chromium.org Commiter: mcnee@chromium.org Date: 2019-01-16 16:24:49 +0000 UTC M72: PDF Viewer: Fix use of touch-action TBR=dsinclair@chromium.org,avi@chromium.org PDF Viewer: Fix use of touch-action PaintTouchActionRects appears to have regressed touch-action with respect to the plugin element. However, touch-action still works if applied to the document element. Since it makes sense to have the touch-action on the document element for the PDF viewer anyway, we do so here. We also update the PDF pinch browser tests to check that the page scale doesn't change. The tests ensured that the viewer implements its own pinch zoom mechanism, but they did not test that the browser's native pinch zoom was prevented. We also lacked an end-to-end test for the touchscreen case. (cherry picked from commit eb2d65feb0063fd3cc30b0eac7eeadad5fc975cf) Bug: 919286 Change-Id: If9eb60031375cfd65d45eed8c56bb01e884fa8a2 Reviewed-on: https://chromium-review.googlesource.com/c/1407488 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: dsinclair <dsinclair@chromium.org> Commit-Queue: Kevin McNee <mcnee@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#622554} Reviewed-on: https://chromium-review.googlesource.com/c/1413785 Reviewed-by: Kevin McNee <mcnee@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#713} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Jan 16
(6 days ago)
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by mcnee@chromium.org
, Jan 7Components: Internals>Input>Touch>Screen
Labels: -Pri-3 OS-Linux OS-Windows Pri-1
Owner: mcnee@chromium.org
Status: Assigned (was: Untriaged)