593967 - Add more elements into tabbrowser tabs for easier stylability

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Markus Stange [:mstange]]

Attachment: v1

This is needed for a better tab appearance, e.g. bug 547787 and bug 570279.

This patch has two parts: the actual addition of elements in tabbrowser.xml, and a mechanical change to all instances of browser.css and tabbrowser.css, which is just an automatic replacement of " > .tab-" with " > * > * > .tab-".

Comment 1

[Markus Stange [:mstange]]

Attachment: v2

updated to trunk

Comment 2

[Markus Stange [:mstange]]

This is necessary for blocking bug 547787, and it affects add-on compatibility, so it needs to block beta6.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 472756 [details] [diff] [review]
v2

Let's clean this up a bit and make it more forward-compatible by adding tabbrowser specific classes and inherting some attributes where it makes sense (preferably with a "tab" prefix, I think, e.g. fadein as tabfadein -- although fadein may be a case where inheriting isn't worth it).

Comment 4

[Markus Stange [:mstange]]

Can you write the patch that does that? You know this code much better than I do.

Comment 5

[Dietrich Ayala (:dietrich)]

Blocks a blocker -> blocking+.

Comment 6

[Dão Gottwald [:dao]]

Attachment: wip

Only tested on Linux so far.

This doesn't add tab-background-*. This needs to be done in bug 547787.

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch

Comment 8

[Dão Gottwald [:dao]]

tab-icon-image and tab-close-button are already tabbrowser-specific, so I didn't have to invent new classes. For the label I chose tab-label instead of tabbrowser-tab-label or tabbrowser-tab-text as it would be the only class following a tabbrowser-tab-* pattern.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>         <toolbarbutton id="allTabs-tab-close-button"
>-                       class="tab-close-button"
>+                       class="tabs-closebutton"

I don't really understand what the differences between these two are supposed to be... tab-close-button is for the buttons on the tabs, and tabs-closebutton is for all the others? Why doesn't this one want to be like the button on the tab? Wish we'd used more distinctive names...

There are .tab-close-button and .tab-label rules in both browser.css and tabbrowser.css, and tab-icon-image rules are only in browser.css - shouldn't this be more consistent?

>diff --git a/browser/base/content/tabbrowser.css b/browser/base/content/tabbrowser.css

>-.tabbrowser-tab[pinned] > .tab-icon-image {
>+.tab-stack {
>+  vertical-align: middle; /* for pinned tabs */

Why change to vertical-aligning the entire stack rather than just the image?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

We should probably get this in sooner rather than later (i.e. in b7).

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #9)
> I don't really understand what the differences between these two are supposed
> to be... tab-close-button is for the buttons on the tabs, and tabs-closebutton
> is for all the others? Why doesn't this one want to be like the button on the
> tab? Wish we'd used more distinctive names...

tabs-closebutton was really supposed to be used only for the tab close button at the end of the tab strip, but it leaked into different places.

> There are .tab-close-button and .tab-label rules in both browser.css and
> tabbrowser.css, and tab-icon-image rules are only in browser.css - shouldn't
> this be more consistent?

Yes, I tried to slightly improve it by leaving only the rules related to the animation in browser.css...

> >diff --git a/browser/base/content/tabbrowser.css b/browser/base/content/tabbrowser.css
> 
> >-.tabbrowser-tab[pinned] > .tab-icon-image {
> >+.tab-stack {
> >+  vertical-align: middle; /* for pinned tabs */
> 
> Why change to vertical-aligning the entire stack rather than just the image?

Just the image doesn't work, as the image's parent isn't display:block, but the stack's parent is.

Comment 12

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/875f1912a091

Comment 13

[Markus Stange [:mstange]]

Thanks Dão!