Native theming: Support for Mac OS X source lists

RESOLVED FIXED in Firefox 52

Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: stefanh, Assigned: stefanh)

Tracking

Trunk
mozilla52
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: tpi:+)

Attachments

(5 attachments, 5 obsolete attachments)

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.
Posted patch WIP (obsolete) — Splinter Review
(not sure why the affected flag was set)
I pushed the WIP patch to try, builds are here: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/stefanh@inbox.com-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).
Posted patch New version (obsolete) — Splinter Review
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).
Attachment #8644655 - Attachment is obsolete: true
Attachment #8676408 - Flags: feedback?(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.
Attachment #8676408 - Flags: feedback?(mstange) → feedback+
(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?).
(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).
> 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.
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.
Attachment #8676408 - Attachment is obsolete: true
Attachment #8752512 - Flags: review?(mstange)
Gijs, mind looking at this? Basically, it should look the same as before.
Attachment #8752513 - Flags: review?(gijskruitbosch+bugs)
Status: NEW → ASSIGNED
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?
Attachment #8752513 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8752513 - Flags: review?(gijskruitbosch+bugs)
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 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.
Attachment #8752513 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8752513 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8752513 [details] [diff] [review]
Source list background, css part

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

WFM.
Attachment #8752513 - Flags: review?(gijskruitbosch+bugs) → review+
(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 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.
Attachment #8752512 - Flags: review?(mstange) → review+
Keywords: leave-open
Priority: -- → P2
Whiteboard: tpi:+
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.
Attachment #8785693 - Flags: review?(mats)
These are the rest of the core changes.
Attachment #8785694 - Flags: review?(mstange)
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).
Attachment #8785696 - Attachment description: se ’-moz-mac-source-list-selection’ and ’-moz-mac-active-source-list-selection’ for selected sidebar rows → Use ’-moz-mac-source-list-selection’ and ’-moz-mac-active-source-list-selection’ for selected sidebar rows
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?
Flags: needinfo?(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.
Flags: needinfo?(stefanh)
(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.
Flags: needinfo?(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.
Flags: needinfo?(stefanh)
(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 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 :-)
Attachment #8785694 - Flags: review?(mstange) → review+
Attachment #8785696 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8785696 - Flags: review?(gijskruitbosch+bugs)
(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.
(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 on attachment 8785693 [details] [diff] [review]
Add support for vibrant tree rows in nsTreeBodyFrame.cpp

Clearing this request since a new patch will come.
Attachment #8785693 - Flags: review?(mats)
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 ;-)
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?
Flags: needinfo?(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 :-)
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #38)
> So just ignore what I said :-)

OK! thanks - that makes things easier :-)
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.
Attachment #8785693 - Attachment is obsolete: true
Attachment #8785694 - Attachment is obsolete: true
Attachment #8785696 - Attachment is obsolete: true
Attachment #8799064 - Flags: review?(mats)
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.
Attachment #8799066 - Flags: review?(mstange)
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).
Attachment #8799069 - Flags: review?(gijskruitbosch+bugs)
Attachment #8799069 - Attachment description: CSS changes in order to support native styling of selected rows in source lists. r?Gijs. → CSS changes in order to support native styling of selected rows in source lists
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.
Attachment #8799064 - Flags: review?(mats) → review+
(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++) {
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?
Flags: needinfo?(stefanh)
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.
(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.
Flags: needinfo?(stefanh)
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 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?
Attachment #8799069 - Flags: review?(gijskruitbosch+bugs) → review+
(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;
(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...
(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;
+}
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;
 }
----------------------------------------------
Flags: needinfo?(gijskruitbosch+bugs)
Sorry for the delay. Those changes look fine to me.
Flags: needinfo?(gijskruitbosch+bugs)
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.
Attachment #8799066 - Flags: review?(mstange) → review+
Pushed by stefanh@inbox.com:
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.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/1a30ac62cb4e
https://hg.mozilla.org/mozilla-central/rev/9ea9d56141a1
https://hg.mozilla.org/mozilla-central/rev/4df9e6ee8dc4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1310780
You need to log in before you can comment on or make changes to this bug.