Url bar hiding moves scroll position which breaks viewing wide images |
||||||
Issue descriptionChrome Version: 60.0.3088.0 OS: Android What steps will reproduce the problem? (1) Visit a wide image such as "Departure Herald": https://upload.wikimedia.org/wikipedia/commons/a/a0/Departure_Herald-Ming_Dynasty.jpg (2) Pinch+zoom in so that the image fills the screen (3) Scroll up and down a bit, notice the image disappears! The image is disappearing because the url bar changes the page's height, and the image is re-positioned to a very different position when this happens. What is the expected result? Url bar hiding should not change the image's scroll position. In other words, the image should stay in a fixed position on the screen. What happens instead? I have a video of the broken behavior: https://photos.google.com/share/AF1QipMzu8Kq0NzWQ3d_uhpMSVN7hzzreUU77uNb0XsZjn8Tg0HASJsV3mxV5csnsxJQSA?key=Uk5rMTFXWGJ2aW53a0gzQXVyellES0lfQU1CWnhR David, is this something you could look into to triage? I have not looked closely at the root cause. This could also be the recent image viewer changes (dfalcantara) or Steve's scroll anchoring work.
,
May 9 2017
Re #1, why downloads? This is an issue with ImageDocument.cpp This is WAI in a way. The image wants to be centered. When we hide/show the URL bar, the center shifts. It looks odd when you're zoomed in because the size of the "jump" scales as the user zooms in but it makes sense in terms that, when seen fully zoomed out, the image is centered. ImageDocument.cpp manually listens for viewport size changes and resizes the containing div based on the innerHeight which does change with the URL bar hiding. Who owns ImageDocument.cpp? That's where this needs to be changed.
,
May 11 2017
ping, dfalcantara@: do you own ImageDocument and have some spare time to fix this? Or do you know someone who might?
,
May 18 2017
,
May 28 2017
This was changed for the Downloads feature, which is now owned by dtrainor@.
,
May 29 2017
dtrainor@, any objections if I go ahead and remove the JS based resizing in ImageDocument.cpp?
,
Jul 17 2017
I've been looking into more scroll jumping issues and found this bug again. Were you able to make progress on this?
,
Jul 17 2017
Sorry for the delayed response. I'm okay with you removing that resizing.
,
Jul 17 2017
I haven't yet but I'll go ahead and make that change shortly.
,
Oct 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a38acc8557b35c676bd5c936db0e1187bfa4ee57 commit a38acc8557b35c676bd5c936db0e1187bfa4ee57 Author: David Bokan <bokan@chromium.org> Date: Fri Oct 06 15:35:42 2017 Don't recenter image document when URL bar hides This patch changes the ImageDocument so that hiding the URL bar doesn't attempt to vertically re-center an image that's wider than the viewport. Bug: 718515 Change-Id: I0b45beb011c2c8dbc6f095a0456774f6f5e041ff Reviewed-on: https://chromium-review.googlesource.com/700617 Reviewed-by: David Trainor <dtrainor@chromium.org> Commit-Queue: David Bokan <bokan@chromium.org> Cr-Commit-Position: refs/heads/master@{#507079} [modify] https://crrev.com/a38acc8557b35c676bd5c936db0e1187bfa4ee57/third_party/WebKit/Source/core/html/ImageDocument.cpp [modify] https://crrev.com/a38acc8557b35c676bd5c936db0e1187bfa4ee57/third_party/WebKit/Source/core/html/ImageDocumentTest.cpp [modify] https://crrev.com/a38acc8557b35c676bd5c936db0e1187bfa4ee57/third_party/WebKit/Source/core/testing/sim/SimRequest.cpp [modify] https://crrev.com/a38acc8557b35c676bd5c936db0e1187bfa4ee57/third_party/WebKit/Source/core/testing/sim/SimRequest.h
,
Oct 6 2017
...and by shortly I meant in 3 months. Anyway - should be fixed now.
,
Oct 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0400d0014243549fb4fd84d6d4af1db3fc8fb252 commit 0400d0014243549fb4fd84d6d4af1db3fc8fb252 Author: Henrik Grunell <grunell@chromium.org> Date: Mon Oct 09 15:06:42 2017 Revert "Don't recenter image document when URL bar hides" This reverts commit a38acc8557b35c676bd5c936db0e1187bfa4ee57. Reason for revert: breaks Android bot: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28Nexus4%29?numbuilds=200 http://crbug.com/772860 Original change's description: > Don't recenter image document when URL bar hides > > This patch changes the ImageDocument so that hiding the URL bar doesn't > attempt to vertically re-center an image that's wider than the viewport. > > Bug: 718515 > Change-Id: I0b45beb011c2c8dbc6f095a0456774f6f5e041ff > Reviewed-on: https://chromium-review.googlesource.com/700617 > Reviewed-by: David Trainor <dtrainor@chromium.org> > Commit-Queue: David Bokan <bokan@chromium.org> > Cr-Commit-Position: refs/heads/master@{#507079} TBR=bokan@chromium.org,dtrainor@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 718515 , 772860 Change-Id: I5d82ae26c29068597bf90b29dbddcd9a46ba153d Reviewed-on: https://chromium-review.googlesource.com/707075 Reviewed-by: Henrik Grunell <grunell@chromium.org> Commit-Queue: Henrik Grunell <grunell@chromium.org> Cr-Commit-Position: refs/heads/master@{#507376} [modify] https://crrev.com/0400d0014243549fb4fd84d6d4af1db3fc8fb252/third_party/WebKit/Source/core/html/ImageDocument.cpp [modify] https://crrev.com/0400d0014243549fb4fd84d6d4af1db3fc8fb252/third_party/WebKit/Source/core/html/ImageDocumentTest.cpp [modify] https://crrev.com/0400d0014243549fb4fd84d6d4af1db3fc8fb252/third_party/WebKit/Source/core/testing/sim/SimRequest.cpp [modify] https://crrev.com/0400d0014243549fb4fd84d6d4af1db3fc8fb252/third_party/WebKit/Source/core/testing/sim/SimRequest.h
,
Oct 10 2017
Thanks for fixing this! It is a real product excellence bug. Reopening due to revert.
,
Oct 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9fc521cbe571957d8d1d600797e090791ecdbb09 commit 9fc521cbe571957d8d1d600797e090791ecdbb09 Author: David Bokan <bokan@chromium.org> Date: Wed Oct 11 14:31:59 2017 [Reland] Don't recenter image document when URL bar hides This patch changes the ImageDocument so that hiding the URL bar doesn't attempt to vertically re-center an image that's wider than the viewport. This is a reland of r507079 which was reverted in r507376. That patch previously removed the early out in ImageCenteredAtDeviceScaleFactor test since it never runs with the "viewport enabled" setting. It seems that r502453 (which added the test) erred since the checks were never being run. Enabling them works on Desktop but fails on Android -- I'm not sure what the difference is -- but this would explain the early-out. I've now replaced it with a DISABLED macro on Android only. Bug: 718515 Change-Id: I40c5b628de3630889e4b5655e52b975f6924d291 TBR: dtrainor@chromium.org Reviewed-on: https://chromium-review.googlesource.com/710316 Commit-Queue: David Bokan <bokan@chromium.org> Reviewed-by: David Bokan <bokan@chromium.org> Cr-Commit-Position: refs/heads/master@{#507979} [modify] https://crrev.com/9fc521cbe571957d8d1d600797e090791ecdbb09/third_party/WebKit/Source/core/html/ImageDocument.cpp [modify] https://crrev.com/9fc521cbe571957d8d1d600797e090791ecdbb09/third_party/WebKit/Source/core/html/ImageDocumentTest.cpp [modify] https://crrev.com/9fc521cbe571957d8d1d600797e090791ecdbb09/third_party/WebKit/Source/core/testing/sim/SimRequest.cpp [modify] https://crrev.com/9fc521cbe571957d8d1d600797e090791ecdbb09/third_party/WebKit/Source/core/testing/sim/SimRequest.h
,
Feb 15 2018
,
Mar 26 2018
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by dfalcant...@chromium.org
, May 4 2017