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

Issue 604092 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug



Sign in to add a comment

Fullscreen bubble on OSX has two background shades

Project Member Reported by lgar...@chromium.org, Apr 16 2016

Issue description

Chrome 51.0.2679.0 
OSX 10.11.4

What steps will reproduce the problem?
(1) Visit https://permission.site
(2) Click on "Fullscreen"

What is the expected output?
The fullscreen bubble has a single background shade.

What do you see instead?
The right of the bubble has a lighter shade.

mgiuca, based on  Issue 352425  it seems that you implemented this on Mac; could you look into it?
 
Screen Shot 2016-04-15 at 17.05.18.png
17.4 KB View Download
Status: WontFix (was: Assigned)
Seems to be fixed in Chrome 52.
Screen Shot 2016-04-29 at 11.26.52.png
15.3 KB View Download
Cc: mgiuca@chromium.org
Owner: tapted@chromium.org
Hm, not sure why that happened or how it got fixed. tapted@ implemented on Mac so assigning to him in case it resurfaces (but keeping as WontFix).
Ooh - thanks for filing this! Quite Bizarre.. I'll definitely keep an eye out. I can't think of anything specific that changed, but there are quite a few cleanups around the way composited layers are built up on Mac - stuff in the compositor parts shared by webcontent and this UI. Some of this was behind experiments of their own, so it may appear by chance in dev/beta.

I'll need to poke around some more in m51, since the current plan is to let that ride into stable from beta at 100%. If this is the norm in m51 stable, we might want to wait until m52 :/
I tried a bunch of things - building head, building 712dcefb076f7a297568fa96750bcf6118d2ff08 (m50), building Release/Debug, and various languages but I couldn't reproduce this. I still need to try a retina screen - I think that's the case based on your screenshots at least.

I did, however, discover  Issue 608575  ("gunk" around the text with subpixel AA). That could be related - I'll try to get the fix merged to m51.
Cc: nyerramilli@chromium.org tkonch...@chromium.org
 Issue 616724  has been merged into this issue.
We've got another report of this, this time on the "Press ⌘+← to go back" bubble. There is an attached video on  Issue 616724 .
Labels: -Pri-3 M-52 Pri-1
Status: Assigned (was: WontFix)
I fixed the gunk I mentioned in #c4 in r391452 ( Issue 608575 ). But yeah, this still crops up on Retina screens.

Bizarrely, as well as requiring hidpi, it seems to depend on a particular length of the UI string as well. And the string for backspace-goes-back (perhaps some translations also) is to tickle this weirdness.

I'll do some dev work on my retina macbook soon to try and chase this down.
Project Member

Comment 8 by sheriffbot@chromium.org, Jun 6 2016

Labels: -M-52 M-53 MovedFrom-52
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 9 by tapted@chromium.org, Jun 14 2016

Status: Started (was: Assigned)
It's still kinda bizarre, but I think I have the fix in https://codereview.chromium.org/2065003002/
Labels: -M-53 Merge-Request-52 M-52
Bugdroid is still asleep, but Verified fixed in 53.0.2773.0. Requesting merge of r400273 to M-52 which is where "Press ⌘+← to go back" is launching (see  Issue 610039 ).

Description (duplicated from https://codereview.chromium.org/2065003002/) is

Do not SchedulePaint() inside views::Label::OnPaint()

View::SchedulePaint() shouldn't be called inside an OnPaint() method. At
best it wastes computation doing a follow-up, redundant paint. On Mac
it's causing a weird interaction with transparent backgrounds for
strings of particular lengths on retina screens.

For performance, MaybeBuildRenderTextLines() is always deferred until a
Paint. It calls RecalculateColors() to apply colors to the lines it
newly creates, and that schedules a paint. But
MaybeBuildRenderTextLines() just needs to apply the colors that have
already been calculated.

So, to fix, split RecalculateColors() into the color calculation and
ApplyTextColors(), which can be called from OnPaint().

BUG= 604092 
TEST=On a retina-screen Mac with a fresh profile, navigate (e.g. to
chrome://version), then press Backspace, The "Press <key> to go back"
popup should appear and it should have a consistent, transparent
background.

Committed: https://crrev.com/2b40c5ca82b21f503cc7cc75ac101f1d2b710fc1
Cr-Commit-Position: refs/heads/master@{#400273}

Comment 11 by tin...@google.com, Jun 21 2016

Labels: -Merge-Request-52 Merge-Approved-52 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M52 (branch: 2743)
Project Member

Comment 12 by bugdroid1@chromium.org, Jun 21 2016

Labels: -merge-approved-52 merge-merged-2743
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1812ed000f950a01a19e12f0bc3dfdc5b1642724

commit 1812ed000f950a01a19e12f0bc3dfdc5b1642724
Author: Trent Apted <tapted@chromium.org>
Date: Tue Jun 21 11:55:17 2016

[merge-m52] Do not SchedulePaint() inside views::Label::OnPaint()

View::SchedulePaint() shouldn't be called inside an OnPaint() method. At
best it wastes computation doing a follow-up, redundant paint. On Mac
it's causing a weird interaction with transparent backgrounds for
strings of particular lengths on retina screens.

For performance, MaybeBuildRenderTextLines() is always deferred until a
Paint. It calls RecalculateColors() to apply colors to the lines it
newly creates, and that schedules a paint. But
MaybeBuildRenderTextLines() just needs to apply the colors that have
already been calculated.

So, to fix, split RecalculateColors() into the color calculation and
ApplyTextColors(), which can be called from OnPaint().

BUG= 604092 
TEST=On a retina-screen Mac with a fresh profile, navigate (e.g. to
chrome://version), then press Backspace, The "Press <key> to go back"
popup should appear and it should have a consistent, transparent
background.

Review-Url: https://codereview.chromium.org/2065003002
Cr-Commit-Position: refs/heads/master@{#400273}
(cherry picked from commit 2b40c5ca82b21f503cc7cc75ac101f1d2b710fc1)

Review URL: https://codereview.chromium.org/2085623006 .

Cr-Commit-Position: refs/branch-heads/2743@{#427}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/1812ed000f950a01a19e12f0bc3dfdc5b1642724/ui/views/controls/label.cc
[modify] https://crrev.com/1812ed000f950a01a19e12f0bc3dfdc5b1642724/ui/views/controls/label.h
[modify] https://crrev.com/1812ed000f950a01a19e12f0bc3dfdc5b1642724/ui/views/controls/label_unittest.cc

Status: Fixed (was: Started)
Labels: TE-Verified-M52 TE-Verified-52.0.2743.49
Tested the same on MacBook Pro Retina 10.11.5 chrome version 52.0.2743.49 as per the TEST in comment #12 - "Press to go back" has a consistent, transparent background.

Fix works as expected

Please find the screenshot
Screen Shot 2016-06-22 at 2.40.47 PM.png
736 KB View Download
Components: -Security>UX
Labels: Team-Security-UX
Security>UX component is deprecated in favor of the Team-Security-UX label

Sign in to add a comment