Closed Bug 438302 Opened 12 years ago Closed 11 years ago

Remove Pinstripe text-shadow hacks

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.1b2

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 4 obsolete files)

Once bug 10713 is fixed, we should kill the XBL hack that currently creates the shadow effect in the Mac theme.
No you can't. See bug 10713 comment #159.
great
Depends on: 438517
No longer depends on: text-shadow
No longer blocks: 434348
Attached patch possible patch (obsolete) — Splinter Review
Dammit, I really should have searched for this before I spent some time trying to actually do this. Anyway here is a patch that I think is about right assuming bug 438517 is ever fixed.
Assignee: nobody → dao
Summary: Use CSS text-shadow for tabbrowser tabs on OS X → Remove Pinstripe text-shadow hacks
Attached patch patch (obsolete) — Splinter Review
Attachment #334776 - Attachment is obsolete: true
Attachment #341247 - Flags: review?(rflint)
Attached patch patch v2 (obsolete) — Splinter Review
actually, I don't think we want to add the shadow to all toolbar buttons...
Attachment #341247 - Attachment is obsolete: true
Attachment #341248 - Flags: review?(rflint)
Attachment #341247 - Flags: review?(rflint)
Attached patch patch v2 (obsolete) — Splinter Review
tabbox.css was missing
Attachment #341248 - Attachment is obsolete: true
Attachment #341253 - Flags: review?(rflint)
Attachment #341248 - Flags: review?(rflint)
(In reply to comment #5)
> Created an attachment (id=341248) [details]
> patch v2
> 
> actually, I don't think we want to add the shadow to all toolbar buttons...

Why not? That's what I did when I worked on it this morning... :)
And I think text-shadow: 0 1px rgba(255, 255, 255, 0.4); would be more accurate, even though it usually doesn't make a difference.
(In reply to comment #7)
> (In reply to comment #5)
> > Created an attachment (id=341248) [details] [details]
> > patch v2
> > 
> > actually, I don't think we want to add the shadow to all toolbar buttons...
> 
> Why not?

Are other OS X apps doing that?

(In reply to comment #8)
> And I think text-shadow: 0 1px rgba(255, 255, 255, 0.4); would be more
> accurate, even though it usually doesn't make a difference.

Dunno, rgb(240, 240, 240) at 50% opacity is what pinstripe uses currently.
(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > Created an attachment (id=341248) [details] [details] [details]
> > > patch v2
> > > 
> > > actually, I don't think we want to add the shadow to all toolbar buttons...
> > 
> > Why not?
> 
> Are other OS X apps doing that?

Yes. On OS X, all text that's directly on the dark grey chrome background has a shadow. And since we're styling toolbars with that color by default, I think we should make the text-shadow the default, too.
Attached patch patch v3Splinter Review
Now again for all toolbar buttons and with rgba(255, 255, 255, 0.4) (although I don't know why that's supposed to be better.)
Apparently we can also remove pinstripe's tabbrowser-tab binding, as Mossop already did it correctly in his first patch.
Attachment #341253 - Attachment is obsolete: true
Attachment #341273 - Flags: review?(rflint)
Attachment #341253 - Flags: review?(rflint)
Blocks: 458111
Blocks: 458197
Attachment #341273 - Flags: review?(rflint) → review+
Comment on attachment 341273 [details] [diff] [review]
patch v3

Please wait until after the current txul regression is sorted out if you're planning on landing this for beta 1.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/ffd6a43042b9
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b2
Hardware: PC → All
Depends on: 477693
Verified by check-in on trunk.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.