[Mac][Material Design] Fix regression with initial bookmark bar text.

[Mac][Material Design] Fix regression with initial bookmark bar text.

[Mac][Material Design] Fix regression with initial bookmark bar text.

When you have an empty bookmarks bar, the bar displays a message about
adding bookmarks. With the switch to Material Design and the addition
of an NSVisualEffectView, the textfields' opaque ancestor was no
longer opaque, messing up subpixel anti-aliasing.

This cl changes the BookmarkBarToolbarView's isOpaque: method to
always return YES. The BookmarkBarToolbarView was in fact always
opaque, so returning YES is correct. Doing so causes the
BookmarkBarToolbarView's to be the textfields' opaque ancestor,
leading to correctly-drawn text.

R=tapted@chromium.org
BUG=617550, 617856
Committed: https://crrev.com/eadf1aa099669f7a232ee8deed94d5b9d2da14a8
Cr-Commit-Position: refs/heads/master@{#401391}

[shrike, 2016-06-14 22:48:11]

Description was changed from

==========
[Mac][Material Design] Fix regression with initial bookmark bar text.

When you have an empty bookmarks bar, the bar displays a message about
adding bookmarks. With the switch to Material Design (suspecting the
NSVisualEffectView), this text is drawn into a transparent layer, which
means subpixel anti-aliasing no longer works. To functino correctly,
SBAA needs to know the color of the pixels the text is being drawn on
top of, which it can't know because the layer has a clear background.
The result is strange-looking text when using a custom theme.

The bar uses a textfield and a button to draw the text. This change
causes the textfield to fill its background with the theme background
(making it opaque). For the button, this change uses private Appkit
API to provide a hint background color for text drawing the text
(there are no good hooks for pre-drawing the background color into
the button).

R=avi@chromium.org
BUG=617550,617856
==========

to

==========
[Mac][Material Design] Fix regression with initial bookmark bar text.

When you have an empty bookmarks bar, the bar displays a message about
adding bookmarks. With the switch to Material Design (suspecting the
NSVisualEffectView), this text is drawn into a transparent layer, which
means subpixel anti-aliasing no longer works. To functino correctly,
SBAA needs to know the color of the pixels the text is being drawn on
top of, which it can't know because the layer has a clear background.
The result is strange-looking text when using a custom theme.

The bar uses a textfield and a button to draw the text. This change
causes the textfield to fill its background with the theme background
(making it opaque). For the button, this change uses private Appkit
API to provide a hint background color for text drawing the text
(there are no good hooks for pre-drawing the background color into
the button).

R=tapted@chromium.org
BUG=617550,617856
==========

[shrike, 2016-06-14 22:48:11]

shrike@chromium.org changed reviewers:
+ tapted@chromium.org
- avi@chromium.org

[shrike, 2016-06-14 22:48:23]

PTAL

[tapted, 2016-06-15 02:09:04]

I played around a bit - I think I've nutted out the problem. First, I noticed
that the subpixel AA on the `Apps` button doesn't get screwy, but it should be
subject to the same rules.

The Apps button is drawn by BookmarkButtonCell it does.. stuff. But the
interesting bit is the -[NSView opaqueAncestor] property, which is always a
BookmarkButton.

But for the HyperlinkButtonCell that draws "Import bookmarks now...", it
switches between BookmarkBarViewCocoa (when detached), and NSThemeFrame (when
attached).

MD switches NSThemeFrame to a VisualEffectView, so now the opaque ancestor isn't
really opaque any more, and subpixel AA goes funny.

A ... possible fix seems to be simply to change the definition of
-[BookmarkBarToolbarView isOpaque] from:

- (BOOL)isOpaque {
  return [controller_ isInState:BookmarkBar::DETACHED];
}
to
- (BOOL)isOpaque {
  return YES;
}

it all looks fine both with/without a theme, and with/without MD. At least on
10.11. .. maybe that's all we need, but to properly claim the
BookmarkBarToolbarView is opaque, I think we need to give it a drawRect --
probably similar to the BookmarkBarTextField one you added.

I think this approach will be more robust than the fontSmoothingBackgroundColor
stuff.

https://codereview.chromium.org/2061353002/diff/1/chrome/browser/ui/cocoa/boo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.mm (right):

https://codereview.chromium.org/2061353002/diff/1/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.mm:30: @interface
NSView (Private)
nit: PrivateAPI? (Chrome has a few "Private" categories just for extending
interfaces of our own classes e.g. BrowserWindowController(Private))

https://codereview.chromium.org/2061353002/diff/1/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.mm:112:
@selector(setFontSmoothingBackgroundColor)]) {
@selector(setFontSmoothingBackgroundColor:)? (missing colon)

https://codereview.chromium.org/2061353002/diff/1/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.mm:131:
[importBookmarksButton_ setFontSmoothingBackgroundColor:
if (. respondsToSelector..)?

https://codereview.chromium.org/2061353002/diff/1/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.mm:132:
themeProvider->GetNSImageColorNamed(IDR_THEME_TOOLBAR)];
I have no idea whether fontSmoothingBackgroundColor supports "pattern"
NSColors.. but I suspect it will be out of phase anyway - I don't think we
should do this.

[tapted, 2016-06-15 02:58:18]

On 2016/06/15 02:09:04, tapted wrote:
> I played around a bit - I think I've nutted out the problem. First, I noticed
> that the subpixel AA on the `Apps` button doesn't get screwy, but it should be
> subject to the same rules.
> 
> The Apps button is drawn by BookmarkButtonCell it does.. stuff. But the
> interesting bit is the -[NSView opaqueAncestor] property, which is always a
> BookmarkButton.

huh, in fact bookmark_button.mm just does


- (BOOL)isOpaque {
  // Make this control opaque so that sub-pixel anti-aliasing works when
  // CoreAnimation is enabled.
  return YES;
}

- (void)drawRect:(NSRect)rect {
  NSView* bookmarkBarToolbarView = [[self superview] superview];
  [self cr_drawUsingAncestor:bookmarkBarToolbarView inRect:(NSRect)rect];
  [super drawRect:rect];
}


So BookmarkBarToolbarView really is opaque. And that's because of
-[BackgroundGradientView drawBackground] -- if there's a theme, it will first
fillRect with fully opaque, 20% black. So I think it's correct to have

- (BOOL)isOpaque {
  return YES;
}

in background_gradient_view.mm, which BookmarkBarToolbarView inherits from. But
BookmarkBarToolbarView doesn't invoke [super isOpaque]. But since that's always
YES, BookmarkBarToolbarView's isOpaque override can just be deleted.

[shrike, 2016-06-21 16:33:43]

On 2016/06/15 02:58:18, tapted wrote:
> On 2016/06/15 02:09:04, tapted wrote:
> > I played around a bit - I think I've nutted out the problem. First, I
noticed
> > that the subpixel AA on the `Apps` button doesn't get screwy, but it should
be
> > subject to the same rules.
> > 
> > The Apps button is drawn by BookmarkButtonCell it does.. stuff. But the
> > interesting bit is the -[NSView opaqueAncestor] property, which is always a
> > BookmarkButton.
> 
> huh, in fact bookmark_button.mm just does
> 
> 
> - (BOOL)isOpaque {
>   // Make this control opaque so that sub-pixel anti-aliasing works when
>   // CoreAnimation is enabled.
>   return YES;
> }
> 
> - (void)drawRect:(NSRect)rect {
>   NSView* bookmarkBarToolbarView = [[self superview] superview];
>   [self cr_drawUsingAncestor:bookmarkBarToolbarView inRect:(NSRect)rect];
>   [super drawRect:rect];
> }
> 
> 
> So BookmarkBarToolbarView really is opaque. And that's because of
> -[BackgroundGradientView drawBackground] -- if there's a theme, it will first
> fillRect with fully opaque, 20% black. So I think it's correct to have
> 
> - (BOOL)isOpaque {
>   return YES;
> }
> 
> in background_gradient_view.mm, which BookmarkBarToolbarView inherits from.
But
> BookmarkBarToolbarView doesn't invoke [super isOpaque]. But since that's
always
> YES, BookmarkBarToolbarView's isOpaque override can just be deleted.

The only thing is technically, BackgroundGradientView is not opaque because it
does not implement drawRect:. Certainly if you were to add a
BackgroundGradientView instance to a view hierarchy, that instance would not be
opaque. For this reason I am hesitant to implement - (BOOL)isOpaque { return
YES; } on that class. I have instead made the change in BookmarkBarToolbarView.
What are your thoughts on this?

PTAL

[tapted, 2016-06-22 00:50:53]

On 2016/06/21 16:33:43, shrike wrote:
> On 2016/06/15 02:58:18, tapted wrote:
> > On 2016/06/15 02:09:04, tapted wrote:
> > > I played around a bit - I think I've nutted out the problem. First, I
> noticed
> > > that the subpixel AA on the `Apps` button doesn't get screwy, but it
should
> be
> > > subject to the same rules.
> > > 
> > > The Apps button is drawn by BookmarkButtonCell it does.. stuff. But the
> > > interesting bit is the -[NSView opaqueAncestor] property, which is always
a
> > > BookmarkButton.
> > 
> > huh, in fact bookmark_button.mm just does
> > 
> > 
> > - (BOOL)isOpaque {
> >   // Make this control opaque so that sub-pixel anti-aliasing works when
> >   // CoreAnimation is enabled.
> >   return YES;
> > }
> > 
> > - (void)drawRect:(NSRect)rect {
> >   NSView* bookmarkBarToolbarView = [[self superview] superview];
> >   [self cr_drawUsingAncestor:bookmarkBarToolbarView inRect:(NSRect)rect];
> >   [super drawRect:rect];
> > }
> > 
> > 
> > So BookmarkBarToolbarView really is opaque. And that's because of
> > -[BackgroundGradientView drawBackground] -- if there's a theme, it will
first
> > fillRect with fully opaque, 20% black. So I think it's correct to have
> > 
> > - (BOOL)isOpaque {
> >   return YES;
> > }
> > 
> > in background_gradient_view.mm, which BookmarkBarToolbarView inherits from.
> But
> > BookmarkBarToolbarView doesn't invoke [super isOpaque]. But since that's
> always
> > YES, BookmarkBarToolbarView's isOpaque override can just be deleted.
> 
> The only thing is technically, BackgroundGradientView is not opaque because it
> does not implement drawRect:. Certainly if you were to add a
> BackgroundGradientView instance to a view hierarchy, that instance would not
be
> opaque. For this reason I am hesitant to implement - (BOOL)isOpaque { return
> YES; } on that class. I have instead made the change in
BookmarkBarToolbarView.
> What are your thoughts on this?
> 
> PTAL

sounds good!

lgtm

[tapted, 2016-06-22 00:51:20]

ooh except the CL description should be updated :)

