New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 764713 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Regression



Sign in to add a comment

ImageDocument zoom-in position is wrong on high-dpi devices

Reported by henriton...@gmail.com, Sep 13 2017

Issue description

UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36

Steps to reproduce the problem:
1. on high dpi screen, navigate to an image that is bigger than the (virtual) display area
1b. eg https://upload.wikimedia.org/wikipedia/commons/e/e9/Cathedral_rock_sedona_arizona_2.jpg)
2. click on area of image to zoom, especially bottom-right quarter

What is the expected behavior?
should zoom to native resolution centered on the area that was clicked

What went wrong?
zoom area is centered on an area offset to the top-left. This is probably because of a calculation that does not take into account the fact that the image is displayed at a scaling factor because of the high-dpi setting.

Did this work before? No 

Chrome version: 60.0.3112.113  Channel: stable
OS Version: 
Flash Version: Shockwave Flash 26.0 r0

The zoomed-in image is not displayed at the native resolution of the screen, but 2x (or whatever scaling factor). Maybe this is the real bug. In ImageDocument mode, we are not concerned with its relationship with other ui elements, but we want to see the image at native resolution, so maybe scaling should be disabled. However, small images will look very small. But Large images will look perfect.
Relevant source: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/ImageDocument.cpp
 
chrome_zoom1.jpg
844 KB View Download
chrome_zoom2.jpg
647 KB View Download
Labels: Needs-Triage-M60
Cc: sc00335...@techmahindra.com
Components: -UI UI>HighDPI
Labels: -Type-Bug -Pri-2 Triaged-ET M-63 hasbisect Pri-1 Type-Bug-Regression
Owner: malaykeshav@chromium.org
Status: Assigned (was: Unconfirmed)
Tested the issue on reported version 60.0.3112.113, latest stable 61.0.3163.79 and latest canary 63.0.3215.0 using Ubuntu 14.04 with scaling factor as 1.38 and is reproducible.

Manual Bisect Info:
===============
Good Build: 55.0.2877.0
Bad Build: 55.0.2878.0

NOTE: Don't have per revision bisect set up on Linux as of now, hence updating the chromuim bisect

You are probably looking for a change made after 422259 (known good), but no later than 422277 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/575f82957d9e1780440a7cb4cca6c326c1942d10..fb7c88eb2f4e2216a244ae78142cf2097c8f1076

Suspecting https://codereview.chromium.org/2363553005 from the changelog.

@malaykeshav: Please confirm the behaviour and help in assigning to appropriate owner if it has nothing to do with your change.

Thanks!
Cc: osh...@chromium.org
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 15 2017

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

commit a6033b857861ce76581f535c97d122710c405eee
Author: F#m <malaykeshav@chromium.org>
Date: Fri Sep 15 02:56:44 2017

Match scroll offset for ImageDocument when setting actual size

The scroll offset does not take the device scale factor into
consideration when setting the actual size of the document. This leads
to an incorrect centering of the image. This patch fixes this issue.

Updates unit tests to add this case.

Bug:  764713 
Change-Id: Ica415bcaaed8084435af9d326d2bec362f1eee2e
Component: ImageDocument, Zoom, Device Scale Factor
Reviewed-on: https://chromium-review.googlesource.com/667879
Commit-Queue: Malay Keshav <malaykeshav@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502145}
[modify] https://crrev.com/a6033b857861ce76581f535c97d122710c405eee/third_party/WebKit/Source/core/html/ImageDocument.cpp
[modify] https://crrev.com/a6033b857861ce76581f535c97d122710c405eee/third_party/WebKit/Source/core/html/ImageDocumentTest.cpp

