Closed Bug 604531 Opened 9 years ago Closed 9 years ago

Styles for toolbarbutton inside #TabsToolbar not applied if it's also inside a toolbaritem

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 4.0b8

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(1 file, 5 obsolete files)

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 {
Gah, first three lines of the second block should also have > selectors, I didn't undo far enough before I copied it. :(
I think we need to go with the first option. :(
Assignee: nobody → geoff
OS: Windows Vista → All
Hardware: x86 → All
Summary: Styles for .toolbarbutton-1 inside #TabsToolbar not applied if it's also inside a toolbaritem → Styles for toolbarbutton inside #TabsToolbar not applied if it's also inside a toolbaritem
Attached patch patch (obsolete) — Splinter Review
Seems to occur in more places than I first expected. I've only tested this out on Windows.
Attachment #483667 - Flags: review?(dao)
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.
Attached patch patch v2 (obsolete) — Splinter Review
(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.
Attachment #483667 - Attachment is obsolete: true
Attachment #483667 - Flags: review?(dao)
Attachment #483915 - Flags: review?(dao)
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"])?
Attached patch patch v3 (obsolete) — Splinter Review
(In reply to comment #6)
> Can you simplify this using #TabsToolbar
> toolbarbutton:not([type="menu-button"])?

Yes, if I add #navigator-toolbox:not([customizing]).
Attachment #483915 - Attachment is obsolete: true
Attachment #488366 - Flags: review?(dao)
Attachment #483915 - Flags: review?(dao)
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.
Comment on attachment 488366 [details] [diff] [review]
patch v3

Sorry, this needs at least one more iteration. See comment 8.
Attachment #488366 - Flags: review?(dao) → review-
Attached patch patch v4 (obsolete) — Splinter Review
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. :(
Attachment #488366 - Attachment is obsolete: true
Attachment #489130 - Flags: review?(dao)
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.
Attached patch patch v5 (obsolete) — Splinter Review
Attachment #489130 - Attachment is obsolete: true
Attachment #489976 - Flags: review?(dao)
Attachment #489130 - Flags: review?(dao)
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!
Attachment #489976 - Flags: review?(dao) → review+
(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?
It seems that the :hover styling just isn't applied while customizing regardless of "#navigator-toolbox:not([customizing]) >", I don't know why.
Attached patch patch v6Splinter Review
Attachment #489976 - Attachment is obsolete: true
Attachment #490218 - Flags: approval2.0?
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.
Blocks: 598920
Descendent rules often perform very poorly, has this patch had performance tests run on it?
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).
Attachment #490218 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/41617bb06c49
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
You need to log in before you can comment on or make changes to this bug.