[shrike, 2016-06-22 18:47:58]

Description was changed from

==========
[Mac][Material Design] Fix regression with initial bookmark bar text.

When you have an empty bookmarks bar, the bar displays a message about
adding bookmarks. With the switch to Material Design (suspecting the
NSVisualEffectView), this text is drawn into a transparent layer, which
means subpixel anti-aliasing no longer works. To functino correctly,
SBAA needs to know the color of the pixels the text is being drawn on
top of, which it can't know because the layer has a clear background.
The result is strange-looking text when using a custom theme.

The bar uses a textfield and a button to draw the text. This change
causes the textfield to fill its background with the theme background
(making it opaque). For the button, this change uses private Appkit
API to provide a hint background color for text drawing the text
(there are no good hooks for pre-drawing the background color into
the button).

R=tapted@chromium.org
BUG=617550,617856
==========

to

==========
[Mac][Material Design] Fix regression with initial bookmark bar text.

[Mac][Material Design] Fix regression with initial bookmark bar text.

When you have an empty bookmarks bar, the bar displays a message about
adding bookmarks. With the switch to Material Design and the addition
of an NSVisualEffectView, the textfields' opaque ancestor was no
longer opaque, messing up subpixel anti-aliasing.

This cl changes the BookmarkBarToolbarView's isOpaque: method to
always return YES. The BookmarkBarToolbarView was in fact always
opaque, so returning YES is correct. Doing so causes the
BookmarkBarToolbarView's to be the textfields' opaque ancestor,
leading to correctly-drawn text.

