Geoff Lankow (:darktrojan) Assignee
	Description • 14 years ago

http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/browser.css#603

Here we style .toolbarbutton-1 inside #TabsToolbar if it's a child of #TabsToolbar, or a child of a toolbarpaletteitem, but not if there's also a toolbaritem.

I'm happy to write the patch here, but I want to know whether it's better to use:

#TabsToolbar .toolbarbutton-1,
#TabsToolbar .toolbarbutton-1 > .toolbarbutton-menubutton-button,
#TabsToolbar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker,
#TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button {

which is tidier, but potentially a performance hit, or:

#TabsToolbar .toolbarbutton-1,
#TabsToolbar .toolbarbutton-1 > .toolbarbutton-menubutton-button,
#TabsToolbar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker,
#TabsToolbar > toolbarpaletteitem > .toolbarbutton-1,
#TabsToolbar > toolbarpaletteitem > .toolbarbutton-1 > .toolbarbutton-menubutton-button,
#TabsToolbar > toolbarpaletteitem > .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker,
#TabsToolbar > toolbaritem > .toolbarbutton-1,
#TabsToolbar > toolbaritem > .toolbarbutton-1 > .toolbarbutton-menubutton-button,
#TabsToolbar > toolbaritem > .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker,
#TabsToolbar > toolbarpaletteitem > toolbaritem > .toolbarbutton-1,
#TabsToolbar > toolbarpaletteitem > toolbaritem > .toolbarbutton-1 > .toolbarbutton-menubutton-button,
#TabsToolbar > toolbarpaletteitem > toolbaritem > .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker,
#TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button {

	Geoff Lankow (:darktrojan) Assignee
	Comment 1 • 14 years ago

Gah, first three lines of the second block should also have > selectors, I didn't undo far enough before I copied it. :(

	Dão Gottwald [:dao]
	Comment 2 • 14 years ago

I think we need to go with the first option. :(

	Geoff Lankow (:darktrojan) Assignee
	Comment 3 • 14 years ago

Seems to occur in more places than I first expected. I've only tested this out on Windows.

	Dão Gottwald [:dao]
	Comment 4 • 14 years ago

Comment on attachment 483667 [details] [diff] [review]
patch

>-#TabsToolbar > toolbarbutton > .toolbarbutton-icon,
>-#TabsToolbar > toolbarbutton > .toolbarbutton-menu-dropmarker,
>-#TabsToolbar > toolbarpaletteitem > toolbarbutton > .toolbarbutton-icon,
>-#TabsToolbar > toolbarpaletteitem > toolbarbutton > .toolbarbutton-menu-dropmarker,
>+#TabsToolbar toolbarbutton > .toolbarbutton-icon,
>+#TabsToolbar toolbarbutton > .toolbarbutton-menu-dropmarker,
> #TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button > .toolbarbutton-icon,
> #TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button > .toolbarbutton-menu-dropmarker,
> #TabsToolbar > toolbarpaletteitem > #bookmarks-menu-button-container > #bookmarks-menu-button > .toolbarbutton-icon,
> #TabsToolbar > toolbarpaletteitem > #bookmarks-menu-button-container > #bookmarks-menu-button > .toolbarbutton-menu-dropmarker {

Your changes seem to make the selectors involving #bookmarks-menu-button redundant.

	Geoff Lankow (:darktrojan) Assignee
	Comment 5 • 14 years ago

(In reply to comment #4)
> Your changes seem to make the selectors involving #bookmarks-menu-button
> redundant.

OK, got them, and a few others.

|toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button| and friends are also redundant, but I've left them alone, assuming they're written that way for a reason.

	Dão Gottwald [:dao]
	Comment 6 • 14 years ago

Comment on attachment 483915 [details] [diff] [review]
patch v2

>--- a/browser/themes/gnomestripe/browser/browser.css
>+++ b/browser/themes/gnomestripe/browser/browser.css

>+#TabsToolbar toolbarbutton > .toolbarbutton-icon,
>+#TabsToolbar toolbarbutton > .toolbarbutton-menu-dropmarker {
>   margin-top: -2px;
>   margin-bottom: -2px;
> }

You can remove "toolbarbutton > " here.

>--- a/browser/themes/pinstripe/browser/browser.css
>+++ b/browser/themes/pinstripe/browser/browser.css

>+#TabsToolbar toolbarbutton,
>+#TabsToolbar toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button,
>+#TabsToolbar toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-dropmarker {
>   margin: 0;
>   padding: 0;
>   border: none;
>   border-radius: 0;
>   background: none;
>   box-shadow: none;
> }

