347400 - search button disconnected from search field

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Asa Dotzler [:asa]]

The search button is disconnected from the search field in the primary toolbar. this is fine on windows but broken on mac.

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: looks good on windows, but not mac

Comment 2

[Asa Dotzler [:asa]]

Attachment: search button disconnect on Mac

Comment 3

[Marcia Knous [:marcia]]

*** Bug 347787 has been marked as a duplicate of this bug. ***

Comment 4

[Mike Connor [:mconnor]]

Pam, can you take a look here?

Comment 5

[Pam Greene]

A similar space, I presume extraneous, appears between the location bar and its Go button.

Comment 6

[Tracy Walker [:tracy]]

no the go button is only slightly disconnected from the location bar. I believe there is a bug on that as well.

Comment 7

[Asa Dotzler [:asa]]

that's bug 347405. 

Comment 8

[Vladimir Lushnikov]

This happens on Linux (Gentoo gcc 4.1.1 glibc 2.4 cairo 1.2) too.

http://www.vladsharp.com/images/work/search_button_mouseover.png

Comment 9

[Pam Greene]

Attachment: Screenshot of fix on Mac and Windows

Comment 10

[Pam Greene]

Attachment: Glow background for Mac

This will need to be added as
browser/themes/pinstripe/browser/Search-provider-bkgnd.png

In addition, the following images in browser/themes/pinstripe/browser/ are no longer needed and will be removed:
Search.png
Search-add-engines.png
search-bar-background-left.png
search-bar-background-right.png

Finally, the image currently named Search-add-engines.png will be re-added as Search-go.png.

Comment 11

[Pam Greene]

Attachment: Fixes searchbar appearance on Windows and Mac

This patch rolls up fixes for this bug as well as bug 347961.

Comment 12

[Pam Greene]

Jay, I'd appreciate it if you could take a look at the CSS parts of this patch.  In particular, I'm not at all familiar with what's needed for RTL, nor do I know how to test that situation.

Comment 13

[(not reading, please use [e-mail] instead)]

pam, the way I have been testing RTL is by adding this to my userChrome.css file:

:root { direction : rtl }

(cc'ing asaf and simon, kings of RTL)

Comment 14

[Simon Montagu :smontagu]

(In reply to comment #13)
> pam, the way I have been testing RTL is by adding this to my userChrome.css
> file:
> 
> :root { direction : rtl }

To test something like this which depends on chromedir, you also need to set &locale.dir; to rtl. We need to find a way to do that at run-time, or create an rtl pseudo-l10n.

Comment 15

[Pam Greene]

Comment on attachment 233164 [details] [diff] [review]
Fixes searchbar appearance on Windows and Mac

Scratch this.  RTL-enabled patch coming soon.

Comment 16

[Vladimir Lushnikov]

This is off topic - but what is RTL?

Comment 17

[Pam Greene]

Right-to-left, in contrast to LTR as a direction for text.

Comment 18

[Vladimir Lushnikov]

Ok, that makes more sense than "Real Time Linking" - thank you.

Comment 19

[Pam Greene]

Attachment: New Search-rtl.png for winstripe

This image has the four individual frames flipped rather than the whole thing, to simplify the CSS rules.

Comment 20

[Pam Greene]

Attachment: Search-go-rtl.png for pinstripe

Comment 21

[Pam Greene]

Attachment: Search-provider-bkgnd-rtl.png for pinstripe

Comment 22

[Pam Greene]

Attachment: RTL screenshots for Mac and Windows

Comment 23

[Pam Greene]

Attachment: Fixed searchboxes, including RTL

To summarize, in addition to applying this patch, the following changes will be needed:

In pinstripe/browser/:
  add Search-provider-bkgnd.png
  add Search-provider-bkgnd-rtl.png
  add Search-go-rtl.png
  update Search-go.png with what's currently Search-add-engines.png
  remove Search.png
  remove Search-add-engines.png
  remove search-bar-background-left.png
  remove search-bar-background-right.png
  remove Search-bar.png

In winstripe/browser/:
  update Search-rtl.png
  remove Search-add-engines.png
  remove Search-bar.png

Comment 24

[Mike Connor [:mconnor]]

Comment on attachment 233253 [details] [diff] [review]
Fixed searchboxes, including RTL

>   <binding id="searchbar"
>            extends="chrome://browser/content/search/search.xml#searchbar-base">
>     <content>
>       <xul:stringbundle src="chrome://browser/locale/search.properties"
>                         anonid="searchbar-stringbundle"/>
>       <xul:box class="searchbar-left" flex="1">
>         <xul:box class="searchbar-right" flex="1">

*snip*

why do we have nested boxes like this?  seems like its just cruft, and shouldn't be at _all_ necessary.

>         </xul:box>
>       </xul:box>
>     </content>


I'm not completely sure about this from a UI perspective, but trying to keep the button inside of the textbox is a pain, so I'm inclined to try it for beta2.

Going to approve as well so we can get on branch ASAP and get more feedback.

Comment 25

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

(In reply to comment #24)
> why do we have nested boxes like this?  seems like its just cruft, and
> shouldn't be at _all_ necessary.

It's needed for the Mac search box styling, you reviewed the patch to add it in bug 337292 :)

Comment 26

[Pam Greene]

(In reply to comment #24)
 
> why do we have nested boxes like this?  seems like its just cruft, and
> shouldn't be at _all_ necessary.

Like Gavin said, it used to be used for the images forming the ends of the Mac searchbar.  It's not needed anymore, since the ends are occupied by the two buttons now.  I left it in with the idea that it'd let a theme author play around with those parts, but I don't know anything about theme writing, so perhaps that's pointless?

Comment 27

[Mike Connor [:mconnor]]

Hey Pam, you weren't on IRC, so I went ahead and landed this on the 1.8 branch since we're trying to do as much of a run as we can tonight on getting new theme pieces landed.  As gavin pointed out, the nested boxes are just evil Mac pain, so we'll live for now.

Thanks!

Comment 28

[Pam Greene]

Attachment: Updated Search-rtl.png for winstripe

I notice that winstripe's Search.png got updated in tonight's landing.  I've checked in this new Search-rtl.png to match.

Eventually we'll want to clear out the unused images, too, but they're not in the jar.mn, so they're no real harm.