337292 - Since searchbox oval is a single hard-coded image, it breaks with the new searchbar flex behavior

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Joe Hughes]

We need to either disable the flex on the Mac or break the searchbox background into parts.

Comment 1

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

Joe, should we back out the flex patch from bug 258679 or IFDEF around it for Mac for A2? This ends up making it possible to have some really odd looking search boxes, and even makes it hard to switch engines in some cases.

Comment 2

[Joe Hughes]

I vote for IFDEFing it out on Mac OS X, so that we still get the benefit of the enlarged searchbox on other platforms, but let's do whatever we need to do, 'cause we shouldn't ship it like this.  Sorry for the breakage.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: ifdef it out for Mac

I think this is all that's needed for reverting this on Mac (the flex=1 on the search bar won't matter, right?). Can someone test this patch on mac?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: really ifdef it out for Mac

Comment 5

[Joe Hughes]

Comment on attachment 221460 [details] [diff] [review]
really ifdef it out for Mac

Tested OK on my Mac.

Comment 6

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

Comment on attachment 221460 [details] [diff] [review]
really ifdef it out for Mac

Yup. This seems like the best case for A2, and we should fix it properly for B1.

Comment 7

[Joe Hughes]

Attachment: This patch makes the searchbar look good at arbitrary widths on the Mac (removing the need for ifdefing the flex on the Mac)

Comment 8

[Joe Hughes]

Attachment: searchbar left image for patch 221523

Comment 9

[Joe Hughes]

Attachment: searchbar middle image for patch 221523

Comment 10

[Joe Hughes]

Attachment: searchbar right image for patch 221523

Comment 11

[Joe Hughes]

BTW, marking severity as "blocker" (relative to a2) since that's what we decided at today's Bon Echo status meeting.

Comment 12

[Pam Greene]

I've tested this on the 1.8 branch in OS X and it looks good.

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 221523 [details] [diff] [review]
This patch makes the searchbar look good at arbitrary widths on the Mac (removing the need for ifdefing the flex on the Mac)

I get the feeling there should be _a_ container around the textbox that can be bound to in the theme then have the binding include the textbox in its <children/>... but I'm not sold to that idea. Let's see what gavin says, then I'll mark sr.

Comment 14

[Joe Hughes]

Attachment: diff -w version of previous patch (also removed the old background image from jar.mn)

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 221523 [details] [diff] [review]
This patch makes the searchbar look good at arbitrary widths on the Mac (removing the need for ifdefing the flex on the Mac)

>Index: browser/components/search/content/search.xml

>       <handler event="keypress" keycode="vk_up" modifiers="accel"
>                phase="capturing"
>-               action="this.parentNode.selectEngine(event, false);"/>
>+               action="this._getParentSearchbar().selectEngine(event, false);"
>+               />

leave the /> on the same line? This file seems to have lost the 80-char-line war a while ago :).

>Index: browser/themes/pinstripe/browser/jar.mn

>   skin/classic/browser/search-bar-background.png

Don't forget to cvs remove this file (in addition to the jar.mn removal from the new patch). Actually, maybe it's best to just change this file to be the "mid" you attached, and not rename it? How do the two differ?

I think this approach is fine. Having it be theme (and thus platform-) specific could lead to unexpected breakage when changing the search bar binding, and it's easier for themes to not have to deal with binding the search bar and instead just style the <box>s if desired.

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 221523 [details] [diff] [review]
This patch makes the searchbar look good at arbitrary widths on the Mac (removing the need for ifdefing the flex on the Mac)

Ok, looks good enough to ship, getting this landed soon for a2

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Landed this on the 1.8 branch with the nit addressed. I haven't cvs removed the now-unused icon yet, can do that later if needed.

Comment 18

[Pam Greene]

Attachment: Misbehavior on Mac after latest patch

Gavin, can you post what actually got checked in?  I don't think it's working anymore: see attached composite screenshot.

I just did a sync and full build, and either my build is obscurely broken, or the final patch wasn't functionally equivalent to Joe's from Comment #7.  This fix doesn't appear to have made it into last night's Mac nightly, so I can't double-check there.

Comment 19

[Joe Hughes]

(In reply to comment #18)
> I just did a sync and full build, and either my build is obscurely broken, or
> the final patch wasn't functionally equivalent to Joe's from Comment #7.  This
> fix doesn't appear to have made it into last night's Mac nightly, so I can't
> double-check there.

Really?  It manifested properly for me in the nightlies I downloaded both at home and here...

Comment 20

[Pam Greene]

The nightly looks good to me too now.  Must be something in my build.  Ignore my comment #18, please.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Just for the record, the only thing different between my branch checkin and attachment 221634 [details] [diff] [review] is my nit from comment 15 about the "/>", and attachment 221634 [details] [diff] [review] only differs from attachment 221523 [details] [diff] [review] in that it removes search-bar-background.png (unused) from jar.mn.

Comment 22

[Joe Hughes]

I also landed this on trunk, and removed the unused image that gavin had left in on his branch landing.