862886 - The toolbarbutton that spawns the subview in the menu panel should have a blue background

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This is supposed to be like http://people.mozilla.com/~shorlander/panel-experiment-03/panel-experiment.html.

The buttons were off by 2 pixels. I was able to get that down to 1 pixel separation between the toolbarbutton and the subview by subtracting the toolbarbutton borders, but I can't figure out where the other two pixels are coming from.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 738561 [details] [diff] [review]
Patch

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

This looks OK to me, but I'd like to get information from shorlander about colours before giving an r+.

::: browser/base/content/panelUI.js
@@ +278,5 @@
> +      let anchorBorderLeft = anchorCS.borderLeftWidth;
> +      let anchorBorderRight = anchorCS.borderRightWidth;
> +      let width = anchorRect.width;
> +      if (anchorBorderLeft && anchorBorderRight) {
> +        width -= parseInt(anchorBorderLeft, 10) + parseInt(anchorBorderRight, 10);

Is it worth trying to coerce these into integers in the event of NaN?

::: browser/themes/shared/panelUIOverlay.inc.css
@@ +228,5 @@
>    list-style-image: url("chrome://mozapps/skin/places/defaultFavicon.png");
>  }
> +
> +#PanelUI-mainView toolbarbutton.anchor {
> +  background-image: linear-gradient(#6db1e2, #3e8ad2);

I'm not sure we want these colours hard-coded. I've had my knuckles wrapped doing stuff like that. We might want to use platform-specific colours, via -moz-MenuHover and -moz-MenuHoverText.

I'll ui-r? shorlander on this to be sure.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 738561 [details] [diff] [review]
Patch

Posting Windows 7 screenshot next...

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Screenshot of patch on Windows 7

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

It was my understanding that the customize colors would be unified across all platforms.

(In reply to Mike Conley (:mconley) from comment #2)
> > +      if (anchorBorderLeft && anchorBorderRight) {
> > +        width -= parseInt(anchorBorderLeft, 10) + parseInt(anchorBorderRight, 10);
> 
> Is it worth trying to coerce these into integers in the event of NaN?
> 

The only way that NaN should get returned from parseInt in this scenario would be if parseInt was passed an empty string, but the line above it checks for a truthy-value in both of the variables.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jared Wein [:jaws] from comment #5)
> It was my understanding that the customize colors would be unified across
> all platforms.
> 

Just spoke to Steven in #ux, and we're going to need platform-specific colours on these.

> (In reply to Mike Conley (:mconley) from comment #2)
> > > +      if (anchorBorderLeft && anchorBorderRight) {
> > > +        width -= parseInt(anchorBorderLeft, 10) + parseInt(anchorBorderRight, 10);
> > 
> > Is it worth trying to coerce these into integers in the event of NaN?
> > 
> 
> The only way that NaN should get returned from parseInt in this scenario
> would be if parseInt was passed an empty string, but the line above it
> checks for a truthy-value in both of the variables.

Ok, sounds good.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

A note on that screenshot - I used -moz-menuhover for the background, but was forced to use #FFFFFF for the text colour. This is because -moz-menuhovertext was black...and hard to read.

I'm not sure where Aero overrides this stuff, because looking at -moz-menuhover...that's not what hovered menuitems looks like...

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Switched to using -moz-menuhover. I kept the #fff since the default value of black for -moz-menuhovertext on Windows doens't match the spec.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 739189 [details] [diff] [review]
Patch

Mixing -moz-MenuHover and #fff means that your text can become invisible depending on the OS theme.

You might want to use highlight/highlighttext.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v.2

Thanks, I feel better about not having to hardcode the #fff.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 739655 [details] [diff] [review]
Patch v.2

>       // We need to find the left edge of the anchor, relative to the main
>       // panel. Then we need to add half the width of the anchor. This is the
>       // target that we need to transition to.
>       let anchorRect = aAnchor.getBoundingClientRect();
>       let mainViewRect = this.mainView.getBoundingClientRect();
>       let leftEdge = anchorRect.left - mainViewRect.left;
>-      let center = (anchorRect.width / 2);
>+      let anchorCS = aAnchor.ownerDocument.defaultView.getComputedStyle(aAnchor);
>+      let anchorBorderLeft = anchorCS.borderLeftWidth;
>+      let anchorBorderRight = anchorCS.borderRightWidth;
>+      let width = anchorRect.width;
>+      if (anchorBorderLeft && anchorBorderRight) {
>+        width -= parseInt(anchorBorderLeft, 10) + parseInt(anchorBorderRight, 10);
>+      }

Seems like you should use clientWidth rather than messing with borders.

>+#PanelUI-mainView toolbarbutton.anchor {
>+  background-color: Highlight;
>+  background-image: linear-gradient(rgba(255,255,255,0.3), rgba(255,255,255,0));
>+  background-repeat: repeat-x;
>+  color: HighlightText;
>+}

Why is #PanelUI-mainView needed in this selector?

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v.3

(In reply to Dão Gottwald [:dao] from comment #11)
> Comment on attachment 739655 [details] [diff] [review]
> Patch v.2
> 
> Seems like you should use clientWidth rather than messing with borders.

Thanks, much better.

> >+#PanelUI-mainView toolbarbutton.anchor {
> >+  background-color: Highlight;
> >+  background-image: linear-gradient(rgba(255,255,255,0.3), rgba(255,255,255,0));
> >+  background-repeat: repeat-x;
> >+  color: HighlightText;
> >+}
> 
> Why is #PanelUI-mainView needed in this selector?

The panelUIOverlay.css file is included in browser.xul, and I didn't want the 'anchor' attribute, if present, to get applied to toolbarbuttons in the nav-bar or elsewhere.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 740336 [details] [diff] [review]
Patch v.3

(In reply to Jared Wein [:jaws] from comment #12)
> > >+#PanelUI-mainView toolbarbutton.anchor {
> > >+  background-color: Highlight;
> > >+  background-image: linear-gradient(rgba(255,255,255,0.3), rgba(255,255,255,0));
> > >+  background-repeat: repeat-x;
> > >+  color: HighlightText;
> > >+}
> > 
> > Why is #PanelUI-mainView needed in this selector?
> 
> The panelUIOverlay.css file is included in browser.xul, and I didn't want
> the 'anchor' attribute, if present, to get applied to toolbarbuttons in the
> nav-bar or elsewhere.

Do you actually set the class in cases where toolbar buttons are outside of the panel? Or are you concerned that unrelated code may set the class for different purposes? This should be addressed by choosing a more distinct name.

In any case, you should avoid the descendent selector where feasible: https://developer.mozilla.org/en-US/docs/CSS/Writing_Efficient_CSS?redirectlocale=en-US&redirectslug=Writing_Efficient_CSS#Avoid_the_descendant_selector.21

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v.4

(In reply to Dão Gottwald [:dao] from comment #13)
> Comment on attachment 740336 [details] [diff] [review]
> Patch v.3
> 
> (In reply to Jared Wein [:jaws] from comment #12)
> > > >+#PanelUI-mainView toolbarbutton.anchor {
> > > >+  background-color: Highlight;
> > > >+  background-image: linear-gradient(rgba(255,255,255,0.3), rgba(255,255,255,0));
> > > >+  background-repeat: repeat-x;
> > > >+  color: HighlightText;
> > > >+}
> > > 
> > > Why is #PanelUI-mainView needed in this selector?
> > 
> > The panelUIOverlay.css file is included in browser.xul, and I didn't want
> > the 'anchor' attribute, if present, to get applied to toolbarbuttons in the
> > nav-bar or elsewhere.
> 
> Do you actually set the class in cases where toolbar buttons are outside of
> the panel? Or are you concerned that unrelated code may set the class for
> different purposes? This should be addressed by choosing a more distinct
> name.

I'm more concerned about other people using the className of 'anchor'. I switched it to 'panelui-mainview-anchor', which Mike Conley was OK with on an interim basis (we both want to find a better name than panelui, but it's what is being used throughout our code currently).

Unfortunately, only using `toolbarbutton.panelui-mainview-anchor` isn't enough to get these styles to apply because a preceding rule with `#PanelUI-mainView toolbarbutton` has higher precedence. I prefixed my new rule with `#PanelUI-contents > ` so that it will match the specificity.

I'd like to file a follow-up bug to clean up the usage of the descendent selectors as well as lowering the specificity of most of these selectors so hacks like this won't need to continue.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/projects/jamun/rev/c20675d7fbac

Filed bug 864811 as a follow-up to clean up some of the CSS.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/projects/ux/rev/c20675d7fbac

Comment 17

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/c20675d7fbac