Closed Bug 579632 Opened 14 years ago Closed 14 years ago

Fix OS X tabs-on-top styling and put tabs on top by default

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b2
Tracking Status
blocking2.0 --- beta2+

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(3 files, 1 obsolete file)

No description provided.
Attached patch patch (obsolete) (deleted) — Splinter Review
I *think* this works ok, but I couldn't test it on OS X. Eventually this might end up being implemented differently, but I don't want to rush bug 547787 and bug 562733 into the beta.
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #458076 - Flags: review?(gavin.sharp)
I've found two problems with this: - the "new tab" icon is squeezed vertically - in tabs-on-top mode, the border of app tabs extends 1px into the lighter toolbar below. Other than that it's a definitive improvement over the current state. I'll fix up your patch and then rebase bug 547787 on top of this. I hope you realize how frustrating this process is for me.
The new tab icon is squeezed because the toolbarbutton has 1px to much vertical padding. The app tab spilling is because the nav bar has margin-top: -1px, and thus overlays normal tabs but not position:fixed app tabs. I've found another problem: The favicon on app tabs in 2px higher than on normal tabs. I'm not sure where this comes from, but it's directly related to position:fixed. Setting style="position:static" on the app tab fixes the vertical position of the favicon. Maybe position:fixed somehow interferes with box-align.
Attached patch patch v2 (deleted) — Splinter Review
I think this fixes the mentioned issues, but again I'm not quite sure...
Attachment #458076 - Attachment is obsolete: true
Attachment #458092 - Flags: review?(mstange)
Attachment #458092 - Flags: review?(gavin.sharp)
Attachment #458076 - Flags: review?(gavin.sharp)
Comment on attachment 458092 [details] [diff] [review] patch v2 Bingo, it does. Maybe add an XXX comment to the favicon position workaround since we're not sure why it's necessary. The 5px box shadow blur worries me a little, but we can still change that if Txul complains, and it's (hopefully) only temporary anyway.
Attachment #458092 - Flags: review?(mstange) → review+
And for the record, the app tab favicon issue wasn't caused by this patch, it's present on trunk already.
(In reply to comment #6) > And for the record, the app tab favicon issue wasn't caused by this patch, it's > present on trunk already. Yep, it's also an issue on all platforms. box-align doesn't work there; I'm not sure whether we should expect it to work. (In reply to comment #2) > I hope you realize how frustrating this process is for me. I'm sorry, I know about your efforts -- the result is quite fascinating in a way -- and I realize that you'd like to see that stuff land rather than spending more time it. However, there's no "looks great, let's land it" automation. I'm not sure what the app tab story is with your patches, but even if we can fix this and other issues that might arise before the fx4 release, I'd still be worried about what happens after that. We're unlikely to freeze the UI or tabbrowser features, so I'd like to make sure what lands is not a dead end. I'm still very skeptical about whether the styling is going to be manageable by anyone but yourself (or manageable by yourself even if a tricky enough problem pops up).
Markus: I also appreciate your patience and dedication - is there something we can do better to keep everyone on the same page vis-a-vis work? Our goals are shared, I feel like it's our workflows which are not.
Also: can someone post a screenshot of this for Steven's perusal? Or Steven, can you apply this patch and let us know if it's OK for beta 2?
(In reply to comment #8) > is there something we > can do better to keep everyone on the same page vis-a-vis work? That's a good question, but at the moment I can't think of anything. I'll think about it some more. I'm going to continue my conversation with Dão over email.
Depends on: 579776
Markus: is that dependency just to get App-Tabs working properly?
blocking2.0: --- → beta2+
(In reply to comment #9) > Also: can someone post a screenshot of this for Steven's perusal? Or Steven, > can you apply this patch and let us know if it's OK for beta 2? Looks a lot better than what we have now and is ok for beta 2 until bug 547787 and bug 562733 are ready.
(In reply to comment #13) > Markus: is that dependency just to get App-Tabs working properly? Right. We don't really depend on it in this bug because the patch in this bug contains a workaround for it, but it's necessary if we want to remove the workaround.
No longer depends on: 579776
Attachment #458092 - Flags: review?(gavin.sharp) → review+
Thanks, Gavin - Markus: want to check this in?
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b2
Blocks: 580220
Tabs are still not on top by default in a new profile. Is a new bug needed?
(In reply to comment #18) > Tabs are still not on top by default in a new profile. Is a new bug needed? If that's true, then yes, a bug should be filed.
(In reply to comment #18) > Tabs are still not on top by default in a new profile. WFM, fwiw.
Yeah, WFM now too.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: