1192053 - Native theming: Support for Mac OS X source lists

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Stefan [:stefanh]]

I have a patch which is almost finished, I just need to test it a bit more. Will continue with this when I'm back from vacation.

Comment 1

[Stefan [:stefanh]]

Attachment: WIP

Comment 2

[Stefan [:stefanh]]

(not sure why the affected flag was set)

Comment 3

[Stefan [:stefanh]]

I pushed the WIP patch to try, builds are here: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-92f7f01e9409/. These builds should work fine but so far I've only tested on 10.9 and 10.10 (places window and history/bookmarks sidebar).

Comment 4

[Stefan [:stefanh]]

Attachment: New version

Marcus, can you please giving me some feedback so far... I think my nsTreeBodyFrame changes are a bit ugly, but I guess there's no other way(?).

Some comments:
--- a/browser/themes/osx/places/places.css
+++ b/browser/themes/osx/places/places.css
@@ -4,63 +4,67 @@
 
 %include ../shared.inc
 
 /* Sidebars */
 
 #bookmarksPanel, 
 #history-panel,
 #sidebar-search-container {
-  -moz-appearance: none !important;
-  background-color: transparent !important;
-  border-top: none !important;
+  background-color: transparent;
+  -moz-appearance: none;
 }
 }

Removing the background-color rule (or setting it to something else than transparent) on the #history-panel/#bookmarksPanel creates a very weird effect (not caused by this patch). We talked about this for some time ago. It's like the sidebar background falls apart and stuff from the main window gets painted in it as well. I could possibly avoid setting those -moz-appearance rules, but since I only support selection vibrancy if the parent is styled with -moz-mac-source-list, it’ll be bit more work to find the -moz-mac-source-list appearance rule from nsNativeThemeCocoa.

