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

Issue 900946 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Text and icon appear very small on image placeholders

Project Member Reported by bshealy@google.com, Nov 1

Issue description

Text and icon appear very small on image placeholders on lite pages. See attached screenshots.




 
Screen Shot 2018-11-01 at 8.00.55 AM.png
189 KB View Download
Screen Shot 2018-11-01 at 8.05.18 AM.png
217 KB View Download
Cc: sclit...@chromium.org
Components: Blink>Previews
Labels: -Pri-3 M-71 Pri-1
Owner: sclit...@chromium.org
Status: Started (was: Untriaged)
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.
Thanks, Scott!
Cc: schenney@chromium.org
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.
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.
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).
Cc: -bengr@google.com f...@opera.com
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? 
Labels: Restrict-View-Google
I'm gonna see if I can remove the automatic Restrict-View-Google label by adding and removing the label manually.
Labels: -Restrict-View-Google
Aaand I hope this removes the Restrict-View-Google label.
Cc: bshealy@google.com
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?
Labels: allpublic
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.
So... what's the easy way to enable something like in the description? (For bisection.)
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.
Thanks! I haven't been able to reproduce any "good" (non-broken) behavior though...
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.
I've only seen the broken behavior.
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.
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.
Labels: -M-71 ReleaseBlock-Stable M-72
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
Project Member

Comment 22 by sheriffbot@chromium.org, Dec 6

Cc: bengr@chromium.org
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
Labels: OS-Android

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.
The fix is in-progress and should land soon.

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.
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.
Project Member

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

Labels: Merge-Request-72
Requesting merge into M72.
Project Member

Comment 30 by sheriffbot@chromium.org, Jan 4

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
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
How is the change looking in canary? Is it verified on canary and safe to merge?
This change is in Canary, I don't see any crashes that seem at all related to it. It should be safe to merge.
Labels: -Merge-Review-72 Merge-Approved-72
Approving merge to M72 branch 3626 based on comment #32. Please merge ASAP and mark bug as fixed. Thank you.
Labels: -Merge-Approved-72 Merge-Merged-72-3626
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}
Project Member

Comment 35 by bugdroid1@chromium.org, Jan 7

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

Status: Fixed (was: Started)

Sign in to add a comment