Closed Bug 900162 Opened 11 years ago Closed 11 years ago

UX - New bookmark drop-down panel should use platform-consistent styling when in the toolbar

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jsbruner, Assigned: mikedeboer)

References

Details

(Whiteboard: [Australis:P3][Australis:M9])

Attachments

(2 files, 4 obsolete files)

Attached image Clash of panels
When the bookmark button is in the toolbar and one presses the drop-down indicator button to view bookmarks, the dropdown panel is now a customized grayish color. However when using that to enter child views, we switch to the native UI, which causes a non-appealing clash. (See screenshot)

While the bookmark button is in the toolbar, we should style its drop-down panel to match OS X as closely as possible.
The problem here is that both of those are native. but native arrow panels are grey, while native menupopups are white.
We can try to change one of the two colors, making it non-native. Though, iirc the arrow color is hardcoded.
In any case this can't stay the way it is. Stephen, do you have a plan for how this should look in the end on Mac?
Flags: needinfo?(shorlander)
(In reply to Marco Bonardo [:mak] from comment #1)
> The problem here is that both of those are native. but native arrow panels
> are grey, while native menupopups are white.
> We can try to change one of the two colors, making it non-native. Though,
> iirc the arrow color is hardcoded.

I know it's not easy to do this but... could we make the bookmarks widget a custom widget that has a menupopup in the toolbar, rather than a subview-like-thing, and shows a "regular" menu, but shows a/the subview in the menupanel? That might be the nicest way to fix this...
(In reply to :Gijs Kruitbosch from comment #3)
> I know it's not easy to do this but... could we make the bookmarks widget a
> custom widget that has a menupopup in the toolbar, rather than a
> subview-like-thing, and shows a "regular" menu, but shows a/the subview in
> the menupanel? That might be the nicest way to fix this...

I'm sorry but I don't understand, what is a "menupopup in the toolbar"?
I can't change the menu from being a menu, without dropping features like drag & drop.

(In reply to Markus Stange [:mstange] from comment #2)
> In any case this can't stay the way it is. Stephen, do you have a plan for
> how this should look in the end on Mac?

We could completely replace all of the arrow panel graphic on mac to make the background white. Then it would look uncoherent with other arrow panels in the product and in the system. Then people would complain the arrow panel is not native :)
It's not either/or. In the Windows mockups it looks like it's planned to have unscrollable things in the top and bottom areas of the panel, and only scroll the actual list of bookmarks in between. We could make the scrollable part lighter and properly align it flush with the left and right edges of the panel, for example. That would be a big step towards reducing the clash between the two styles.
Whiteboard: [Australis:P?][Australis:M?]
I actually don't think this is as bad as comment #0 or comment #2 make it out to be.
Whiteboard: [Australis:P?][Australis:M?] → [Australis:P4][Australis:M?]
(In reply to Marco Bonardo [:mak] from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > I know it's not easy to do this but... could we make the bookmarks widget a
> > custom widget that has a menupopup in the toolbar, rather than a
> > subview-like-thing, and shows a "regular" menu, but shows a/the subview in
> > the menupanel? That might be the nicest way to fix this...
> 
> I'm sorry but I don't understand, what is a "menupopup in the toolbar"?
> I can't change the menu from being a menu, without dropping features like
> drag & drop.

I mean that when you click the dropdown marker for the widget, you get a panel. We could make that be an actual menu? Then the consistency problem would be solved - although obviously all the other buttons would show a panel, and this button would show a menu, but perhaps that'd make more sense?
(In reply to :Gijs Kruitbosch from comment #7)
> I mean that when you click the dropdown marker for the widget, you get a
> panel. We could make that be an actual menu?

No, when you click the dropdown marker you get a menupopup that is styled as a panel, it's not a panel, for various technical reasons related to how the Places view works.  And it _was_ a normal menu before, UX requested it to be an arrow panel, and that's why it's styled as an arrow panel.

So I think it's not really my decision here, but an UX call. Fwiw during the Summit I took a look at it with Shorlander and it didn't look that bad. Though if we keep the 2 colors we should fix colors of the scrolling arrows areas.
(In reply to Marco Bonardo [:mak] from comment #8)
> No, when you click the dropdown marker you get a menupopup that is styled as
> a panel, it's not a panel, for various technical reasons related to how the
> Places view works.  And it _was_ a normal menu before, UX requested it to be
> an arrow panel, and that's why it's styled as an arrow panel.

I think the point is that a not fully implemented 'styling as an arrow panel' is the source of the problem. The panel itself and its background are a-ok, but the panel items retain the styling of (context-)menu items. When they would be styled like panel-items, the confusion will be gone, I think.

For the best example you'd have to customize UX to have the History button in the navbar. Click it and you'll see a panel with items in its full glory.

But. This won't be trivial to implement. The two routes I see are 1) nasty CSS hackery to keep the XUL the same as now but change the appearance and 2) change the XUL based on the position of the bookmarks (toolbaritems for panels, IIRC). I shuddered while writing down the second option.

If you think this story of mine touches base, I'd be happy to volunteer for taking this bug and elevate the priority to P3/ P2.
(In reply to Mike de Boer [:mikedeboer] from comment #9)
> 2)
> change the XUL based on the position of the bookmarks (toolbaritems for
> panels, IIRC). I shuddered while writing down the second option.

Toolbarbuttons. But that'd make it hard to implement the submenus. We should just go for option 1 and correct the background, color and hover styling to be correct for the panel. This will also fix bug 901469.
(In reply to :Gijs Kruitbosch from comment #10)
> Toolbarbuttons. But that'd make it hard to implement the submenus. We should
> just go for option 1 and correct the background, color and hover styling to
> be correct for the panel. This will also fix bug 901469.

Hence the *shudder* while writing it down :P
Assignee: nobody → mdeboer
Flags: needinfo?(shorlander)
Whiteboard: [Australis:P4][Australis:M?] → [Australis:P3][Australis:M?]
Status: NEW → ASSIGNED
You can see a screencast of how things look with this patch applied on OSX here: https://vimeo.com/76711166 (current patch has improved styling)

Shorlanders reaction to watching the screencast:

[17:43:32] <mikedeboer> shorlander: https://vimeo.com/76711166
[17:48:08] <shorlander> mikedeboer: wooo! ^5
[17:48:24] <mikedeboer> shorlander: ah! you like? goooood :)
[17:48:30] <mikedeboer> shorlander: ^5
[17:48:39] <shorlander> I think we should look into anchoring the panel on the other side

... I think he likes it.
Attachment #816557 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 816557 [details] [diff] [review]
Patch v1: make bookmark panel items look like panel items, not contextmenu items

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

F+ because too many comments. Also, if you're here anyway... slight scope creep, but if you like, can you fix the scrollbox arrows as well? See bug 900164.

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +66,5 @@
>  }
> +
> +#BMB_bookmarksPopup > menuitem,
> +#BMB_bookmarksPopup > menu {
> +  -moz-padding-end: calc(1em + 6px);

Wat. What is this doing and where do the magic numbers come from? Do we really need calc() ? Why do this only on OS X (there doesn't seem to be an equivalent rule on Windows...). (Where) do we have an equivalent rule for toolbarbuttons in panels? Ideally they should be using the same rules as much as possible, and where that's not possible because of differing requirements in what gets overridden etc., they should at least be in similar places in the source code so that people trying to grok this aren't made into code spelunkers.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +4,5 @@
>  
>  %filter substitution
>  %define menuPanelWidth 21em
>  %define exitSubviewGutterWidth 38px
> +%define panelButtonStateHover :not([disabled]):not([checked]):not([open]):not(:active):hover

Please make this rule mirror the following one by using:

:not(:-moz-any([disabled],[checked],[open],:active)):hover

@@ +5,5 @@
>  %filter substitution
>  %define menuPanelWidth 21em
>  %define exitSubviewGutterWidth 38px
> +%define panelButtonStateHover :not([disabled]):not([checked]):not([open]):not(:active):hover
> +%define panelButtonStateActive :not([disabled]):-moz-any([open],[checked],:hover:active)

Why 'panelButton' ? This button can occur anywhere, not just in panels, and the rules aren't panel-specific. Please use a clearer name.

It may also make sense to add a @define@ for the #BMB_bookmarksPopup > .whatever that is somewhat explanatory. "menuItemInBookmarksPanel" or somesuch. Up to you.

@@ +292,5 @@
>  #customization-palette .toolbarbutton-text {
>    display: none;
>  }
>  
> +#BMB_bookmarksPopup > menuitem,

Please use something more specific on the right hand side. Don't all these menus and menuitems have a class in common?

@@ +298,4 @@
>  panelview toolbarbutton,
>  #widget-overflow-list > toolbarbutton,
>  .customizationmode-button {
> +  -moz-appearance: none !important; /* important, to override toolkit rules */

I don't understand. Originally, this rule didn't have !important, not even the one you removed in browser.css. Why does it need it now? Even if it really does, please only do that for the menuitems, and leave this one untouched.

@@ +317,4 @@
>  #edit-controls@inAnyPanel@ > toolbarbutton,
>  #zoom-controls@inAnyPanel@ > toolbarbutton,
> +.customizationmode-button,
> +#BMB_bookmarksPopup > menuitem@panelButtonStateHover@,

Again, please make this more specific.

@@ +330,3 @@
>  .customizationmode-button:not([disabled]):hover:active,
> +#widget-overflow-list > toolbarbutton@panelButtonStateActive@,
> +#BMB_bookmarksPopup > menuitem@panelButtonStateActive@,

Again, please make this more specific.

@@ +343,5 @@
>  }
>  
> +#BMB_bookmarksPopup > menuitem,
> +#BMB_bookmarksPopup > menu {
> +  font: inherit;

Is this actually necessary? Do we use different fonts for buttons and menus on all platforms? (and, once more, please use more specific selectors)

@@ +348,5 @@
> +}
> +
> +#BMB_bookmarksPopup > menuitem:not([disabled="true"]),
> +#BMB_bookmarksPopup > menu:not([disabled="true"]) {
> +  color: inherit !important;

What happens in the disabled case? Can you add a comment about why the !important is necessary and what the practical consequence of this change is?

@@ +352,5 @@
> +  color: inherit !important;
> +}
> +
> +panelview toolbarseparator,
> +#BMB_bookmarksPopup > menuseparator {

Ideally, you'd use a class here, too, but I'm not sure menuseparators will have one.

::: browser/themes/windows/customizableui/panelUIOverlay.css
@@ +20,5 @@
> +	padding-bottom: 1px;
> +}
> +
> +#BMB_bookmarksPopup > menuitem > .menu-text,
> +#BMB_bookmarksPopup > menuitem > .menu-iconic-text,

Oy. Why do we need these? Can we come up with better selectors if we really really really need to have 'em?

Nit: please use the file's indentation (two spaces)
Attachment #816557 - Flags: review?(gijskruitbosch+bugs) → feedback+
Thanks for the feeback!

Gijs, please bear in mind that this approach has to deal with platform-specific, toolkit theme code that is, in my experience, rather messy in some places.
I will make sure to put the hacky stuff as isolated from other rules as possible (the !important flags, for example).

Answering some of your questions:

> What is this doing and where do the magic numbers come from? Do we really need calc() ? Why do this only on OS X (there doesn't seem to be an equivalent rule on Windows...). (Where) do we have an equivalent rule for toolbarbuttons in panels?

Well, this is to make sure that end-aligned menu elements (arrows and hotkey labels) aren't stuck to the edge, but instead take a reasonable offset from the edge, including the default 6px padding. I used 1em to keep it consistent across font-sizes/ platforms.

> Don't all these menus and menuitems have a class in common?

No. *sigh*

> Originally, this rule didn't have !important, not even the one you removed in browser.css. Why does it need it now?

I need to make it more specific indeed. It'll move to be specific for menuitem elements on Linux. The toolkit theme has an important! rule for those with type="checkbox". (*sigh* no. 2)

> Is this actually necessary? Do we use different fonts for buttons and menus on all platforms?

Not on all platforms, you are right about that. Windows for sure and possibly Linux (have to double-check that). However, that are many other font-* properties set for this shorthand that can differ o so slightly. Therefore I sanitize it here for all platforms.

> What happens in the disabled case?

Well, because of `inherit`, the text color will stay the default instead of a greytone whether the menu(-item) is disabled or not.

> Oy. Why do we need these?

On Windows, the toolkit theme adds too much padding around the labels, hence I really, really, really needed to change it.
(In reply to Mike de Boer [:mikedeboer] from comment #14)
> Thanks for the feedback!
> 
> Gijs, please bear in mind that this approach has to deal with
> platform-specific, toolkit theme code that is, in my experience, rather
> messy in some places.
> I will make sure to put the hacky stuff as isolated from other rules as
> possible (the !important flags, for example).

Thanks! And thank you for the elaborate feedback to my questions. I heard you like responses... so I put some responses after your responses:

> > What is this doing and where do the magic numbers come from? Do we really need calc() ? Why do this only on OS X (there doesn't seem to be an equivalent rule on Windows...). (Where) do we have an equivalent rule for toolbarbuttons in panels?
> 
> Well, this is to make sure that end-aligned menu elements (arrows and hotkey
> labels) aren't stuck to the edge, but instead take a reasonable offset from
> the edge, including the default 6px padding. I used 1em to keep it
> consistent across font-sizes/ platforms.

Right, but the rule is in an OS X specific file, so the platform consistency isn't a good argument here. As for the hotkey labels, do we show those? That's not consistent with the other views, but I suppose showing them is probably better than not showing them (I'm lookin' at you, devtools panel). So if this needs adjusting on OS X, please have a rule for it that just includes the correct px value. If it needs adjusting elsewhere, well, then that was missing from your original patch (but you could probably add such a value elsewhere as well). :-)

> > Don't all these menus and menuitems have a class in common?
> 
> No. *sigh*

Let's fix that. You can change the bookmarks code that generates the actual elements to give them a class. That way, 3rd party themes can also use these and everyone is happier.

> > Originally, this rule didn't have !important, not even the one you removed in browser.css. Why does it need it now?
> 
> I need to make it more specific indeed. It'll move to be specific for
> menuitem elements on Linux. The toolkit theme has an important! rule for
> those with type="checkbox". (*sigh* no. 2)

OK. Yeah, making it specific for that is probably a saner option. For the checkbox case, erm, yeah, I guess we get to override that with more !important. It's like a virus. :-(

> > What happens in the disabled case?
> 
> Well, because of `inherit`, the text color will stay the default instead of
> a greytone whether the menu(-item) is disabled or not.

But we do want it to have greytones, right? That's what the toolbarbuttons in the panels do...
just a side note, I artificially reduced the padding in the panel on Mac due to the fact Mac users were finding it very odd (and it was indeed looking very odd)... So, could be your rule is fighting with mine, you can search for:
#BMB_bookmarksPopup > .panel-arrowcontainer > .panel-arrowcontent {
  padding: 6px
}
(I would link the code but projects mxr is barely usable)

Is any widget related code in browser supposed to end up in panelUIOverlay.inc.css? I thought this was just for the main menupanel stuff, but this styling is only used when the widget is in the toolbar, never in the menupanel.

Finally the last complaint, I really dislike the hover style in Australis panels, I honestly thought it was a placeholder for the real style, I think in this case (panel opening menupopups) it looks weird (also the padding across the panel and the menupopups, as seen in the video). Though this is UX stuff, so that's just my uninformed opinion. Imo here we should only fix the arrowbuttons background, I don't see anything else really bad in the screenshot in comment 0, while the hover and padding style in the video are totally non-native.
(In reply to Marco Bonardo [:mak] from comment #16)
> just a side note, I artificially reduced the padding in the panel on Mac due
> to the fact Mac users were finding it very odd (and it was indeed looking
> very odd)... So, could be your rule is fighting with mine, you can search
> for:
> #BMB_bookmarksPopup > .panel-arrowcontainer > .panel-arrowcontent {
>   padding: 6px
> }
> (I would link the code but projects mxr is barely usable)

Alright, thanks for the info! I'll check it out.

> Is any widget related code in browser supposed to end up in
> panelUIOverlay.inc.css? I thought this was just for the main menupanel
> stuff, but this styling is only used when the widget is in the toolbar,
> never in the menupanel.

As of recent, Australis related styling for any panel are put in panelUIOverlay.inc.css.

> Finally the last complaint, I really dislike the hover style in Australis
> panels, I honestly thought it was a placeholder for the real style, I think
> in this case (panel opening menupopups) it looks weird (also the padding
> across the panel and the menupopups, as seen in the video). Though this is
> UX stuff, so that's just my uninformed opinion. Imo here we should only fix
> the arrowbuttons background, I don't see anything else really bad in the
> screenshot in comment 0, while the hover and padding style in the video are
> totally non-native.

The styling is indeed no-native and intended to be uniform across platforms; strengthening the Fx identity. I guess there is always room for improvement re. the style that is currently implemented, but that's outside the scope of this bug.

Right now the bookmarks panel is the odd-one-out; the only panel that retained its native style and that needs to be Australified. Yes, please add 'to Australify' to your vocabulary! ;)
(In reply to :Gijs Kruitbosch from comment #15)
> But we do want it to have greytones, right? That's what the toolbarbuttons
> in the panels do...

(responses are totally my thing, you heard that right!)
This is what the patch currently does. It makes the text be grey when disabled.
Attachment #816557 - Attachment is obsolete: true
Comment on attachment 818754 [details] [diff] [review]
Patch v1.1: make bookmark panel items look like panel items, not contextmenu items

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

For as to why I'm asking you to split up the pretty :-moz-any() stuff:

https://developer.mozilla.org/en-US/docs/Web/CSS/:any#Issues_with_performance_and_specificity

This is nearly there - it would be an r+ if I understood the answers to the two questions I asked below.

For the next patch, can you (also) get an rs from Marco to land the .xul and .js bits on fx-team/m-c so we don't get into conflict hell? Assuming "places-item" doesn't conflict with anything else, that should essentially be a no-op in terms of current effects. :-)

::: browser/themes/linux/customizableui/panelUIOverlay.css
@@ +4,5 @@
>  
>  %include ../../shared/customizableui/panelUIOverlay.inc.css
> +
> +#BMB_bookmarksPopup > .places-item[type="checkbox"] {
> +  -moz-appearance: none !important; /* important, to override toolkit rule */

Why only on Linux? Windows seems to define the same -moz-appearance:

http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/menu.css#232

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +70,5 @@
> +  padding-bottom: 5px;
> +}
> +
> +/* Override OSX-specific toolkit styles for the bookmarks panel */
> +#BMB_bookmarksPopup > .places-item > :-moz-any(.menu-accel-container,.menu-right) {

