Closed Bug 862886 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(2 files, 4 obsolete files)

No description provided.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #738561 - Flags: review?(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.
Attachment #738561 - Flags: review?(mconley) → feedback+
Comment on attachment 738561 [details] [diff] [review]
Patch

Posting Windows 7 screenshot next...
Attachment #738561 - Flags: ui-review?(shorlander)
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.
(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.
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...
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #738561 - Attachment is obsolete: true
Attachment #738561 - Flags: ui-review?(shorlander)
Attachment #739189 - Flags: review?(mconley)
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.
Attachment #739189 - Flags: review-
Attached patch Patch v.2 (obsolete) — Splinter Review
Thanks, I feel better about not having to hardcode the #fff.
Attachment #739189 - Attachment is obsolete: true
Attachment #739189 - Flags: review?(mconley)
Attachment #739655 - Flags: review?(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?
Attached patch Patch v.3 (obsolete) — Splinter Review
(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.
Attachment #739655 - Attachment is obsolete: true
Attachment #739655 - Flags: review?(dao)
Attachment #740336 - Flags: review?(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
Attachment #740336 - Flags: review?(dao) → review-
Attached patch Patch v.4Splinter Review
(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.
Attachment #740336 - Attachment is obsolete: true
Attachment #740862 - Flags: review?(dao)
Attachment #740862 - Flags: review?(dao) → review+
https://hg.mozilla.org/projects/jamun/rev/c20675d7fbac

Filed bug 864811 as a follow-up to clean up some of the CSS.
Whiteboard: [fixed in jamun]
https://hg.mozilla.org/projects/ux/rev/c20675d7fbac
Whiteboard: [fixed in jamun] → [fixed in jamun][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/c20675d7fbac
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in jamun][fixed-in-ux]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.