Status: Fixed (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 15 2017

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

commit b534d3dcc4db4de5d6436552b3c8c1826b8f811a
Author: Max Morin <maxmorin@chromium.org>
Date: Fri Sep 15 08:30:25 2017

Revert "Match scroll offset for ImageDocument when setting actual size"

This reverts commit a6033b857861ce76581f535c97d122710c405eee.

Reason for revert: ImageDocumentTests test failing on Android: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28Nexus4%29/builds/69452

I   96.348s run_tests_on_device(0273818f0c5cabb6)  ../../third_party/WebKit/Source/core/html/ImageDocumentTest.cpp:219: Failure
I   96.348s run_tests_on_device(0273818f0c5cabb6)        Expected: 22
I   96.348s run_tests_on_device(0273818f0c5cabb6)  To be equal to: offset.Width()
I   96.348s run_tests_on_device(0273818f0c5cabb6)        Which is: 20
I   96.348s run_tests_on_device(0273818f0c5cabb6)  ../../third_party/WebKit/Source/core/html/ImageDocumentTest.cpp:220: Failure
I   96.348s run_tests_on_device(0273818f0c5cabb6)        Expected: 42
I   96.348s run_tests_on_device(0273818f0c5cabb6)  To be equal to: offset.Height()
I   96.348s run_tests_on_device(0273818f0c5cabb6)        Which is: 20
I   96.348s run_tests_on_device(0273818f0c5cabb6)  ../../third_party/WebKit/Source/core/html/ImageDocumentTest.cpp:231: Failure
I   96.348s run_tests_on_device(0273818f0c5cabb6)        Expected: 22
I   96.348s run_tests_on_device(0273818f0c5cabb6)  To be equal to: offset.Height()
I   96.348s run_tests_on_device(0273818f0c5cabb6)        Which is: 20
I   96.348s run_tests_on_device(0273818f0c5cabb6)  [  FAILED  ] ImageDocumentTest.ImageCenteredAtDeviceScaleFactor (15 ms)


Original change's description:
> Match scroll offset for ImageDocument when setting actual size
> 
> The scroll offset does not take the device scale factor into
> consideration when setting the actual size of the document. This leads
> to an incorrect centering of the image. This patch fixes this issue.
> 
> Updates unit tests to add this case.
> 
> Bug:  764713 
> Change-Id: Ica415bcaaed8084435af9d326d2bec362f1eee2e
> Component: ImageDocument, Zoom, Device Scale Factor
> Reviewed-on: https://chromium-review.googlesource.com/667879
> Commit-Queue: Malay Keshav <malaykeshav@chromium.org>
> Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#502145}

TBR=wangxianzhu@chromium.org,malaykeshav@chromium.org

Change-Id: Ib572b45fa88572f5214b9084f05e2d100ea720d8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  764713 
Reviewed-on: https://chromium-review.googlesource.com/667337
Reviewed-by: Max Morin <maxmorin@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502205}
[modify] https://crrev.com/b534d3dcc4db4de5d6436552b3c8c1826b8f811a/third_party/WebKit/Source/core/html/ImageDocument.cpp
[modify] https://crrev.com/b534d3dcc4db4de5d6436552b3c8c1826b8f811a/third_party/WebKit/Source/core/html/ImageDocumentTest.cpp

Status: Started (was: Fixed)

Comment 8 by donnd@google.com, Sep 15 2017

Issue 765755 has been merged into this issue.
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 16 2017

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

commit b346b061e76751039272dc58f4c69dbe839da178
Author: F#m <malaykeshav@chromium.org>
Date: Sat Sep 16 00:39:35 2017

Match scroll offset for ImageDocument when setting actual size

This is a reland for 
https://chromium-review.googlesource.com/c/chromium/src/+/667879

The scroll offset does not take the device scale factor into
consideration when setting the actual size of the document. This leads
to an incorrect centering of the image. This patch fixes this issue.

Updates unit tests to add this case.

Bug:  764713 
Change-Id: I4195a05d6d5101cc1242617db2cea9519237d590
Component: ImageDocument, Zoom, Device Scale Factor
Reviewed-on: https://chromium-review.googlesource.com/669156
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502453}
[modify] https://crrev.com/b346b061e76751039272dc58f4c69dbe839da178/third_party/WebKit/Source/core/html/ImageDocument.cpp
[modify] https://crrev.com/b346b061e76751039272dc58f4c69dbe839da178/third_party/WebKit/Source/core/html/ImageDocumentTest.cpp

Status: Fixed (was: Started)

Sign in to add a comment