R=tapted@chromium.org
BUG=617550,617856
==========

[shrike, 2016-06-22 18:48:10]

The CQ bit was checked by shrike@chromium.org

[commit-bot: I haz the power, 2016-06-22 18:48:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2061353002/20001

[shrike, 2016-06-22 18:50:11]

On 2016/06/22 00:51:20, tapted wrote:
> ooh except the CL description should be updated :)

Updated.

[commit-bot: I haz the power, 2016-06-22 20:33:40]

Description was changed from

==========
[Mac][Material Design] Fix regression with initial bookmark bar text.

[Mac][Material Design] Fix regression with initial bookmark bar text.

When you have an empty bookmarks bar, the bar displays a message about
adding bookmarks. With the switch to Material Design and the addition
of an NSVisualEffectView, the textfields' opaque ancestor was no
longer opaque, messing up subpixel anti-aliasing.

This cl changes the BookmarkBarToolbarView's isOpaque: method to
always return YES. The BookmarkBarToolbarView was in fact always
opaque, so returning YES is correct. Doing so causes the
BookmarkBarToolbarView's to be the textfields' opaque ancestor,
leading to correctly-drawn text.

R=tapted@chromium.org
BUG=617550,617856
==========

to

==========
[Mac][Material Design] Fix regression with initial bookmark bar text.