-.sidebar-placesTreechildren::-moz-tree-cell-text(selected) {  
-  font-weight: bold !important;
-  color: #ffffff !important;
+@media (-moz-mac-yosemite-theme) {
+  .sidebar-placesTreechildren::-moz-tree-cell-text(selected) {
+    font-weight: 500;
+    color: -moz-DialogText;
+  }
+
+  .sidebar-placesTreechildren::-moz-tree-cell-text(selected,focus) {
+    font-weight: normal;
+    color: HighLightText;
+  }

This isn’t a 100% match to what Apple does, but I think it’ll work.

diff --git a/gfx/src/nsThemeConstants.h b/gfx/src/nsThemeConstants.h
--- a/gfx/src/nsThemeConstants.h
+++ b/gfx/src/nsThemeConstants.h
@@ -283,8 +283,13 @@
 #define NS_THEME_WIN_EXCLUDE_GLASS                         242
 
 #define NS_THEME_MAC_VIBRANCY_LIGHT                        243
 #define NS_THEME_MAC_VIBRANCY_DARK                         244
 #define NS_THEME_MAC_DISCLOSURE_BUTTON_OPEN                245
 #define NS_THEME_MAC_DISCLOSURE_BUTTON_CLOSED              246
 
 #define NS_THEME_GTK_INFO_BAR                              247
+
+// Mac source lists
+#define NS_THEME_MAC_SOURCE_LIST                           248
+#define NS_THEME_MAC_SOURCE_LIST_SELECTION                 249
+#define NS_THEME_MAC_ACTIVE_SOURCE_LIST_SELECTION          250

I couldn’t think of anything shorter...I added the last one for some time ago since I think it’s simpler than using the nsITreeBoxObject (afaik I'd need that in order to check if tree is focused).

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8676408 [details] [diff] [review]
New version

Review of attachment 8676408 [details] [diff] [review]:
-----------------------------------------------------------------

Looks mostly good. There are some coding style violations (lines too long, there should be a space in "if(", variables should be lowercase).

How does the new code handle selected tree cells with -moz-appearance where the -moz-appearance is something other than a selection background?

::: widget/cocoa/VibrancyManager.mm
@@ +272,5 @@
>    [effectView performSelector:@selector(setAppearance:)
>                     withObject:AppearanceForVibrancyType(aType)];
>    [effectView setState:VisualEffectStateForVibrancyType(aType)];
>  
> +  BOOL ElCapitanMaterial = nsCocoaFeatures::OnElCapitanOrLater();

"canUseElCapitanMaterials" maybe? Needs to be lower case, and elCapitan looks funny.

@@ +277,5 @@
>    if (aType == VibrancyType::MENU) {
> +    // Before 10.11 there is no material that perfectly matches the menu
> +    // look. Of all available material types, NSVisualEffectMaterialTitlebar
> +    // is the one that comes closest.
> +    [effectView setMaterial:(ElCapitanMaterial) ? NSVisualEffectMaterialMenu :

I'd remove the brackets or put them around the whole ternary expression.

::: widget/cocoa/nsNativeThemeCocoa.mm
@@ +3035,5 @@
> +
> +    case NS_THEME_MAC_SOURCE_LIST_SELECTION:
> +    case NS_THEME_MAC_ACTIVE_SOURCE_LIST_SELECTION: {
> +      // Note that we don't draw any vibrant background unless we're in
> +      // a source list.

Can you add a comment that says why?

@@ +3040,5 @@
> +      if (VibrancyManager::SystemSupportsVibrancy() && IsInSourceList(aFrame)) {
> +        ThemeGeometryType type = ThemeGeometryTypeForWidget(aFrame, aWidgetType);
> +        DrawVibrancyBackground(cgContext, macRect, aFrame, type);
> +      } else {
> +        BOOL IsActiveSelection = (aWidgetType == NS_THEME_MAC_ACTIVE_SOURCE_LIST_SELECTION);

Variable names should be lowercase.

@@ +3045,5 @@
> +        RenderWithCoreUI(macRect, cgContext,
> +          [NSDictionary dictionaryWithObjectsAndKeys:
> +            [NSNumber numberWithBool:IsActiveSelection], @"focus",
> +            [NSNumber numberWithBool:YES], @"is.flipped",
> +            @"kCUIVariantGradientSideBarSelection", @"kCUIVariantKey",

Apple won't be happy about us adding another CoreUI call here... Have you checked whether there's a way to draw this using HITheme? (I know I'm asking for a lot here.)
We also need to make sure that this works all the way down to 10.6.

@@ +4038,5 @@
> +      return eThemeGeometryTypeSourceList;
> +    case NS_THEME_MAC_SOURCE_LIST_SELECTION:
> +      return !IsInSourceList(aFrame) ? eThemeGeometryTypeUnknown : eThemeGeometryTypeSourceListSelection;
> +    case NS_THEME_MAC_ACTIVE_SOURCE_LIST_SELECTION:
> +        return !IsInSourceList(aFrame) ? eThemeGeometryTypeUnknown : eThemeGeometryTypeActiveSourceListSelection;

Better reverse these conditions.

Comment 6

[Stefan [:stefanh]]

(In reply to Markus Stange [:mstange] from comment #5)
> Comment on attachment 8676408 [details] [diff] [review]
> New version
> 
> Review of attachment 8676408 [details] [diff] [review]:
.
.
.

> There are some coding style violations (lines too long, there should be a space in "if(", variables should be lowercase).

Ah, yeah - sorry about that. I'll fix all these and your other comments in an upcoming version.

> 
> How does the new code handle selected tree cells with -moz-appearance where
> the -moz-appearance is something other than a selection background?

It will be painted as a background. I haven't really thought about this - do you think it's an issue? If you, for example, for some reason want to draw a menuicon in the row, it will look funny because the tree-cell text will be drawn on top of it.

.
.
.

> 
> @@ +3045,5 @@
> > +        RenderWithCoreUI(macRect, cgContext,
> > +          [NSDictionary dictionaryWithObjectsAndKeys:
> > +            [NSNumber numberWithBool:IsActiveSelection], @"focus",
> > +            [NSNumber numberWithBool:YES], @"is.flipped",
> > +            @"kCUIVariantGradientSideBarSelection", @"kCUIVariantKey",
> 
> Apple won't be happy about us adding another CoreUI call here... Have you
> checked whether there's a way to draw this using HITheme? (I know I'm asking
> for a lot here.)
> We also need to make sure that this works all the way down to 10.6.

I tested this for a long time ago on 10.6.8, 10.7.5 and 10.9.x (I don't remember the exact version) and it looked fine. Interestingly, there are 3 visual states on 10.6.8 and 2 on the other OS versions.

The alternative to CoreUI is to draw the gradients manually (afaik there is no way we can draw this using HITheme). I could then use only 2 visual states (2 gradients). Do you think that's a better way? It shouldn't be that hard to draw 2 gradients. I could probably use NSGradient (or is Quartz better?).

Comment 7

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #6)
> It shouldn't be that
> hard to draw 2 gradients.
Oh, well - make that 4 (was only thinking about Aqua - forgot Graphite).

Comment 8

[Stefan [:stefanh]]

> Interestingly, there are 3 visual states on 10.6.8 and 2 on the other OS versions.
Now when I started to sample the colors, it turns out I was wrong about this, there are 3 different visual states on 10.9.5.

Comment 9

[Stefan [:stefanh]]

Attachment: Source list background, widget part

I decided to do the sidebar background first. shorlander updated the background-colors recently and I've just ripped the 10.9-gradients from him.

I don't know if the selections of the individual list items are worth the code, but I'll post a patch for it later on.

Comment 10

[Stefan [:stefanh]]

Attachment: Source list background, css part

Gijs, mind looking at this? Basically, it should look the same as before.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8752513 [details] [diff] [review]
Source list background, css part

Review of attachment 8752513 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/osx/browser.css
@@ -2433,5 @@
>  }
>  
>  @media (-moz-mac-yosemite-theme) {
>    #sidebar-box {
> -    -moz-appearance: -moz-mac-vibrancy-light;

This loses the vibrancy / translucency on 10.11, at least (and presumably the same on 10.10). That seems like a regression. Am I missing something?

Comment 12

[Stefan [:stefanh]]

Comment on attachment 8752513 [details] [diff] [review]
Source list background, css part

(In reply to :Gijs Kruitbosch from comment #11)
> Comment on attachment 8752513 [details] [diff] [review]
> Source list background, css part
> 
> Review of attachment 8752513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/osx/browser.css
> @@ -2433,5 @@
> >  }
> >  
> >  @media (-moz-mac-yosemite-theme) {
> >    #sidebar-box {
> > -    -moz-appearance: -moz-mac-vibrancy-light;
> 
> This loses the vibrancy / translucency on 10.11, at least (and presumably
> the same on 10.10). That seems like a regression. Am I missing something?

Yes :-). We don't need to special-case this for 10.10+ since the widget code handles it. So it's enough to just set "-moz-appearance: -moz-mac-source-list;" on the #sidebar-box like this:

 #sidebar-box {
-  background-color: hsl(212,19%,85%);
-  background-image: linear-gradient(hsl(213,26%,93%), hsl(212,19%,85%));
+  -moz-appearance: -moz-mac-source-list;
   box-shadow: inset -2px 0 0 hsla(0,0%,100%,.2);
 }

We still need to reset the box-shadow for 10.10+, though.

Comment 13

[Stefan [:stefanh]]

BTW, sorry for not providing any try build here, It just slipped my mind and now I'm at work. I can push to try later on this evening (GMT+2) if you want.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8752513 [details] [diff] [review]
Source list background, css part

No, I mean, I tested with both patches applied, and the translucency is gone. I have a build environment, and I used it - if the mac-source-list stuff is supposed to use the translucency on 10.10 or 10.11 then I'm afraid it just doesn't work.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8752513 [details] [diff] [review]
Source list background, css part

(In reply to :Gijs Kruitbosch from comment #14)
> Comment on attachment 8752513 [details] [diff] [review]
> Source list background, css part
> 
> No, I mean, I tested with both patches applied, and the translucency is
> gone. I have a build environment, and I used it - if the mac-source-list
> stuff is supposed to use the translucency on 10.10 or 10.11 then I'm afraid
> it just doesn't work.

Or, of course, (q)importbz was just lying. Sigh. :-\
I'll file a bug on that later. STR:
1. get tree
2. hg import bz://1192053
3. hit enter, or even type '1,2' and hit enter
Actual results: only patch #2 gets imported.

I'll have another look.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8752513 [details] [diff] [review]
Source list background, css part

Review of attachment 8752513 [details] [diff] [review]:
-----------------------------------------------------------------

WFM.

Comment 17

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #15)
> Comment on attachment 8752513 [details] [diff] [review]
> Source list background, css part
> 
> (In reply to :Gijs Kruitbosch from comment #14)
> > Comment on attachment 8752513 [details] [diff] [review]
> > Source list background, css part
> > 
> > No, I mean, I tested with both patches applied, and the translucency is
> > gone. I have a build environment, and I used it - if the mac-source-list
> > stuff is supposed to use the translucency on 10.10 or 10.11 then I'm afraid
> > it just doesn't work.
> 
> Or, of course, (q)importbz was just lying. Sigh. :-\
> I'll file a bug on that later. STR:
> 1. get tree
> 2. hg import bz://1192053
> 3. hit enter, or even type '1,2' and hit enter
> Actual results: only patch #2 gets imported.

Filed bug 1273107 for this, and filed bug 1273106 for an unrelated issue that this patch made me notice (doesn't seem to be related to the patch).

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8752512 [details] [diff] [review]
Source list background, widget part

Review of attachment 8752512 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the comment addressed

::: widget/cocoa/VibrancyManager.mm
@@ +272,5 @@
> +    // is the one that comes closest.
> +    [effectView setMaterial:canUseElCapitanMaterials ? NSVisualEffectMaterialMenu
> +                                                     : NSVisualEffectMaterialTitlebar];
> +  } else if (aType == VibrancyType::SOURCE_LIST && canUseElCapitanMaterials) {
> +      [effectView setMaterial:NSVisualEffectMaterialSidebar];

You broke the indent here and in the if case below.

::: widget/cocoa/nsNativeThemeCocoa.mm
@@ +3017,5 @@
> +        CGFloat activeGradientColors[8] = { 0.9137, 0.9294, 0.9490, 1.0,
> +                                            0.8196, 0.8471, 0.8784, 1.0 };
> +        CGFloat inactiveGradientColors[8] = { 0.9686, 0.9686, 0.9686, 1.0,
> +                                              0.9216, 0.9216, 0.9216, 1.0 };
> +        CGPoint start = CGPointMake(macRect.origin.x, macRect.origin.y);

This can just be = macRect.origin;

@@ +3019,5 @@
> +        CGFloat inactiveGradientColors[8] = { 0.9686, 0.9686, 0.9686, 1.0,
> +                                              0.9216, 0.9216, 0.9216, 1.0 };
> +        CGPoint start = CGPointMake(macRect.origin.x, macRect.origin.y);
> +        CGPoint end = CGPointMake(macRect.origin.x + macRect.size.width,
> +                                  macRect.origin.y + macRect.size.height);

So this computes the bottom right corner. Don't you want the bottom left corner? Otherwise I expect you'll get a diagonal gradient.

Comment 19

[Stefan [:stefanh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bc62de5f103

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0daf8488dbda

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0daf8488dbda

Comment 22

[Stefan [:stefanh]]

Attachment: Add support for vibrant tree rows in nsTreeBodyFrame.cpp

Mats, mind take a look at this? I want to apply vibrancy on selected tree rows. It’s basically 3 changes:
1) Make it possible to use theme drawing for selected rows. I don’t understand the reason for why it wasn’t possible before (this was added by pinkerton in 2002 - bug 115747).
2) Register theme geometrics (RegisterThemeGemetry) for the relevant row(s) in nsTreeBodyFrame::BuildDisplayList.
3) Set the right font smoothing background color for the row(s) in nsTreeBodyFrame::PaintRow.

Comment 23

[Stefan [:stefanh]]

Attachment: Implement NS_THEME_MAC_SOURCE_LIST_SELECTION, NS_THEME_MAC_ACTIVE_SOURCE_LIST_SELECTION

These are the rest of the core changes.

Comment 24

[Stefan [:stefanh]]

Attachment: Use ’-moz-mac-source-list-selection’ and ’-moz-mac-active-source-list-selection’ for selected sidebar rows

These are the css changes. syncedtabs/sidebar.css changes are untested. Gijs isn't accepting review requests until after the 29:th, so I'll ask him then (I'm unsure if Dao does these kind of reviews nowadays).

Comment 25

[Stefan [:stefanh]]

Try push (all patches folded together): https://treeherder.mozilla.org/#/jobs?repo=try&revision=32d4ef98b372

Try builds available here: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-32d4ef98b37207cd8681f30c192f7177ec263a89/

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 8785693 [details] [diff] [review]
Add support for vibrant tree rows in nsTreeBodyFrame.cpp

>layout/xul/tree/nsTreeBodyFrame.cpp
>+    PrefillPropertyArray(i, nullptr);
>+    nsAutoString properties;
>+    mView->GetRowProperties(i, properties);
>+    nsTreeUtils::TokenizeProperties(properties, mScratchArray);
>+    nsStyleContext* rowContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreerow);
>+    const nsStyleDisplay* displayData = rowContext->StyleDisplay();
>+
>+    if (displayData->mAppearance) {

Tokenizing properties and creating a new style context seems fairly
expensive to do for each visible row each time we build a display
list.  Maybe it's OK though if we assume XUL trees are rare and not
used in primary UI.

Perhaps we could make this whole loop #ifdef OSX though?
(see below)


>+      nsITheme *theme = nullptr;
>+      theme = PresContext()->GetTheme();

Nit: use one line, move the * next to the type name.

>-  bool isSelected = false;
>-  nsCOMPtr<nsITreeSelection> selection;
>-  mView->GetSelection(getter_AddRefs(selection));
>-  if (selection) 
>-    selection->IsSelected(aRowIndex, &isSelected);
>-  if (useTheme && !isSelected) {

Hmm, did you test this change on Linux and Windows?
Specifically, dropping the "!isSelected" check makes the assumption that
the theme will handle selected rows, right?  But, I only see widget/CSS
changes for OSX in this bug so how is this handled on other platforms?

Comment 27

[Stefan [:stefanh]]

(In reply to Mats Palmgren (:mats) from comment #26)
> Comment on attachment 8785693 [details] [diff] [review]
> Add support for vibrant tree rows in nsTreeBodyFrame.cpp
> 
> >layout/xul/tree/nsTreeBodyFrame.cpp
> >+    PrefillPropertyArray(i, nullptr);
> >+    nsAutoString properties;
> >+    mView->GetRowProperties(i, properties);
> >+    nsTreeUtils::TokenizeProperties(properties, mScratchArray);
> >+    nsStyleContext* rowContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreerow);
> >+    const nsStyleDisplay* displayData = rowContext->StyleDisplay();
> >+
> >+    if (displayData->mAppearance) {
> 
> Tokenizing properties and creating a new style context seems fairly
> expensive to do for each visible row each time we build a display
> list.  Maybe it's OK though if we assume XUL trees are rare and not
> used in primary UI.

I've been wondering about the performance impact... Hmm, I suppose there is no other way than the way I do it? That said, I wonder if I could limit this to trees which <tree> elements are styled as source lists... (I need the tree itself to be vibrant anyway, so in the widget patch I don't draw a vibrant background if the tree row isn't inside a vibrant source list).

> 
> Perhaps we could make this whole loop #ifdef OSX though?
> (see below)

We could absolutely do that, if you think it's better.

> 
> 
> >+      nsITheme *theme = nullptr;
> >+      theme = PresContext()->GetTheme();
> 
> Nit: use one line, move the * next to the type name.

Ah, right. Will do.
> 
> >-  bool isSelected = false;
> >-  nsCOMPtr<nsITreeSelection> selection;
> >-  mView->GetSelection(getter_AddRefs(selection));
> >-  if (selection) 
> >-    selection->IsSelected(aRowIndex, &isSelected);
> >-  if (useTheme && !isSelected) {
> 
> Hmm, did you test this change on Linux and Windows?
> Specifically, dropping the "!isSelected" check makes the assumption that
> the theme will handle selected rows, right?  But, I only see widget/CSS
> changes for OSX in this bug so how is this handled on other platforms?

Yes, the assumption is that the theme will handle rows, but only if they're themed and the theme supports it (see the added lines below). Since Linux/Windows doesn't theme their tree rows (with -moz-appearance) this shouldn't have any effect on Linux/Windows.

Comment 28

[Mats Palmgren (inactive)]

(In reply to Stefan [:stefanh] from comment #27)
> That said, I wonder if I could limit this
> to trees which <tree> elements are styled as source lists...

Yes, that would be good.

> > Perhaps we could make this whole loop #ifdef OSX though?
> > (see below)
> 
> We could absolutely do that, if you think it's better.

Yes, please do that as well, with a note that only OSX themes
use '-moz-appearence' on the rows currently.

> > >-  bool isSelected = false;
> > >-  nsCOMPtr<nsITreeSelection> selection;
> > >-  mView->GetSelection(getter_AddRefs(selection));
> > >-  if (selection) 
> > >-    selection->IsSelected(aRowIndex, &isSelected);
> > >-  if (useTheme && !isSelected) {
> > 
> Yes, the assumption is that the theme will handle rows, but only if they're
> themed and the theme supports it

But I assume that the "!isSelected" check is there precisely because
the "theme->DrawWidgetBackground" branch currently *can't* handle
selected rows.  And now you're adding that support to OSX, but don't
we still need to take the PaintBackgroundLayer branch on other platforms?

Or are you saying that all XUL trees are currently non-themed on all
platforms except on OSX? (IOW, "useTheme" is always false if non-OSX?)

> (see the added lines below).

I don't follow which lines you're referring to here.

> Since Linux/Windows doesn't theme their tree rows (with -moz-appearance)
> this shouldn't have any effect on Linux/Windows.

OK, but the change from "PaintBackgroundLayer" to
"theme->DrawWidgetBackground" for selected rows seems like a functional
change on Linux/Windows then.  I don't understand why that change works.

Comment 29

[Stefan [:stefanh]]

Linux/Windows doesn't hit "theme->DrawWidgetBackground", since the first if (..) below is always false (since they don't theme tree rows). They will instead use "PaintBackgroundLayer". But I agree that it isn't a good solution since if someone wanted to theme tree rows on Linux/Windows they might run into problems - then they would also theme selected rows. 

+  if (theme && theme->ThemeSupportsWidget(aPresContext, nullptr,
+                                          displayData->mAppearance)) {
+    nscolor color;
+    if (theme->WidgetProvidesFontSmoothingBackgroundColor(this, displayData->mAppearance, &color)) {
+      // Set the font smoothing background color provided by the widget.
+      ctx->SetFontSmoothingBackgroundColor(ToDeviceColor(color));
+    }
     nsRect dirty;
     dirty.IntersectRect(rowRect, aDirtyRect);
     theme->DrawWidgetBackground(&aRenderingContext, this, 
                                 displayData->mAppearance, rowRect, dirty);
   } else {
     result &= PaintBackgroundLayer(rowContext, aPresContext, aRenderingContext,
                                    rowRect, aDirtyRect);

Anyway, it's late here - I'll try to come up with a new patch during the week.

Comment 30

[Mats Palmgren (inactive)]

(In reply to Stefan [:stefanh] from comment #29)
> Linux/Windows doesn't hit "theme->DrawWidgetBackground", since the first if
> (..) below is always false (since they don't theme tree rows).

Oh, OK, that should work then.  Still, it would be good to do a Try
run for all platforms, just in case.

> if someone wanted to theme tree rows on Linux/Windows they might run into problems

Maybe, but I guess we could defer that until such time...

Comment 31

[Markus Stange [:mstange]]

Comment on attachment 8785694 [details] [diff] [review]
Implement NS_THEME_MAC_SOURCE_LIST_SELECTION, NS_THEME_MAC_ACTIVE_SOURCE_LIST_SELECTION

Review of attachment 8785694 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsNativeThemeCocoa.mm
@@ +513,5 @@
>      return [NativeWindowForFrame(aFrame) isMainWindow];
>    return FrameIsInActiveWindow(aFrame);
>  }
>  
> +static bool IsInSourceList(nsIFrame* aFrame) {

Can you add a comment that describes that this only checks the direct parent, and doesn't walk up the frame tree, and that this is fine because the source lists we care about are XUL trees, so aFrame is the tree body frame.

Alternatively, you could add a loop that walks up the frame tree :-)

Comment 32

[Stefan [:stefanh]]

Comment on attachment 8785696 [details] [diff] [review]
Use ’-moz-mac-source-list-selection’ and ’-moz-mac-active-source-list-selection’ for selected sidebar rows

(In reply to Stefan [:stefanh] from comment #24)
> Created attachment 8785696 [details] [diff] [review]
> Use ’-moz-mac-source-list-selection’ and
> ’-moz-mac-active-source-list-selection’ for selected sidebar rows
> 
> These are the css changes. syncedtabs/sidebar.css changes are untested

And I just tested the sidebar.css changes and they don't work (sigh, this is not xul...). I probably need to change some stuff in the cocoa patch in order to make this work.

Comment 33

[Markus Stange [:mstange]]

(In reply to Stefan [:stefanh] from comment #32)
> (sigh, this is not xul...)

This should make you happy :-)
And it also forces your hand in how to address my review comment - you will need to add the loop.

Comment 34

[Stefan [:stefanh]]

(In reply to Markus Stange [:mstange] from comment #33)
> (In reply to Stefan [:stefanh] from comment #32)
> > (sigh, this is not xul...)
> 
> This should make you happy :-)

Ah, heh - but in this case I assumed it was a tree ;-)

> And it also forces your hand in how to address my review comment - you will
> need to add the loop.

I haven't had the time to dig into it, but I think it'll require more (it's a html document and I did a quick check yesterday night by just styling the parent to the selection as a source list and the selection then turned white).

Comment 35

[Stefan [:stefanh]]

Comment on attachment 8785693 [details] [diff] [review]
Add support for vibrant tree rows in nsTreeBodyFrame.cpp

Clearing this request since a new patch will come.

Comment 36

[Stefan [:stefanh]]

Regarding comment #34, it appears that the selection is in a sub-document and we currently don't support vibrancy there, but I think I can solve that. Just need some time ;-)

Comment 37

[Stefan [:stefanh]]

Markus, if I understood you right when we talked for a while ago, I can't clear the background behind a widget in a subdocument. Would you mind explaining the reason for that?

Comment 38

[Markus Stange [:mstange]]

I don't remember. If enabling vibrancy in (chrome) subdocuments just works in your tests, then go for it. If it doesn't work, I can look at it again.

I might have thinking of the conceptual intermediate surface that the subdocument introduces (because it's a stacking context). But we only have a problem if there is a *real* intermediate surface during compositing, and we only create those if there are other CSS effects like opacity / transform / mix-blend-mode, and in those cases vibrancy would break even if it weren't in a subdocument. So subdocuments don't really make that situation any worse. So just ignore what I said :-)

Comment 39

[Stefan [:stefanh]]

(In reply to Markus Stange [:mstange] from comment #38)
> So just ignore what I said :-)

OK! thanks - that makes things easier :-)

Comment 40

[Stefan [:stefanh]]

Attachment: Add support for vibrant tree rows in nsTreeBodyFrame.cpp

Mats and I have talked about this - testing the nsTreebodyFrame changes in Instruments, I’ve noticed that tokenizing properties for only selected rows does improve performance, so I’ve limited our support here to selected rows.

Comment 41

[Stefan [:stefanh]]

Attachment: NS_THEME_MAC_SOURCE_LIST_SELECTION, NS_THEME_MAC_ACTIVE_SOURCE_LIST_SELECTION

I guess you should check my changes in nsDisplayList.cpp. I mean, the seem to work fine, but I have to admit that I’m not 100% sure of the background here...

Also, when it comes to nsNativeThemeCocoa::NeedToClearBackgroundBehindWidget (as my comment says), we don’t need to return false if we’re in a tree, but I suppose I could add a check and return false if we’re in a XUL tree if you prefer that.

Comment 42

[Stefan [:stefanh]]

Attachment: CSS changes in order to support native styling of selected rows in source lists

These are the css changes. The reason for the .content-container addition is that the selections are in a sub-frame in syncedtabs/sidebar.css (IsInSourceList just walkes up one frame).

Comment 43

[Stefan [:stefanh]]

try push (all patches): https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d8aba12731d95f17f4bb05fdb107068d5126424

Comment 44

[Mats Palmgren (inactive)]

Comment on attachment 8799064 [details] [diff] [review]
Add support for vibrant tree rows in nsTreeBodyFrame.cpp

Looks good, r=mats with a few nits:

>+    for (int32_t i = mTopRowIndex; i < mRowCount && i <= mTopRowIndex +
>+         mPageLength; i++) {

It might be clearer to condense the end condition like so:

   const int32_t end = std::min(mRowCount, mTopRowIndex + mPageLength);
   for (int32_t i = mTopRowIndex; i < end; i++) {

>+        const nsStyleDisplay* displayData = rowContext->StyleDisplay();
>+
>+        if (displayData->mAppearance) {

We don't actually need displayData for anything other than mAppearance,
so I think it would be slightly more readable as:

          auto appearance = rowContext->StyleDisplay()->mAppearance;
          if (appearance) {

(displayData->mAppearance occurs three times)

>+          nsITheme* theme = PresContext()->GetTheme();
>+          if (theme && theme->ThemeSupportsWidget(PresContext(), this,
>+                                                  displayData->mAppearance)) {

'theme' is loop-invariant so I'd prefer if we move it (and the null-check)
outside the outermost 'if'.

>-  bool useTheme = false;
>   nsITheme *theme = nullptr;

While we're here, please move the '*' next to the type name.

Comment 45

[Mats Palmgren (inactive)]

(In reply to Mats Palmgren (:mats) from comment #44)
>    const int32_t end = std::min(mRowCount, mTopRowIndex + mPageLength);
>    for (int32_t i = mTopRowIndex; i < end; i++) {

Oops, I meant:

   const int32_t end = std::min(mRowCount, mTopRowIndex + mPageLength + 1);
   for (int32_t i = mTopRowIndex; i < end; i++) {

Comment 46

[Mats Palmgren (inactive)]

Hmm, is "i <= mTopRowIndex + mPageLength" actually the right test to begin with?
Let's say there is only 1 row visible, isn't mPageLength == 1 in that case?
Then we would iterate twice through this loop, which seems wrong.
Am I missing something?

Comment 47

[Mats Palmgren (inactive)]

I see that this class have these accessors:
  int32_t FirstVisibleRow() const { return mTopRowIndex; }
  int32_t LastVisibleRow() const { return mTopRowIndex + mPageLength; }

So I guess the "i <= mTopRowIndex + mPageLength" is correct after all.
Perhaps it would clearer to use those accessors then:

   const auto end = std::min(FirstVisibleRow(), LastVisibleRow() + 1);
   for (auto i = FirstVisibleRow(); i < end; i++) {

The relation between mPageLength and LastVisibleRow() is a bit odd IMO,
but I guess it could be for covering cases where the view fits half
a row at the end.  So if the view shows 5.5 rows, mPageLength would be 5
and LastVisibleRow() would also be 5 (assuming row 0 is the first row),
which means the last visible row is actually the 6th row.

Comment 48

[Stefan [:stefanh]]

(In reply to Mats Palmgren (:mats) from comment #47)
> Perhaps it would clearer to use those accessors then:
> 
>    const auto end = std::min(FirstVisibleRow(), LastVisibleRow() + 1);
>    for (auto i = FirstVisibleRow(); i < end; i++) {

Thanks, I'll fix this and the rest of your comments before landing.

Comment 49

[Mats Palmgren (inactive)]

Bah, I made a typo there again, it should be:
   const auto end = std::min(mRowCount, LastVisibleRow() + 1);
   for (auto i = FirstVisibleRow(); i < end; i++) {

Comment 50

[:Gijs (he/him)]

Comment on attachment 8799069 [details] [diff] [review]
CSS changes in order to support native styling of selected rows in source lists

Review of attachment 8799069 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Stefan [:stefanh] from comment #43)
> try push (all patches):
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3d8aba12731d95f17f4bb05fdb107068d5126424

When trying to download and run a build from this, my mac tells me the app is "damaged" and should be moved to the trash. :-\

Testing locally, feels like this patch is fine.

::: browser/themes/osx/places/organizer.css
@@ +65,2 @@
>    #placesList > treechildren::-moz-tree-cell-text(selected) {
>      color: -moz-dialogtext;

So is the background native and the foreground color CSS-native? Hrmpf. This pattern keeps biting us on Windows because of high contrast mode. Probably OK here because it's not web UI - the changes here are never visible on web pages, right?

::: browser/themes/osx/places/places.css
@@ +17,5 @@
>  
>  .sidebar-placesTree,
>  .sidebar-placesTreechildren::-moz-tree-row {
>    background-color: transparent;
>    border-color: transparent;

Are the background/border/sizing changes in this rule still necessary?

Comment 51

[Stefan [:stefanh]]

(In reply to :Gijs Kruitbosch from comment #50)
> ::: browser/themes/osx/places/organizer.css
> @@ +65,2 @@
> >    #placesList > treechildren::-moz-tree-cell-text(selected) {
> >      color: -moz-dialogtext;
> 
> So is the background native and the foreground color CSS-native? Hrmpf. This
> pattern keeps biting us on Windows because of high contrast mode. Probably
> OK here because it's not web UI - the changes here are never visible on web
> pages, right?

Hmm I don't understand the issue here :-) menu.css uses 'color: MenuText;' and '-moz-appearance: menuitem;' and tooltips have 'color: InfoText;' and '-moz-appearance: tooltip;'.

> ::: browser/themes/osx/places/places.css
> @@ +17,5 @@
> >  
> >  .sidebar-placesTree,
> >  .sidebar-placesTreechildren::-moz-tree-row {
> >    background-color: transparent;
> >    border-color: transparent;
> 
> Are the background/border/sizing changes in this rule still necessary?

We need the rows to have a transparent background-color (otherwise not selected rows will have a white background) and we also need the font-size. But, yes the border rules can go. I guess I could move the ’background-color: transparent;’ to a separate block since we don’t need it for the .sidebar-placesTree.

And I see now that I missed that the border-top rule below can go:
 .sidebar-placesTree {
+  -moz-appearance: -moz-mac-source-list;
   border-top: 1px solid #bebebe;

Comment 52

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #51)
> And I see now that I missed that the border-top rule below can go:
>  .sidebar-placesTree {
> +  -moz-appearance: -moz-mac-source-list;
>    border-top: 1px solid #bebebe;

And actually, since this isn't displayed because of the -moz-appearance rule I've regressed this and I need to address it...

Comment 53

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #52)
> >  .sidebar-placesTree {
> > +  -moz-appearance: -moz-mac-source-list;
> >    border-top: 1px solid #bebebe;
> 
> And actually, since this isn't displayed because of the -moz-appearance rule
> I've regressed this and I need to address it...

OK, so I'll move the border-rule to this:
+.sidebar-placesTreechildren {
+  border-top: 1px solid #bebebe;
+}

Comment 54

[Stefan [:stefanh]]

Gijs, see comment #51, #52 and #53. Re the border rule: this is the border seen at the top of the tree, under the search box in the history/bookmarks sidebar (I need the tree to have '-moz-appearance: -moz-mac-source-list;'. Relevant parts will look like this when diff'ed:
----------------------------------------------
 .sidebar-placesTree,
 .sidebar-placesTreechildren::-moz-tree-row {
-  background-color: transparent;
-  border-color: transparent;
   padding-bottom: 1px;
   margin: 0;
   height: 24px;
-  border: none;
   font-size: 12px;
 }
 
+.sidebar-placesTree {
+  -moz-appearance: -moz-mac-source-list;
+}
+
+.sidebar-placesTreechildren {
+  border-top: 1px solid #bebebe;
+}
+
 .sidebar-placesTreechildren::-moz-tree-separator {
   border-top: 1px solid #505d6d;
   margin: 0 10px;
 }
 
-.sidebar-placesTree {
-  -moz-appearance: -moz-mac-source-list;
-  border-top: 1px solid #bebebe;
+.sidebar-placesTreechildren::-moz-tree-row {
+  background-color: transparent;
 }
----------------------------------------------

Comment 55

[:Gijs (he/him)]

Sorry for the delay. Those changes look fine to me.

Comment 56

[Markus Stange [:mstange]]

Comment on attachment 8799066 [details] [diff] [review]
NS_THEME_MAC_SOURCE_LIST_SELECTION, NS_THEME_MAC_ACTIVE_SOURCE_LIST_SELECTION

Review of attachment 8799066 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.h
@@ +560,3 @@
>     */
> +  bool IsInChromeDocumentOrPopup() {
> +    return mIsInChromePresContext || (mIsBuildingForPopup && !IsInSubdocument());

Let's just remove the !IsInSubdocument() check completely. Popups in subdocuments, or subdocuments in popups, are not something we need to worry about.

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a30ac62cb4e
Add support for vibrant tree rows in nsTreeBodyFrame.cpp. r=mats.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ea9d56141a1
Implement NS_THEME_MAC_SOURCE_LIST_SELECTION and NS_THEME_MAC_ACTIVE_SOURCE_LIST_SELECTION. r=mstange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4df9e6ee8dc4
Native theming of Mac source lists, css changes. r=Gijs.

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a30ac62cb4e
https://hg.mozilla.org/mozilla-central/rev/9ea9d56141a1
https://hg.mozilla.org/mozilla-central/rev/4df9e6ee8dc4