Profile picker blurry after MD redesign. |
|||||||||||||||||||||||
Issue description
,
Mar 10 2016
Bookmarks bar also looks blurry when there's a theme. This is a non-retina screen.
,
Mar 10 2016
The bookmarks bar case doesn't look so bad when zoomed up, I guess. It looks slightly blurrier on the theme. Tab strip attached. Maybe we should turn off subpixel-AA if there's as theme?
,
Mar 10 2016
For the image I attached, the second "a" of "Agenda" looks really bad. I guess this isn't surprising, since we're applying a fade-out, and subpixel AA definitely doesn't work well with transparency.
,
Mar 11 2016
There are cls forthcoming for the bookmarks bar and the account button. Thank you for flagging these issues.
,
Mar 22 2016
,
Mar 22 2016
,
Mar 29 2016
,
May 6 2016
,
Jun 1 2016
Moving this nonessential bug to the next milestone. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 15 2016
This issue has been moved once and is lower than Pri-1. Removing the milestone. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 29 2016
Ping?
,
Jul 29 2016
Yeah, hoping to get to this in M54.
,
Sep 13 2016
Is this with the reworked user account button?
,
Sep 13 2016
chrome version: 55.0.2859.0
,
Sep 15 2016
This is looking worse because I got rid of the background image for user account button. There haven't been any changes with the text. Anyway, can you elaborate on "subpixel-AA", Erik? I have no idea what that is
,
Sep 15 2016
Sub-pixel refers to the red, green, and blue components of a pixel on the screen. Sub-pixel anti-aliasing is an algorithm that makes text smoother by adjusting the sub-pixel values. This is in contrast to plain old anti-aliasing, where you adjust the gray value of a pixel. So, for example, the SPAA algorithm may determine that by making some edge pixel slightly red, it can create a smoother looking edge. The one catch with SPAA is you need to know the color of the background you're drawing the text on top of. This means text drawn into a transparent layer is generally not going to look good (because the algorithm can't know what that layer will be sitting over). I assume some root view is setWantsLayer:YES, which causes all descendants to have a layer. If you could setWantsLayer:NO on the button I think that would fix the problem, but I don't think you can have a non-layer backed view that's a subview of a layer-backed view. But you could possibly fake it by making the button opaque and drawing into it the composite of the superviews that sit behind it, then drawing the text. (Hope that makes sense.)
,
Sep 15 2016
Sub-pixel refers to the red, green, and blue components of a pixel on the screen. Sub-pixel anti-aliasing is an algorithm that makes text smoother by adjusting the sub-pixel values. This is in contrast to plain old anti-aliasing, where you adjust the gray value of a pixel. So, for example, the SPAA algorithm may determine that by making some edge pixel slightly red, it can create a smoother looking edge. The one catch with SPAA is you need to know the color of the background you're drawing the text on top of. This means text drawn into a transparent layer is generally not going to look good (because the algorithm can't know what that layer will be sitting over). I assume some root view is setWantsLayer:YES, which causes all descendants to have a layer. If you could setWantsLayer:NO on the button I think that would fix the problem, but I don't think you can have a non-layer backed view that's a subview of a layer-backed view. But you could possibly fake it by making the button opaque and drawing into it the composite of the superviews that sit behind it, then drawing the text. (Hope that makes sense.)
,
Sep 15 2016
Subpixel AA works best on a solid color background. It isn't going to work properly when there's a theme, since the background is probably not solid color. I think we should just turn it off when there's a theme.
,
Sep 15 2016
Yes, perhaps that's the best (and simplest) answer.
,
Sep 19 2016
Thanks for the explanation! I'm trying to understand how to turn it off. From what I understand, CALayers are using subpixels. As a result, we should turn off subpixel AA by removing the layers if there's a theme. Is that correct?
,
Sep 19 2016
CALayers don't use subpixels, at least not in the sense of SPAA. The sub-pixels, BTW, are just the individual R-G-B elements that make up each pixel. I'm not sure you can turn off layers for the button because once an NSView has been set to have a layer, all of its subviews will use layers. I don't think there's an explicit line that turns of layers for the account button so it seems like it must be getting set from a superview. It looks like you can disable SPAA with the following call before you draw the text: CGContextSetShouldSmoothFonts(ctx, false); However that may not work if the button is just using its cell to draw it (which it is?). I guess maybe give it a shot and if it doesn't work we can go from there.
,
Sep 19 2016
See https://developer.apple.com/reference/quartzcore/catextlayer and https://lists.apple.com/archives/cocoa-dev/2008/Mar/msg02296.html. If we're drawing text using CoreAnimation, shrike's suggestion should work. I suspect we're drawing text using AppKit's NSTextField, in which case marking the background as non-opaque should work. [btw, I haven't dug into this, but I suspect that the reason this problem exists is because we do something "wonky" to draw the theme, so that AppKit thinks we've got an opaque, solid color background (the normal top bar).]
,
Sep 19 2016
I see, thanks. I'll give that a shot and let you know how it goes. Yes, the button is using the cell to draw it. It's drawing the text by using NSAttributedString
,
Sep 19 2016
erikchen@ is right - the Appkit should disable SPAA when it can't determine the background color (e.g. the control return NO to isOpaque, background color is nil). It's likely we're doing something counter to that.
,
Sep 19 2016
Actually, the button has a transparent background, and the tabstrip background is itself translucent, so I believe SPAA has to be turned off completely. Here's what the text looks like when the button is in the hover state and there's no theme - I don't think all the light pixels around the glyphs are correct. This is after applying your user account button patch 2347283003.
,
Sep 20 2016
I haven't totally tracked down what's up, but this is triggered by NSVisualEffectView. If I remove it from the window, subpixel AA works fine. You can also make the issue go away by checking "Reduce Transparency" in Accessibility preferences > Display and relaunching Chrome. Screenshots attached.
,
Sep 20 2016
Actually, it looks like subpixel AA is off in those screenshots, but based on some other tests I think it'll come on in a non-buggy way with a couple of tweaks. I'll post a POC when I have it, probably not until tomorrow. (Also, the text doesn't exactly look good with this particular theme, but I think that's separate.)
,
Sep 20 2016
Last contribution for tonight. The issue presents when both of the following are true: - The button is a child of a view that draws a background. - That view or any of its ancestors have an NSVisualEffectView child with nonzero size. I attached a test Xcode project and screenshots (but it's tricky to see in the screenshots). It starts in the broken state, and there are spots to uncomment to fix it by moving the button outside of the background-drawing view, and to improve on that by getting the button to do non-broken subpixel AA. I was talking to shrike@ about changing BackgroundGradientView to be be used as a subview instead of a superclass. Maybe I could do that now? Or there might be a better/more immediate solution.
,
Sep 20 2016
If the problem is related to the NSVisualEffectView (which means it should also not occur if you set the visual effect state to NSVisualEffectStateInactive), that suggests the issue is vibrancy being applied to the text. Is the button's text color being set explicitly? If not, doing so might disable the vibrancy.
,
Sep 22 2016
(Slightly updated Xcode project and gif attached.) Unfortunately, setting the NSVisualEffectView's state to .inactive, explicitly setting the window's and/or the button's appearance to NSAppearance(named: NSAppearanceNameAqua), and/or explicitly setting a text color don't help. .inactive temporarily turns off the effect (e.g. to save resources) but keeps the same general appearance. I was a little bit off in my last comment: the issue has nothing to do with whether a superview of the button draws a background. The antialiasing changes whenever a label *or* any of its superviews (up to an opaque superview) overlaps an NSVisualEffectView! I'm going to investigate a little more deeply tonight with some help from otool and the slides from this presentation: https://developer.apple.com/videos/play/wwdc2014/220/
,
Sep 22 2016
Maybe as simple as turning off vibrancy? https://developer.apple.com/reference/appkit/nsvisualeffectview
,
Sep 22 2016
It's off by default — a view can only be vibrant if it has a vibrant .appearance or inherits one from a superview (and the default appearance is not vibrant). On top of that, like shrike@ hinted at, a text field only allows vibrancy when its .textColor is either NSColor.labelColor or NSColor.secondaryLabelColor (the default is NSColor.controlColor, but the current demo just sets it to black).
,
Sep 28 2016
,
Dec 9 2016
,
Dec 9 2016
Sorry for the lag; I have a plan to fix this.
,
Jan 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a5fd3c415f3cc4f8813ba9772d838bee41040ae8 commit a5fd3c415f3cc4f8813ba9772d838bee41040ae8 Author: sdy <sdy@chromium.org> Date: Fri Jan 13 22:51:21 2017 [Mac] Fix rough-looking profile picker text, especially with a dark theme. Issue 593835 shows the AvatarButton's text having jagged, rainbow tinted edges. It turns out that the text cell gets a hint about anti-aliasing from the NSVisualEffect view below it, but the hint is wrong because the NSVisualEffectView is itself below the theme background. This only happens when the NSVisualEffectView's blendingMode is set to NSVisualEffectBlendingModeBehindWindow (which makes the content behind the window show through with a blur effect). To create the effect, the view cuts a hole in the window and generates a mask which the window server uses to render a translucent material behind the window. Because the translucent parts of the window are invisible to the app, the text can't render itself with correct subpixel anti-aliasing, which needs information about the background. To help, NSVisualEffectView implements two undocumented methods: `-shouldSetFontSmoothingBackgroundColor` and `-_backgroundColorForFontSmoothing`. It provides the base color of the translucent material, and for the purpose of antialiasing the text cell pretends that it's over an opaque background of that color. Because the translucency effect is subtle, it's not obvious that the antialiasing is done with a fixed color. NSView implements these methods to first search its immediate subviews and return the first YES/non-nil answer, or return NO/nil if `self.opaque == YES`, then tail call the same method on its superview. A quirk of this strategy is that, to provide a background color to a text view, an NSVisualEffectView must be either a superview or an immediate subview of a superview. So, the AvatarButton's text does get a background color hint with the current hierarchy: - NSThemeFrame |- NSVisualEffectView | |- TabStripBackgroundView |- FullSizeContentView |- AvatarButton …but, if the NSVisualEffectView were wrapped in another view, it doesn't (because each step of the search just looks at immediate subviews): - NSThemeFrame |- NSView | |- NSVisualEffectView | | |- TabStripBackgroundView |- FullSizeContentView |- AvatarButton Anyway, back on topic, our issue is that the text cell accepts the hinted background color, which is never right since Chrome always draws a tint over the visual effect view, and in some cases the hint is drastically wrong. Ideally, we'd provide a fake background for the text to perform subpixel anti-aliasing against, but it's unclear how to do that. For now, the simplest solution is to wrap it in another view. This CL sucks all of the tab strip background drawing into TabStripBackgroundView, and makes it so that other views don't need to know about the NSVisualEffectView at all. It also only creates the NSVisualEffectView when it's used, and gets rid of it when it's not used (like in full screen, or when a theme is installed). BUG= 593835 Review-Url: https://codereview.chromium.org/2430493002 Cr-Commit-Position: refs/heads/master@{#443707} [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/base/mac/sdk_forward_declarations.h [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/browser/ui/cocoa/browser_window_controller_private.mm [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/browser/ui/cocoa/browser_window_controller_unittest.mm [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/browser/ui/cocoa/floating_bar_backing_view.h [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/browser/ui/cocoa/floating_bar_backing_view.mm [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/browser/ui/cocoa/tabs/tab_strip_background_view.h [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm [add] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/browser/ui/cocoa/tabs/tab_strip_controller.h [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/browser/ui/cocoa/tabs/tab_strip_view.h [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/browser/ui/cocoa/tabs/tab_strip_view.mm [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/browser/ui/cocoa/tabs/tab_window_controller.h [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/browser/ui/cocoa/tabs/tab_window_controller.mm [modify] https://crrev.com/a5fd3c415f3cc4f8813ba9772d838bee41040ae8/chrome/test/BUILD.gn
,
Jan 14 2017
,
Jan 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/23d8cf2f9da96b2e9e3496738688b6e9615ea094 commit 23d8cf2f9da96b2e9e3496738688b6e9615ea094 Author: pkalinnikov <pkalinnikov@chromium.org> Date: Tue Jan 17 11:38:38 2017 Revert of [Mac] Fix rough-looking profile picker text, especially with a dark theme. (patchset #9 id:180001 of https://codereview.chromium.org/2430493002/ ) Reason for revert: FullscreenControllerTest.FullscreenOnFileURL from browser_tests flakes chromium.mac/Mac10.10 Tests builder. Original issue's description: > [Mac] Fix rough-looking profile picker text, especially with a dark theme. > > Issue 593835 shows the AvatarButton's text having jagged, rainbow tinted > edges. It turns out that the text cell gets a hint about anti-aliasing > from the NSVisualEffect view below it, but the hint is wrong because the > NSVisualEffectView is itself below the theme background. > > This only happens when the NSVisualEffectView's blendingMode is set to > NSVisualEffectBlendingModeBehindWindow (which makes the content behind > the window show through with a blur effect). To create the effect, the > view cuts a hole in the window and generates a mask which the window > server uses to render a translucent material behind the window. > > Because the translucent parts of the window are invisible to the app, > the text can't render itself with correct subpixel anti-aliasing, which > needs information about the background. To help, NSVisualEffectView > implements two undocumented methods: > `-shouldSetFontSmoothingBackgroundColor` and > `-_backgroundColorForFontSmoothing`. It provides the base color of the > translucent material, and for the purpose of antialiasing the text cell > pretends that it's over an opaque background of that color. Because the > translucency effect is subtle, it's not obvious that the antialiasing is > done with a fixed color. > > NSView implements these methods to first search its immediate subviews > and return the first YES/non-nil answer, or return NO/nil if > `self.opaque == YES`, then tail call the same method on its superview. > > A quirk of this strategy is that, to provide a background color to a > text view, an NSVisualEffectView must be either a superview or an > immediate subview of a superview. So, the AvatarButton's text does get a > background color hint with the current hierarchy: > > - NSThemeFrame > |- NSVisualEffectView > | |- TabStripBackgroundView > |- FullSizeContentView > |- AvatarButton > > …but, if the NSVisualEffectView were wrapped in another view, it doesn't > (because each step of the search just looks at immediate subviews): > > - NSThemeFrame > |- NSView > | |- NSVisualEffectView > | | |- TabStripBackgroundView > |- FullSizeContentView > |- AvatarButton > > Anyway, back on topic, our issue is that the text cell accepts the > hinted background color, which is never right since Chrome always > draws a tint over the visual effect view, and in some cases the hint is > drastically wrong. > > Ideally, we'd provide a fake background for the text to perform subpixel > anti-aliasing against, but it's unclear how to do that. For now, the > simplest solution is to wrap it in another view. > > This CL sucks all of the tab strip background drawing into > TabStripBackgroundView, and makes it so that other views don't need to > know about the NSVisualEffectView at all. > > It also only creates the NSVisualEffectView when it's used, and gets rid > of it when it's not used (like in full screen, or when a theme is > installed). > > BUG= 593835 > > Review-Url: https://codereview.chromium.org/2430493002 > Cr-Commit-Position: refs/heads/master@{#443707} > Committed: https://chromium.googlesource.com/chromium/src/+/a5fd3c415f3cc4f8813ba9772d838bee41040ae8 TBR=spqchan@chromium.org,avi@chromium.org,rsesek@chromium.org,sdy@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 593835 , 681772 Review-Url: https://codereview.chromium.org/2638873002 Cr-Commit-Position: refs/heads/master@{#444028} [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/base/mac/sdk_forward_declarations.h [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/chrome/browser/ui/cocoa/browser_window_controller_private.mm [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/chrome/browser/ui/cocoa/browser_window_controller_unittest.mm [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/chrome/browser/ui/cocoa/floating_bar_backing_view.h [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/chrome/browser/ui/cocoa/floating_bar_backing_view.mm [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/chrome/browser/ui/cocoa/tabs/tab_strip_background_view.h [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/chrome/browser/ui/cocoa/tabs/tab_strip_background_view.mm [delete] https://crrev.com/f518baaa99a18fb01196946dafe59218ffbf4285/chrome/browser/ui/cocoa/tabs/tab_strip_background_view_unittest.mm [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/chrome/browser/ui/cocoa/tabs/tab_strip_controller.h [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/chrome/browser/ui/cocoa/tabs/tab_strip_view.h [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/chrome/browser/ui/cocoa/tabs/tab_strip_view.mm [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/chrome/browser/ui/cocoa/tabs/tab_window_controller.h [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/chrome/browser/ui/cocoa/tabs/tab_window_controller.mm [modify] https://crrev.com/23d8cf2f9da96b2e9e3496738688b6e9615ea094/chrome/test/BUILD.gn
,
Feb 23 2017
,
Feb 23 2017
,
Feb 23 2017
,
Feb 23 2017
,
Jun 27 2017
Tested the issue on Mac Os 10.12.5 using chrome dev M61 #61.0.3141.0 and followed below steps to verify : 1. Added morpheon dark theme to chrome. 2. Opened new tab and observed there is no blurry on bookmarks bar and profile picker icons and buttons. Attached screencast for reference. @Could someone confirm us if this is the expected result or if we have missed any steps in verifying the issue . Thanks!
,
Jul 5 2017
The issue only shows up when the profile has a name, so that the button shows text instead of an icon. But, yes, it should be fixed now. There's currently *no* subpixel AA on the button text — I created issue 739489 to track fixing that. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by erikc...@chromium.org
, Mar 10 2016Owner: shrike@chromium.org
Status: Assigned (was: Untriaged)