>+#TabsToolbar toolbarbutton:not([type="menu-button"]),
>+#TabsToolbar toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button,
>+#TabsToolbar toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-dropmarker {
>   -moz-border-start: 2px solid !important;
>   -moz-border-end: none !important;
>   -moz-border-left-colors: rgba(0,0,0,0.25) rgba(255,255,255,0.15) !important;
>   -moz-border-right-colors: rgba(0,0,0,0.25) rgba(255,255,255,0.15) !important;
>   background-clip: padding-box;
>   margin: 0;
>   padding: 0 1px;
> }

And "toolbarbutton[type="menu-button"] > " here.

>+#TabsToolbar toolbarbutton > .toolbarbutton-icon,
>+#TabsToolbar toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
>   padding: 0;
> }

ditto

> .tabbrowser-arrowscrollbox > .scrollbutton-up:not([disabled="true"]):hover,
> .tabbrowser-arrowscrollbox > .scrollbutton-down:not([disabled="true"]):hover,
> #TabsToolbar > toolbarbutton:not([type="menu-button"]):not([disabled="true"]):not([open="true"]):hover,
>-#TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button:not([type="menu-button"]):not([disabled="true"]):not([open="true"]):hover,
>+#TabsToolbar > toolbaritem > toolbarbutton:not([type="menu-button"]):not([disabled="true"]):not([open="true"]):hover,
> #TabsToolbar > toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button:not([disabled="true"]):hover,
>-#TabsToolbar > toolbarbutton[type="menu-button"]:not([disabled="true"]):not([buttonover="true"]):hover > .toolbarbutton-menubutton-dropmarker {
>+#TabsToolbar > toolbaritem > toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button:not([disabled="true"]):hover,
>+#TabsToolbar > toolbarbutton[type="menu-button"]:not([disabled="true"]):not([buttonover="true"]):hover > .toolbarbutton-menubutton-dropmarker,
>+#TabsToolbar > toolbaritem > toolbarbutton[type="menu-button"]:not([disabled="true"]):not([buttonover="true"]):hover > .toolbarbutton-menubutton-dropmarker {
>   background-image: -moz-linear-gradient(transparent, rgba(0,0,0,.15)) !important;
> }
> 
> .tabbrowser-arrowscrollbox > .scrollbutton-up:not([disabled="true"]):hover:active,
> .tabbrowser-arrowscrollbox > .scrollbutton-down:not([disabled="true"]):hover:active,
> #TabsToolbar > toolbarbutton:not([type="menu-button"]):not([disabled="true"]):hover:active,
>-#TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button:not([type="menu-button"]):not([disabled="true"]):hover:active,
>+#TabsToolbar > toolbaritem > toolbarbutton:not([type="menu-button"]):not([disabled="true"]):hover:active,
> #TabsToolbar > toolbarbutton[type="menu"][open="true"],
>+#TabsToolbar > toolbaritem > toolbarbutton[type="menu"][open="true"],
> #TabsToolbar > toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button:not([disabled="true"]):hover:active,
>-#TabsToolbar > toolbarbutton[type="menu-button"][open="true"]:not([disabled="true"]):hover > .toolbarbutton-menubutton-dropmarker {
>+#TabsToolbar > toolbaritem > toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button:not([disabled="true"]):hover:active,
>+#TabsToolbar > toolbarbutton[type="menu-button"][open="true"]:not([disabled="true"]):hover > .toolbarbutton-menubutton-dropmarker,
>+#TabsToolbar > toolbaritem > toolbarbutton[type="menu-button"][open="true"]:not([disabled="true"]):hover > .toolbarbutton-menubutton-dropmarker {
>   background-image: -moz-linear-gradient(transparent, rgba(0,0,0,.3)) !important;
> }

