fix toolbar background on Mac

RESOLVED FIXED in Firefox 2

Status

()

defect
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: mconnor, Assigned: mconnor)

Tracking

({fixed1.8.1})

unspecified
Firefox 2
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

the original plan was to be more unified-like, and we seem to have lost that a little.  We can tweak this by reusing the bookmark toolbar CSS, and cleaning that up a little, patch upcoming for late inclusion, already got signoff from mscott since this affects Thunderbird as well.
Posted patch fix up toolbars (obsolete) — Splinter Review
Attachment #240426 - Flags: review?(beltzner)
Attachment #240426 - Flags: approval1.8.1?
Comment on attachment 240426 [details] [diff] [review]
fix up toolbars

Looks good here. r=beltzner
Attachment #240426 - Flags: review?(beltzner) → review+
fix a couple nits, let's land this and be done with it
Attachment #240426 - Attachment is obsolete: true
Attachment #240429 - Flags: approval1.8.1?
Attachment #240426 - Flags: approval1.8.1?
Comment on attachment 240429 [details] [diff] [review]
clean up a bit more

a=beltzner for drivers
Attachment #240429 - Flags: approval1.8.1? → approval1.8.1+
I actually liked it white!  It looked a great deal better then the light gray.
(In reply to comment #3)
> Created an attachment (id=240429) [edit]

This looks wrong to me:

>+  background-image: url("chrome://global/skin/toolbar/toolbar-background.gif") repeat-x;
Keywords: fixed1.8.1
(In reply to comment #7)
> Created an attachment (id=247597) [edit]
> trunk patch

Sorry, I have to ask again: Why repeat-x as a value for background-image? Is this a Mozilla quirk?
Dão: you'll have better luck if you directly state your objection in detail, rather than making vague statements like "this looks wrong." If instead you say:

Mano: if you're going to break up a "background" shorthand rule into separate rules, I think you should put the repeat into "background-repeat" where the specification says it belongs, rather than tacking it onto the end of the "background-image" rule, since even if that does happen to work in XUL, there's no reason why it should, nor reason to expect it to keep working.

then you're more likely to get a reaction.
Attachment #247597 - Flags: review?(mconnor) → review+
the graphic used is basically adding a groove to the top of the toolbar, with normal repeat that would show in the wrong place, unless the image was super-tall.  repeat-x is a w3c standard, but I misused it in the original patch.
What's that you say? Throw more words at this?

Nobody's saying it isn't a standard, what they are saying is that with our previous, and proposed future, use of "background-image: ... repeat-x" we are either doing absolutely nothing, no repeat and no image, if our invalid rule is properly being dropped on the floor, or we are exploiting a bug in our parsing of CSS (which I don't think we are, I think we're doing nothing), because while "background: ... repeat-x" is valid, if we use individual rules rather than the single "background:" shorthand rule, we need to instead use "background-image: url(...); background-repeat: repeat-x;"

So, rather than explaining the intent, didn't you mean "r-, please fix my mistake and request branch approval for a fix there, too"?
Ah, no wonder it seems to work in 2.0: just to keep people on their toes, attachment 240429 [details] [diff] [review] wasn't actually what you checked in to the branch - sometime between when you attached it and when you checked it in, you realized that you couldn't tack a repeat-x on at the end of a background-image, so you fixed what you checked in, but not the patch that Mano's now ported to the trunk. I was starting to really wonder wtf, until I realized that (and that the image isn't actually applied to the navigation toolbar).
Posted patch as checked inSplinter Review
mozilla/browser/themes/pinstripe/browser/browser.css 1.37
mozilla/toolkit/themes/pinstripe/global/jar.mn 1.25
mozilla/toolkit/themes/pinstripe/global/toolbar.css 1.7
mozilla/toolkit/themes/pinstripe/global/toolbar/toolbar-background.gif 1.2
Attachment #247597 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 2
Looks like this broke the address bar on the 120706 trunk builds?

Will include image.
Posted image Broken address bar
(In reply to comment #15)
> Created an attachment (id=247894) [edit]
> Broken address bar

I'd say that's unrelated, since the patch touches background rules only.
I filed bug 363125 for that broken location bar
Depends on: 363125
No longer depends on: 363125
Blocks: 370426
You need to log in before you can comment on or make changes to this bug.