Closed Bug 593967 Opened 14 years ago Closed 14 years ago

Add more elements into tabbrowser tabs for easier stylability

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: mstange, Assigned: dao)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
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-".
Attachment #472611 - Flags: review?(dao)
Attached patch v2 (obsolete) — Splinter Review
updated to trunk
Attachment #472611 - Attachment is obsolete: true
Attachment #472756 - Flags: review?(dao)
Attachment #472611 - Flags: review?(dao)
blocking2.0: --- → ?
This is necessary for blocking bug 547787, and it affects add-on compatibility, so it needs to block beta6.
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).
Attachment #472756 - Flags: review?(dao) → review-
Can you write the patch that does that? You know this code much better than I do.
Blocks a blocker -> blocking+.
blocking2.0: ? → betaN+
Assignee: mstange → dao
OS: Mac OS X → All
Hardware: x86 → All
Depends on: 593690
Attached patch wip (obsolete) — Splinter Review
Only tested on Linux so far.

This doesn't add tab-background-*. This needs to be done in bug 547787.
Attachment #472756 - Attachment is obsolete: true
Blocks: 544818
Attached patch patchSplinter Review
Attachment #475051 - Attachment is obsolete: true
Attachment #475089 - Flags: review?(gavin.sharp)
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 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?
Attachment #475089 - Flags: review?(gavin.sharp) → review+
We should probably get this in sooner rather than later (i.e. in b7).
blocking2.0: betaN+ → beta7+
(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.
http://hg.mozilla.org/mozilla-central/rev/875f1912a091
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7
Thanks Dão!
No longer blocks: 544818
Blocks: 544818
Depends on: 601228
You need to log in before you can comment on or make changes to this bug.