Can you simplify this using #TabsToolbar toolbarbutton:not([type="menu-button"])?

	Geoff Lankow (:darktrojan) Assignee
	Comment 7 • 14 years ago

(In reply to comment #6)
> Can you simplify this using #TabsToolbar
> toolbarbutton:not([type="menu-button"])?

Yes, if I add #navigator-toolbox:not([customizing]).

	Dão Gottwald [:dao]
	Comment 8 • 14 years ago

Some of your selectors match toolbarbuttons they shouldn't match, like the tab close buttons. You should probably use the .toolbarbutton-1 class there like you already do in winstripe.

Also, #navigator-toolbox:not([customizing]) #TabsToolbar should be #navigator-toolbox:not([customizing]) > #TabsToolbar.

	Dão Gottwald [:dao]
	Comment 9 • 14 years ago

Comment on attachment 488366 [details] [diff] [review]
patch v3

Sorry, this needs at least one more iteration. See comment 8.

	Geoff Lankow (:darktrojan) Assignee
	Comment 10 • 14 years ago

Okay, I think (hope) I've got everything I should have and nothing I shouldn't have.

I did some crude checks with the help of document.querySelectorAll, and all is good in winstripe and gnomestripe (#tabs-closebutton isn't matched in gnomestripe but another rule applies the same styles to it anyway). Can't check pinstripe.

This bug is getting tedious. :(

	Dão Gottwald [:dao]
	Comment 11 • 14 years ago

Comment on attachment 489130 [details] [diff] [review]
patch v4

>--- a/browser/themes/gnomestripe/browser/browser.css
>+++ b/browser/themes/gnomestripe/browser/browser.css

>-#TabsToolbar > toolbarbutton > .toolbarbutton-icon,
>-#TabsToolbar > toolbarbutton > .toolbarbutton-menu-dropmarker,
>-#TabsToolbar > toolbarpaletteitem > toolbarbutton > .toolbarbutton-icon,
>-#TabsToolbar > toolbarpaletteitem > toolbarbutton > .toolbarbutton-menu-dropmarker,
>-#TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button > .toolbarbutton-icon,
>-#TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button > .toolbarbutton-menu-dropmarker,
>-#TabsToolbar > toolbarpaletteitem > #bookmarks-menu-button-container > #bookmarks-menu-button > .toolbarbutton-icon,
>-#TabsToolbar > toolbarpaletteitem > #bookmarks-menu-button-container > #bookmarks-menu-button > .toolbarbutton-menu-dropmarker {
>+#TabsToolbar .toolbarbutton-1 > .toolbarbutton-icon,
>+#TabsToolbar .toolbarbutton-1 > .toolbarbutton-menu-dropmarker {
>   margin-top: -2px;
>   margin-bottom: -2px;
> }

I think you need to add #TabsToolbar .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon here.

>diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css
>--- a/browser/themes/pinstripe/browser/browser.css
>+++ b/browser/themes/pinstripe/browser/browser.css

>-.tabs-newtab-button > .toolbarbutton-icon,
>-#TabsToolbar > toolbarbutton > .toolbarbutton-icon,
>-#TabsToolbar > toolbarpaletteitem > toolbarbutton > .toolbarbutton-icon,
>-#TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button > .toolbarbutton-icon,
>-#TabsToolbar > toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
>-#TabsToolbar > toolbarpaletteitem > toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
>+#TabsToolbar .toolbarbutton-icon {
>   padding: 0;
> }
> 
>-.tabbrowser-arrowscrollbox > .scrollbutton-up:not([disabled="true"]):hover,
>-.tabbrowser-arrowscrollbox > .scrollbutton-down:not([disabled="true"]):hover,
>-#TabsToolbar > toolbarbutton:not([type="menu-button"]):not([disabled="true"]):not([open="true"]):hover,
>-#TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button:not([type="menu-button"]):not([disabled="true"]):not([open="true"]):hover,
>-#TabsToolbar > toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button:not([disabled="true"]):hover,
>-#TabsToolbar > toolbarbutton[type="menu-button"]:not([disabled="true"]):not([buttonover="true"]):hover > .toolbarbutton-menubutton-dropmarker {
>+#navigator-toolbox:not([customizing]) > #TabsToolbar toolbarbutton:not([type="menu-button"]):not([disabled]):not([open]):hover,
>+#navigator-toolbox:not([customizing]) > #TabsToolbar .toolbarbutton-menubutton-button:not([disabled]):hover,
>+#navigator-toolbox:not([customizing]) > #TabsToolbar :not([disabled]):not([buttonover]):hover > .toolbarbutton-menubutton-dropmarker {
>   background-image: -moz-linear-gradient(transparent, rgba(0,0,0,.15)) !important;
> }
> 
>-.tabbrowser-arrowscrollbox > .scrollbutton-up:not([disabled="true"]):hover:active,
>-.tabbrowser-arrowscrollbox > .scrollbutton-down:not([disabled="true"]):hover:active,
>-#TabsToolbar > toolbarbutton:not([type="menu-button"]):not([disabled="true"]):hover:active,
>-#TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button:not([type="menu-button"]):not([disabled="true"]):hover:active,
>-#TabsToolbar > toolbarbutton[type="menu"][open="true"],
>-#TabsToolbar > toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button:not([disabled="true"]):hover:active,
>-#TabsToolbar > toolbarbutton[type="menu-button"][open="true"]:not([disabled="true"]):hover > .toolbarbutton-menubutton-dropmarker {
>+#navigator-toolbox:not([customizing]) > #TabsToolbar toolbarbutton:not([type="menu-button"]):not([disabled]):hover:active,
>+#navigator-toolbox:not([customizing]) > #TabsToolbar toolbarbutton[type="menu"][open],
>+#navigator-toolbox:not([customizing]) > #TabsToolbar .toolbarbutton-menubutton-button:not([disabled]):hover:active,
>+#navigator-toolbox:not([customizing]) > #TabsToolbar [open]:not([disabled]):hover > .toolbarbutton-menubutton-dropmarker {
>   background-image: -moz-linear-gradient(transparent, rgba(0,0,0,.3)) !important;
> }

