Closed Bug 486476 Opened 14 years ago Closed 13 years ago

Some findbar polish

Categories

(Toolkit :: Themes, defect)

All
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Keywords: polish, Whiteboard: [polish-easy] [polish-visual][polish-p2])

Attachments

(2 files, 3 obsolete files)

Attached image 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.
Attachment #370601 - Flags: ui-review?(faaborg)
Whiteboard: [polish-easy] [polish-visual]
Attached patch fix v1 (obsolete) — Splinter Review
Attachment #370614 - Flags: review?(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?
Attachment #370614 - Flags: review?(dao) → review-
> >+  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?
(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.
(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.
Blocks: 414690
(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...
(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.
Attached patch v2 (obsolete) — Splinter Review
Attachment #370614 - Attachment is obsolete: true
Attachment #375963 - Flags: review?(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?
Attached patch v3 (obsolete) — Splinter Review
Sure.
I chose to use padding: 3px; since that's shorter and looks better.
Attachment #375963 - Attachment is obsolete: true
Attachment #377431 - Flags: review?(dao)
Attachment #375963 - Flags: review?(dao)
The buttons seem to be aligned one pixel lower than the textbox...
Attachment #377431 - Flags: review?(dao)
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.
Whiteboard: [polish-easy] [polish-visual] → [polish-easy] [polish-visual][polish-p2]
Attachment #370601 - Flags: ui-review?(faaborg) → ui-review+
Attached patch v4Splinter Review
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.
Attachment #377431 - Attachment is obsolete: true
Attachment #388662 - Flags: review?(dao)
Attachment #388662 - Flags: review?(dao) → review+
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;?
(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.
I think it's superfluous here, yes.
http://hg.mozilla.org/mozilla-central/rev/104e83235e83
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.