Text and icon appear very small on image placeholders |
|||||||||||||||||
Issue descriptionText and icon appear very small on image placeholders on lite pages. See attached screenshots.
,
Nov 1
I'm not seeing the tiny icons in Stable channel, but I am seeing them in Beta channel (71.0.3578.31). I'll fix this and merge it into Beta.
,
Nov 1
Thanks, Scott!
,
Nov 1
This is super odd - on Android, the icons and text are shown shrunk by 2.5 times, but on Linux, the icons are shown normally. @schenney - Do you know what could have possibly changed in M70 here? I remember you wrote on https://chromium-review.googlesource.com/c/chromium/src/+/858288/ that Android and Mac use an odd zooming behavior, could this be related to that? I'm thinking of just fixing this by scaling the size of the icon and text up by 2.5x on Android, and merging that into Beta.
,
Nov 2
It was the case that Android and Mac were still using an explicit device-scale-factor to control pixel density effects, and then Android switched to using the browser zoom to achieve the same result in M-70. That change happened in https://chromium.googlesource.com/chromium/src.git/+/d0b8bbd2dbf9c06dcbbd01a4b1285cf7a1bc2ba0 so doesn't explain Stable and Beta differing. What's odd is that the change to zoom-for-dsf should make Android match Linux, and diverge from Mac. I don't even know what draws the text/icon in this case, but what I would do is modify the icon for now and leave the bug open so we can try to figure out the underlying cause. If you can add the info about where the icon lives I can follow up on locating the painting code that draws it.
,
Nov 2
I did some more digging, and it looks like the ratio that the icons are scaled by might depend on the DPR. When I found that the icons and text were ~2.5x smaller than they should be, that was on a Nexus 5X phone where the page had 2.625 DPR. I tried it out on an older phone where the page had 2.0 DPR, and the icons and text were 2x smaller than they should be, not ~2.5x. I could probably plumb the DPR down from a higher layer of Blink, but I still don't understand why this changed recently. schenney@, do you have any ideas? The icon lives here (https://cs.chromium.org/chromium/src/third_party/blink/public/default_100_percent/blink/placeholder_icon.png) and the painting code that draws it lives here (https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/graphics/placeholder_image.cc).
,
Nov 5
I also have no idea why it would have changed recently. I'm overloaded right now. Maybe fs@ could take a look, because I'm guessing the fix is reasonably simple?
,
Nov 5
I'm gonna see if I can remove the automatic Restrict-View-Google label by adding and removing the label manually.
,
Nov 5
Aaand I hope this removes the Restrict-View-Google label.
,
Nov 5
Argh, didn't work. @bshealy, do you have any idea on how to remove the automatically added Restrict-View-Google labe, so that fs@opera.com could take a look?
,
Nov 5
,
Nov 5
I'd suspect that schenney's comment from https://chromium-review.googlesource.com/c/chromium/src/+/858288/ was spot on, and that any observations of this "working" should be attributed. If it's indeed interesting to find a supposed point of change, then maybe try a bisect? (I won't be able to do anything before tomorrow, CET.) Generally, different "scalable" Images have different ways to handle these things - SVGImage have a wrapper that keeps the relevant scale factor - gradients (a GeneratedImage) store scaled/zoomed values. Wrapping PlaceholderImage too (like SVGImage) probably wouldn't be too difficult if needed.
,
Nov 6
So... what's the easy way to enable something like in the description? (For bisection.)
,
Nov 6
The easiest way to see the placeholders is to turn on Data Saver in the settings menu, and go to chrome://flags and set "override effective connection type" to 2G or slower.
,
Nov 7
Thanks! I haven't been able to reproduce any "good" (non-broken) behavior though...
,
Nov 7
I see the correct behavior in M70 in Stable, but the placeholders appear too small in 71.0.3578.31. Are you seeing the intended behavior in M70, or are you seeing the broken behaivor? I'm going to give bisecting a shot as well.
,
Nov 7
I've only seen the broken behavior.
,
Nov 8
I found the CL that introduced the problem: https://chromium-review.googlesource.com/c/chromium/src/+/1234014/, which turns on use-zoom-for-dsf by default. I'm going to see if I can find an easy way to fix this, and merge it back into M71.
,
Nov 10
I'm not sure how familiar you are with this code, so you've probably figured this out already. In any event, my first step would be to find some place in the call chain where the device_scale_factor is queried but not the zoom.
,
Dec 5
,
Dec 5
For context, the bug isn't bad enough to justify trying to merge this back into 71, but I will merge the fix back into M72 once it's landed: https://chromium-review.googlesource.com/c/chromium/src/+/1356265
,
Dec 6
This issue is marked as a release blocker with no OS labels associated. Please add an appropriate OS label. All release blocking issues should have OS labels associated to it, so that the issue can tracked and promptly verified, once it gets fixed. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 6
,
Dec 20
Reminder M72 Stable is coming soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If so, please make sure to land the fix and request a merge into the release branch ASAP. Thank you.
,
Dec 20
The fix is in-progress and should land soon.
,
Dec 27
Reminder M72 Stable is coming soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If so, please make sure to land the fix and request a merge into the release branch ASAP. Thank you.
,
Jan 2
Reminder M72 Stable is coming VERY soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If so, please make sure to land the fix and request a merge into the release branch ASAP. Thank you.
,
Jan 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/07f9b211a644068b4828e60b1ef764da756fdae0 commit 07f9b211a644068b4828e60b1ef764da756fdae0 Author: Scott Little <sclittle@chromium.org> Date: Fri Jan 04 22:07:12 2019 Scale placeholder images according to the page zoom factor. Before this CL, the icon and text inside placeholder images did not scale with the page zoom factor, which caused the icons and text within placeholder images to appear much smaller than it should when use-zoom-for-dsf is turned on. This CL fixes this by making the icons and text inside placeholder images scalable, and updating this scale factor for the icons and text whenever paint is invalidated on the enclosing LayoutImage. Bug: 900946 Change-Id: I85ca94d5fc7755dda172566c568665895badef87 Reviewed-on: https://chromium-review.googlesource.com/c/1356265 Reviewed-by: Philip Rogers <pdr@chromium.org> Reviewed-by: Stephen Chenney <schenney@chromium.org> Commit-Queue: Scott Little <sclittle@chromium.org> Cr-Commit-Position: refs/heads/master@{#620080} [modify] https://crrev.com/07f9b211a644068b4828e60b1ef764da756fdae0/third_party/blink/renderer/core/layout/layout_image_resource.cc [modify] https://crrev.com/07f9b211a644068b4828e60b1ef764da756fdae0/third_party/blink/renderer/core/paint/image_painter.cc [modify] https://crrev.com/07f9b211a644068b4828e60b1ef764da756fdae0/third_party/blink/renderer/core/style/style_fetched_image.cc [modify] https://crrev.com/07f9b211a644068b4828e60b1ef764da756fdae0/third_party/blink/renderer/core/style/style_fetched_image_set.cc [modify] https://crrev.com/07f9b211a644068b4828e60b1ef764da756fdae0/third_party/blink/renderer/platform/graphics/placeholder_image.cc [modify] https://crrev.com/07f9b211a644068b4828e60b1ef764da756fdae0/third_party/blink/renderer/platform/graphics/placeholder_image.h [modify] https://crrev.com/07f9b211a644068b4828e60b1ef764da756fdae0/third_party/blink/renderer/platform/graphics/placeholder_image_test.cc
,
Jan 4
Requesting merge into M72.
,
Jan 4
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review 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 7
How is the change looking in canary? Is it verified on canary and safe to merge?
,
Jan 7
This change is in Canary, I don't see any crashes that seem at all related to it. It should be safe to merge.
,
Jan 7
Approving merge to M72 branch 3626 based on comment #32. Please merge ASAP and mark bug as fixed. Thank you.
,
Jan 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9b2b0b659d6f99985f8aa1735d030d63e8d234fe Commit: 9b2b0b659d6f99985f8aa1735d030d63e8d234fe Author: sclittle@chromium.org Commiter: sclittle@chromium.org Date: 2019-01-07 18:12:58 +0000 UTC Scale placeholder images according to the page zoom factor. Before this CL, the icon and text inside placeholder images did not scale with the page zoom factor, which caused the icons and text within placeholder images to appear much smaller than it should when use-zoom-for-dsf is turned on. This CL fixes this by making the icons and text inside placeholder images scalable, and updating this scale factor for the icons and text whenever paint is invalidated on the enclosing LayoutImage. Bug: 900946 Change-Id: I85ca94d5fc7755dda172566c568665895badef87 Reviewed-on: https://chromium-review.googlesource.com/c/1356265 Reviewed-by: Philip Rogers <pdr@chromium.org> Reviewed-by: Stephen Chenney <schenney@chromium.org> Commit-Queue: Scott Little <sclittle@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#620080}(cherry picked from commit 07f9b211a644068b4828e60b1ef764da756fdae0) Reviewed-on: https://chromium-review.googlesource.com/c/1398307 Reviewed-by: Scott Little <sclittle@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#590} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Jan 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9b2b0b659d6f99985f8aa1735d030d63e8d234fe commit 9b2b0b659d6f99985f8aa1735d030d63e8d234fe Author: Scott Little <sclittle@chromium.org> Date: Mon Jan 07 18:12:58 2019 Scale placeholder images according to the page zoom factor. Before this CL, the icon and text inside placeholder images did not scale with the page zoom factor, which caused the icons and text within placeholder images to appear much smaller than it should when use-zoom-for-dsf is turned on. This CL fixes this by making the icons and text inside placeholder images scalable, and updating this scale factor for the icons and text whenever paint is invalidated on the enclosing LayoutImage. Bug: 900946 Change-Id: I85ca94d5fc7755dda172566c568665895badef87 Reviewed-on: https://chromium-review.googlesource.com/c/1356265 Reviewed-by: Philip Rogers <pdr@chromium.org> Reviewed-by: Stephen Chenney <schenney@chromium.org> Commit-Queue: Scott Little <sclittle@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#620080}(cherry picked from commit 07f9b211a644068b4828e60b1ef764da756fdae0) Reviewed-on: https://chromium-review.googlesource.com/c/1398307 Reviewed-by: Scott Little <sclittle@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#590} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/9b2b0b659d6f99985f8aa1735d030d63e8d234fe/third_party/blink/renderer/core/layout/layout_image_resource.cc [modify] https://crrev.com/9b2b0b659d6f99985f8aa1735d030d63e8d234fe/third_party/blink/renderer/core/paint/image_painter.cc [modify] https://crrev.com/9b2b0b659d6f99985f8aa1735d030d63e8d234fe/third_party/blink/renderer/core/style/style_fetched_image.cc [modify] https://crrev.com/9b2b0b659d6f99985f8aa1735d030d63e8d234fe/third_party/blink/renderer/core/style/style_fetched_image_set.cc [modify] https://crrev.com/9b2b0b659d6f99985f8aa1735d030d63e8d234fe/third_party/blink/renderer/platform/graphics/placeholder_image.cc [modify] https://crrev.com/9b2b0b659d6f99985f8aa1735d030d63e8d234fe/third_party/blink/renderer/platform/graphics/placeholder_image.h [modify] https://crrev.com/9b2b0b659d6f99985f8aa1735d030d63e8d234fe/third_party/blink/renderer/platform/graphics/placeholder_image_test.cc
,
Jan 7
|
|||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||
Comment 1 by robertogden@chromium.org
, Nov 1