486476 - Some findbar polish

Status: VERIFIED FIXED

(Toolkit :: Themes, defect)

Description

[Markus Stange [:mstange]]

Attachment: screenshot

See screenshot. Differences:
 - Buttons get a lighter border to make them look more like native "scope bar
   buttons",
 - their disabled state is only reflected in their text color, not in their
   border color (this is how native scope bar buttons behave, too),
 - the search field's border is removed and its inset box shadow and search
   icon are adjusted,
 - the "Match case" checkbox is lighter to match the lighter border color of
   the buttons,
 - a little more text-shadow goodness and
 - all the text in the findbar now has the same baseline.

Comment 1

[Markus Stange [:mstange]]

Attachment: fix v1

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 370614 [details] [diff] [review]
fix v1

> .findbar-closebutton {
>   padding-right: 4px;
>   list-style-image: url("chrome://global/skin/icons/closetab.png");
>   border: none;
>   background-image: none !important;
>   background-color: transparent !important;
>+  -moz-box-shadow: none !important;
> }

This seems unnecessary.

> .findbar-find-next,
> .findbar-find-previous,
> .findbar-highlight {
>   -moz-appearance: none;
>-  border: 1px solid #5F5F5F;
>+  border: 1px solid rgba(0, 0, 0, .4);
>   -moz-border-radius: 100%;
>-  background: url("chrome://global/skin/icons/white-gray-gradient.gif") #A09E9D repeat-x top center;
>-  margin: 0 4px 1px 4px;
>+  background: url("chrome://global/skin/toolbar/white-transparent-gradient.png") #C3C3C3 repeat-x top center;

white-transparent-gradient.png doesn't seem to exist. In that case, it might make sense to use a box-shadow instead.

>diff --git a/toolkit/themes/pinstripe/global/icons/search-textbox.png b/toolkit/themes/pinstripe/global/icons/search-textbox.png

Will this change affect the search bar?

Comment 3

[Dão Gottwald [:dao]]

> >+  background: url("chrome://global/skin/toolbar/white-transparent-gradient.png") #C3C3C3 repeat-x top center;
> 
> white-transparent-gradient.png doesn't seem to exist. In that case, it might
> make sense to use a box-shadow instead.

Also, is there a native color that can be used instead of #C3C3C3?

Comment 4

[Markus Stange [:mstange]]

(In reply to comment #2)
> white-transparent-gradient.png doesn't seem to exist. In that case, it might
> make sense to use a box-shadow instead.

Uh, I totally forgot to mention that the patch depends on the patch in bug 477827.

Comment 5

[Markus Stange [:mstange]]

(In reply to comment #2)
> (From update of attachment 370614 [details] [diff] [review])
> > .findbar-closebutton {
> >   padding-right: 4px;
> >   list-style-image: url("chrome://global/skin/icons/closetab.png");
> >   border: none;
> >   background-image: none !important;
> >   background-color: transparent !important;
> >+  -moz-box-shadow: none !important;
> > }
> 
> This seems unnecessary.

This overrides the box-shadow from .findbar-container > toolbarbutton:not([disabled]):hover:active.

> white-transparent-gradient.png doesn't seem to exist.

Now it exists. :-)

> In that case, it might
> make sense to use a box-shadow instead.

I don't think we should use box-shadows in places where an image gets the same job done simpler.

> >diff --git a/toolkit/themes/pinstripe/global/icons/search-textbox.png b/toolkit/themes/pinstripe/global/icons/search-textbox.png
> 
> Will this change affect the search bar?

Nope, but it seems like it will affect certain autocomplete result rows:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/browser.css#929
Do you know in which cases these styles apply and which buttons to press to see them?
The new image is only 12x12, so there's a slight chance that this might break something.

Comment 6

[Markus Stange [:mstange]]

(In reply to comment #3)
> Also, is there a native color that can be used instead of #C3C3C3?

Not that I know of. And using it would only make sense if its name was
-moz-mac-roundedrectbuttonbackground...

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #5)
> This overrides the box-shadow from .findbar-container >
> toolbarbutton:not([disabled]):hover:active.

Seems like .findbar-find-next, .findbar-find-previous, .findbar-highlight should be used there.

> Nope, but it seems like it will affect certain autocomplete result rows:
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/browser.css#929
> Do you know in which cases these styles apply and which buttons to press to see
> them?

Right click in an input field and select "Add a Keyword for this Search...". After adding the keyword, type it in the urlbar.

Comment 8

[Markus Stange [:mstange]]

Attachment: v2

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 375963 [details] [diff] [review]
v2

> .findbar-closebutton {
>   padding-right: 4px;
>   list-style-image: url("chrome://global/skin/icons/closetab.png");
>   border: none;
>   background-image: none !important;
>   background-color: transparent !important;

Now you're not touching this anymore, but as you're nearby, can some of that stuff be removed? background-color and background-image? Should padding-right be -moz-padding-start instead?

Comment 10

[Markus Stange [:mstange]]

Attachment: v3

Sure.
I chose to use padding: 3px; since that's shorter and looks better.

Comment 11

[Dão Gottwald [:dao]]

The buttons seem to be aligned one pixel lower than the textbox...

Comment 12

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.

Comment 13

[Markus Stange [:mstange]]

Attachment: v4

I've moved the buttons 1px up. Now their bottom edge doesn't align with the searchfield's bottom edge, but that's not as noticable.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 388662 [details] [diff] [review]
v4

> .findbar-closebutton {
>-  padding-right: 4px;
>+  padding: 0;
>+  margin: 0 4px;
>   list-style-image: url("chrome://global/skin/icons/closetab.png");
>+  -moz-image-region: auto;
>   border: none;
>-  background-image: none !important;
>-  background-color: transparent !important;
> }

Why -moz-image-region: auto;?

Comment 15

[Markus Stange [:mstange]]

(In reply to comment #14)
> Why -moz-image-region: auto;?

Because I think whenever list-style-image is specified, -moz-image-region should be specified too, in order to avoid things like bug 497723. Though in this case it probably doesn't matter because there's no reason anyone would set a -moz-image-region on a parent element of the findbar, so I can take it out if you'd like me to.

Comment 16

[Dão Gottwald [:dao]]

I think it's superfluous here, yes.

Comment 17

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/104e83235e83

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Looks great! Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090722 Minefield/3.6a1pre