[Mac][Material Design] Fix regression with initial bookmark bar text.

When you have an empty bookmarks bar, the bar displays a message about
adding bookmarks. With the switch to Material Design and the addition
of an NSVisualEffectView, the textfields' opaque ancestor was no
longer opaque, messing up subpixel anti-aliasing.

This cl changes the BookmarkBarToolbarView's isOpaque: method to
always return YES. The BookmarkBarToolbarView was in fact always
opaque, so returning YES is correct. Doing so causes the
BookmarkBarToolbarView's to be the textfields' opaque ancestor,
leading to correctly-drawn text.

R=tapted@chromium.org
BUG=617550,617856
==========

[commit-bot: I haz the power, 2016-06-22 20:33:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-22 20:36:49]

Description was changed from

==========
[Mac][Material Design] Fix regression with initial bookmark bar text.

[Mac][Material Design] Fix regression with initial bookmark bar text.

When you have an empty bookmarks bar, the bar displays a message about
adding bookmarks. With the switch to Material Design and the addition
of an NSVisualEffectView, the textfields' opaque ancestor was no
longer opaque, messing up subpixel anti-aliasing.

This cl changes the BookmarkBarToolbarView's isOpaque: method to
always return YES. The BookmarkBarToolbarView was in fact always
opaque, so returning YES is correct. Doing so causes the
BookmarkBarToolbarView's to be the textfields' opaque ancestor,
leading to correctly-drawn text.

R=tapted@chromium.org
BUG=617550,617856
==========

to

==========
[Mac][Material Design] Fix regression with initial bookmark bar text.

[Mac][Material Design] Fix regression with initial bookmark bar text.

When you have an empty bookmarks bar, the bar displays a message about
adding bookmarks. With the switch to Material Design and the addition
of an NSVisualEffectView, the textfields' opaque ancestor was no
longer opaque, messing up subpixel anti-aliasing.

This cl changes the BookmarkBarToolbarView's isOpaque: method to
always return YES. The BookmarkBarToolbarView was in fact always
opaque, so returning YES is correct. Doing so causes the
BookmarkBarToolbarView's to be the textfields' opaque ancestor,
leading to correctly-drawn text.

R=tapted@chromium.org
BUG=617550,617856

Committed: https://crrev.com/eadf1aa099669f7a232ee8deed94d5b9d2da14a8
Cr-Commit-Position: refs/heads/master@{#401391}
==========

[commit-bot: I haz the power, 2016-06-22 20:36:50]

Patchset 2 (id:??) landed as
https://crrev.com/eadf1aa099669f7a232ee8deed94d5b9d2da14a8
Cr-Commit-Position: refs/heads/master@{#401391}