Why are you not using toolbarbutton-1 here?

>--- a/browser/themes/winstripe/browser/browser.css
>+++ b/browser/themes/winstripe/browser/browser.css

> @media all and (-moz-touch-enabled) {
>-  .tabbrowser-arrowscrollbox > .scrollbutton-up,
>-  .tabbrowser-arrowscrollbox > .scrollbutton-down,
>-  #TabsToolbar > toolbarbutton,
>-  #TabsToolbar > toolbarpaletteitem > toolbarbutton,
>-  #TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button {
>+  #TabsToolbar toolbarbutton {
>     min-width: 8.1mozmm;
>   }

This looks like it should use toolbarbutton-1 as well.

	Dão Gottwald [:dao]
	Comment 13 • 14 years ago

Comment on attachment 489976 [details] [diff] [review]
patch v5

> .tabbrowser-arrowscrollbox > .scrollbutton-up:not([disabled="true"]):hover,
> .tabbrowser-arrowscrollbox > .scrollbutton-down:not([disabled="true"]):hover,
>-#TabsToolbar > toolbarbutton:not([type="menu-button"]):not([disabled="true"]):not([open="true"]):hover,
>-#TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button:not([type="menu-button"]):not([disabled="true"]):not([open="true"]):hover,
>-#TabsToolbar > toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button:not([disabled="true"]):hover,
>-#TabsToolbar > toolbarbutton[type="menu-button"]:not([disabled="true"]):not([buttonover="true"]):hover > .toolbarbutton-menubutton-dropmarker {
>+#navigator-toolbox:not([customizing]) > #TabsToolbar .toolbarbutton-1:not([type="menu-button"]):not([disabled]):not([open]):hover,
>+#navigator-toolbox:not([customizing]) > #TabsToolbar .toolbarbutton-1 > .toolbarbutton-menubutton-button:not([disabled]):hover,
>+#navigator-toolbox:not([customizing]) > #TabsToolbar .toolbarbutton-1:not([disabled]):not([buttonover]):hover > .toolbarbutton-menubutton-dropmarker {
>   background-image: -moz-linear-gradient(transparent, rgba(0,0,0,.15)) !important;
> }
> 
> .tabbrowser-arrowscrollbox > .scrollbutton-up:not([disabled="true"]):hover:active,
> .tabbrowser-arrowscrollbox > .scrollbutton-down:not([disabled="true"]):hover:active,
>-#TabsToolbar > toolbarbutton:not([type="menu-button"]):not([disabled="true"]):hover:active,
>-#TabsToolbar > #bookmarks-menu-button-container > #bookmarks-menu-button:not([type="menu-button"]):not([disabled="true"]):hover:active,
>-#TabsToolbar > toolbarbutton[type="menu"][open="true"],
>-#TabsToolbar > toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button:not([disabled="true"]):hover:active,
>-#TabsToolbar > toolbarbutton[type="menu-button"][open="true"]:not([disabled="true"]):hover > .toolbarbutton-menubutton-dropmarker {
>+#navigator-toolbox:not([customizing]) > #TabsToolbar .toolbarbutton-1:not([type="menu-button"]):not([disabled]):hover:active,
>+#navigator-toolbox:not([customizing]) > #TabsToolbar .toolbarbutton-1[type="menu"][open],
>+#navigator-toolbox:not([customizing]) > #TabsToolbar .toolbarbutton-1 > .toolbarbutton-menubutton-button:not([disabled]):hover:active,
>+#navigator-toolbox:not([customizing]) > #TabsToolbar .toolbarbutton-1[open]:not([disabled]):hover > .toolbarbutton-menubutton-dropmarker {
>   background-image: -moz-linear-gradient(transparent, rgba(0,0,0,.3)) !important;
> }

