354644 - fix toolbar background on Mac

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mike Connor [:mconnor]]

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.

Comment 1

[Mike Connor [:mconnor]]

Attachment: fix up toolbars

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 240426 [details] [diff] [review]
fix up toolbars

Looks good here. r=beltzner

Comment 3

[Mike Connor [:mconnor]]

Attachment: clean up a bit more

fix a couple nits, let's land this and be done with it

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 240429 [details] [diff] [review]
clean up a bit more

a=beltzner for drivers

Comment 5

[Shawn Smith]

I actually liked it white!  It looked a great deal better then the light gray.

Comment 6

[Dão Gottwald [:dao]]

(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;

Comment 7

[Asaf Romano (gone)]

Attachment: trunk patch

Comment 8

[Dão Gottwald [:dao]]

(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?

Comment 9

[Phil Ringnalda (:philor)]

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.

Comment 10

[Mike Connor [:mconnor]]

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.

Comment 11

[Phil Ringnalda (:philor)]

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"?

Comment 12

[Phil Ringnalda (:philor)]

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).

Comment 13

[Asaf Romano (gone)]

Attachment: as checked in

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

Comment 14

[Shawn Smith]

Looks like this broke the address bar on the 120706 trunk builds?

Will include image.

Comment 15

[Shawn Smith]

Attachment: Broken address bar

Comment 16

[Dão Gottwald [:dao]]

(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.

Comment 17

[philippe (part-time)]

I filed bug 363125 for that broken location bar