Please just use two comma-separated selector expressions here.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +341,5 @@
> +  font: inherit;
> +}
> +
> +#BMB_bookmarksPopup > .places-item:not([disabled="true"]) {
> +  color: inherit;

Is the greytext styling for disabled menuitems here the same as that for disabled toolbarbuttons? If not, you should still be adding a rule for that.

@@ +344,5 @@
> +#BMB_bookmarksPopup > .places-item:not([disabled="true"]) {
> +  color: inherit;
> +}
> +
> +#BMB_bookmarksPopup > .panel-arrowcontainer > .panel-arrowcontent > .popup-internal-box > :-moz-any(.autorepeatbutton-up,.autorepeatbutton-down) {

Again, please just use two selectors.

::: browser/themes/windows/customizableui/panelUIOverlay.css
@@ +18,5 @@
> +  padding-top: 1px;
> +  padding-bottom: 1px;
> +}
> +
> +#BMB_bookmarksPopup > .places-item > :-moz-any(.menu-text,.menu-iconic-text),

And some more splitting out, please.
Attachment #818754 - Flags: review?(gijskruitbosch+bugs) → feedback+
(In reply to :Gijs Kruitbosch from comment #20)
> ::: browser/themes/linux/customizableui/panelUIOverlay.css
> @@ +4,5 @@
> >  
> >  %include ../../shared/customizableui/panelUIOverlay.inc.css
> > +
> > +#BMB_bookmarksPopup > .places-item[type="checkbox"] {
> > +  -moz-appearance: none !important; /* important, to override toolkit rule */
> 
> Why only on Linux? Windows seems to define the same -moz-appearance:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/
> menu.css#232

True, but in that case I needn't use `!important` to override it, just a selector with higher precedence. The Linux theme, however, already uses `!important` in the base theme :(

http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/menu.css#167
(In reply to :Gijs Kruitbosch from comment #20)
> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ +341,5 @@
> > +  font: inherit;
> > +}
> > +
> > +#BMB_bookmarksPopup > .places-item:not([disabled="true"]) {
> > +  color: inherit;
> 
> Is the greytext styling for disabled menuitems here the same as that for
> disabled toolbarbuttons? If not, you should still be adding a rule for that.

It is the same (sorry for the separate comment).
Marco, as per comment 20, are you ok with landing these changes on m-c?
Attachment #818754 - Attachment is obsolete: true
Attachment #820284 - Flags: review?(mak77)
Attachment #820284 - Flags: review?(gijskruitbosch+bugs)
Attachment #820284 - Attachment description: Patch v1.2: introduce a common className for bookmark items and separators → Patch 1.2: introduce a common className for bookmark items and separators
Comment on attachment 820284 [details] [diff] [review]
Patch 1.2: introduce a common className for bookmark items and separators

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

LGTM, but for m-c landing it's probably good if Marco has a look.
Attachment #820284 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 820286 [details] [diff] [review]
Patch 2: make bookmark panel items look like panel items, not contextmenu items

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

LGTM!
Attachment #820286 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 820284 [details] [diff] [review]
Patch 1.2: introduce a common className for bookmark items and separators

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

I honestly don't find this very useful, since you are always referring to it as:
#BMB_bookmarksPopup > .places-item
it's also quite error-prone, add-ons add stuff to the menus, we add stuff to the menus, soon or later things will lack this magic class name... I think we should just for for menuitem and menuseparator
Attachment #820284 - Flags: review?(mak77) → review-
(In reply to Marco Bonardo [:mak] from comment #27)
> I honestly don't find this very useful, since you are always referring to it
> as:
> #BMB_bookmarksPopup > .places-item
> it's also quite error-prone, add-ons add stuff to the menus, we add stuff to
> the menus, soon or later things will lack this magic class name... I think
> we should just for for menuitem and menuseparator

Alright, fine with me. Gijs, what do you think?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Marco Bonardo [:mak] from comment #27)
> Comment on attachment 820284 [details] [diff] [review]
> Patch 1.2: introduce a common className for bookmark items and separators
> 
> Review of attachment 820284 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I honestly don't find this very useful, since you are always referring to it
> as:
> #BMB_bookmarksPopup > .places-item
> it's also quite error-prone, add-ons add stuff to the menus, we add stuff to
> the menus, soon or later things will lack this magic class name... I think
> we should just for for menuitem and menuseparator

Rock, meet hard place. The alternative means much more verbosity in the CSS, and specificity issues. Anyway, sounds like that's going to be what we'll be doing then.
Flags: needinfo?(gijskruitbosch+bugs)
back to menu/menuitem
Attachment #820284 - Attachment is obsolete: true
Attachment #820286 - Attachment is obsolete: true
Attachment #820984 - Flags: review?(gijskruitbosch+bugs)
OS: Mac OS X → All
Comment on attachment 820984 [details] [diff] [review]
Patch 1.3: make bookmark panel items look like panel items, not contextmenu items

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

LGTM, but I think there are some places where you only need one of menu/menuitem, as seen below.

I also noticed that there's only one more !important in here. I'm hoping you checked whether, with a tag name as last selector, these rules still win the specificity fight on all the platforms you're affecting? :-)

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +71,5 @@
> +  padding-bottom: 5px;
> +}
> +
> +/* Override OSX-specific toolkit styles for the bookmarks panel */
> +#BMB_bookmarksPopup > menu > .menu-accel-container,

Do menus have shortcuts?

@@ +74,5 @@
> +/* Override OSX-specific toolkit styles for the bookmarks panel */
> +#BMB_bookmarksPopup > menu > .menu-accel-container,
> +#BMB_bookmarksPopup > menuitem > .menu-accel-container,
> +#BMB_bookmarksPopup > menu > .menu-right,
> +#BMB_bookmarksPopup > menuitem > .menu-right {

Or menuitems arrows (which I guess is what this is...)

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +491,5 @@
>  #customizationui-widget-panel .PanelUI-characterEncodingView-list > toolbarbutton[current] > .toolbarbutton-text {
>    -moz-padding-start: 0px;
>  }
>  
> +#BMB_bookmarksPopup > menu[checked]::before,

Can we have checked menus? I don't think so...

@@ +499,5 @@
>    display: -moz-box;
>    width: 12px;
>  }
> +
> +#BMB_bookmarksPopup > menu[checked] > .menu-iconic-left,

Ditto.
Attachment #820984 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed with comments addressed.

https://hg.mozilla.org/projects/ux/rev/8a02ead5815b
Whiteboard: [Australis:P3][Australis:M?] → [Australis:P3][Australis:M9][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/8a02ead5815b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][Australis:M9][fixed-in-ux] → [Australis:P3][Australis:M9]
Target Milestone: --- → Firefox 28
Depends on: 941576
You need to log in before you can comment on or make changes to this bug.