"#navigator-toolbox:not([customizing]) >" isn't actually needed. r=me with that removed!

	Geoff Lankow (:darktrojan) Assignee
	Comment 14 • 14 years ago

(In reply to comment #13)
> Comment on attachment 489976 [details] [diff] [review]
> patch v5

> "#navigator-toolbox:not([customizing]) >" isn't actually needed. r=me with that
> removed!

I thought that those gradients did't apply while customizing the toolbars, because there is "#TabsToolbar > toolbarbutton" without "#TabsToolbar > toolbarpaletteitem > toolbarbutton".

Is that not the case?

	Dão Gottwald [:dao]
	Comment 15 • 14 years ago

It seems that the :hover styling just isn't applied while customizing regardless of "#navigator-toolbox:not([customizing]) >", I don't know why.

	Dão Gottwald [:dao]
	Comment 17 • 14 years ago

Johnath, Gavin, can either of you please approve that patch? This bug affects not only add-ons but even stock toolbar items, namely the back/forward and zoom buttons.

	Benjamin Smedberg
	Comment 18 • 14 years ago

Descendent rules often perform very poorly, has this patch had performance tests run on it?

	Dão Gottwald [:dao]
	Comment 19 • 14 years ago

Other than Twinopen maybe, we don't have such a test and I don't know what exactly it would measure.

This particular patch shouldn't be a problem, since there are only a few nodes with the toolbarbutton-1 class in a normal window (eight over here).

	Dão Gottwald [:dao]
	Comment 20 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/41617bb06c49