New issue
Advanced search Search tips

Issue 919286 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome
Pri: 1
Type: Bug-Regression



Sign in to add a comment

PDF: Pinch zoom is changing the visual view-port and zooming into the document

Project Member Reported by dstockwell@google.com, Jan 5

Issue description

Chrome 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.
 
Cc: wjmaclean@chromium.org
Components: Internals>Input>Touch>Screen
Labels: -Pri-3 OS-Linux OS-Windows Pri-1
Owner: mcnee@chromium.org
Status: Assigned (was: Untriaged)
I can confirm this is broken.

It works in 71.0.3578.98, but it's broken in 72.0.3626.28.

I'll try bisecting further.
Cc: sunxd@chromium.org xidac...@chromium.org pdr@chromium.org
Labels: -Needs-Bisect
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
Owner: pdr@chromium.org
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
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.
Owner: mcnee@chromium.org
Status: Started (was: Assigned)
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?
Re c#7: Sounds good.
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Project Member

Comment 10 by bugdroid1@chromium.org, 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

Labels: Merge-Request-72
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.
Project Member

Comment 12 by sheriffbot@chromium.org, Jan 15

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
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
Thanks mcnee@ - how safe is this merge overall?have you already verified this in canary?
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.
Labels: -Merge-Review-72 Merge-Approved-72
Approving https://chromium-review.googlesource.com/c/1407488 for M72. Branch:3626
Project Member

Comment 16 by bugdroid1@chromium.org, Jan 16 (6 days ago)

Labels: -merge-approved-72 merge-merged-3626
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

Project Member

Comment 17 by cr-audit...@appspot.gserviceaccount.com, Jan 16 (6 days ago)

Labels: Merge-Merged-72-3626
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}

Comment 18 by mcnee@chromium.org, Jan 16 (6 days ago)

Status: Fixed (was: Started)

Sign in to add a comment