408530 - Fix Find toolbar appearance/UI

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

"Fixing the visual appearance of the bar (background, spacing, etc.)"

* background
  - not sure what Stuart meant there, texture on 10.5?
* button/element spacing and alignment
  - our standard for buttons is 12px of space, but we've never done small 
    buttons before, so 8px is probably OK there
  - "Match Case" has too large of a click area; its bounds should just contain 
    its text, per our guidelines
  - because of the small vertical size of the bar and baseline alignment of all 
    the text, some of the items are not centered vertically and may look odd
  - perhaps explore some different button styles
* anything else?

Comment 2

[Stuart Morgan]

From my dup:
* height of the bar
* color of the "no match" text (I think it's perhaps too red)

For background, I was thinking we may want to play with texture or gradient.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #2)
> For background, I was thinking we may want to play with texture or gradient.

On 10.3, we have the pinstripes in the Find bar, so I don't think it looks too bad there.  On 10.5, it doesn't look quite right, for sure.

I don't think I'd object to the Aqua "accessory light blue color", if we wanted to go that route.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #0)
> * button/element spacing and alignment
>   - our standard for buttons is 12px of space, but we've never done small 
>     buttons before, so 8px is probably OK there
AHIG says 10px between small buttons, so we need 2 more px here.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

So, what's left here after bug 408536 is really only comment 2.

Comment 6

[Stuart Morgan]

Attachment: fix

Since the background looks fine as-is on Tiger and Panther, I left that pretty much untouched. For Leopard, this has a new nib that uses the same style we'll be using for the bookmark bar, and uses the metalish controls that match it (like what Safari does on Leopard). There a couple oddities with the patch, but they are explained in inline comments.

For both the 10.3/10.4 and the 10.5 versions, I made a few changes:
- Toggling case sensitivity immediately triggers a new search
- The bar is one pixel taller, to visually center the buttons
- The red is darker so it's not as in-your-face

I'm aware that the "Match case" toggle button is a bit unusual, but it's the best option; the checkbox looks terrible, and the toggle style that Xcode uses in its help is invisible until pressed, which makes it really non-obvious when it's next to more standard controls.

Comment 7

[Stuart Morgan]

Attachment: updated 10.3/10.4 nib

Minor edits as explained above, using old-school IB.

Comment 8

[Stuart Morgan]

Attachment: new 10.5+ nib

This one is IB-3 born and raised, since it's not possible to make in the old IB. Since it's only usable (and thus testable) on Leopard, having it only be editable on Leopard seems reasonable.

The "Match case" button is deliberately wacky in the nib, per the code comment and IRC discussion.

Comment 9

[Stuart Morgan]

Attachment: screenshot

Screenshot of the new Leopard style, for those following along at home.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #8)
> Created an attachment (id=303149) [details]
> new 10.5+ nib
> 
> This one is IB-3 born and raised, since it's not possible to make in the old
> IB. Since it's only usable (and thus testable) on Leopard, having it only be
> editable on Leopard seems reasonable.

This is actually going to be highly problematic for localization, I realized as I was reading the patch.

This is going to require all localizers to be on 10.5 (or at least have easy access to it), and l10n doesn't really work on 10.5 (it can be done, but it's a chore).

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 303149 [details]
new 10.5+ nib

Comment 10 notwithstanding, r=ardissone on this nib.

I also noticed that if you're currently matching a case-sensitive term and turn case-sensitivity off, we match the *next* instance, instead of the current one.

This reminds me of our bizarre behavior prior to bug 408533.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 303147 [details]
updated 10.3/10.4 nib

Looks good, works as expected (aside from the same caveat on the code's toggle behavior); r=ardissone

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 303149 [details]
new 10.5+ nib

Actually, I discovered quite by accident that this opens in IB 2.5 on 10.5 (the spacing is presented quite differently 29, 27, 23, etc.), but it opens.

Will it open in IB 2.5 on 10.4, or does the nib really depend on something that's only in 10.5?

Comment 14

[Stuart Morgan]

(In reply to comment #13)
> Will it open in IB 2.5 on 10.4, or does the nib really depend on something
> that's only in 10.5?

There are some 10.5-isms; I'm not sure if 2.5 on 10.4 will preserve them on write, or mangle them. I forgot about the workflow issues, but worst case scenario since it's a single string that needs to be localized (the "not found" text in the nib is not actually used) someone (it could be me) could presumably be given a list of the translations of "Match case" into all the languages, and churn out nibs.

(In reply to comment #11)
> I also noticed that if you're currently matching a case-sensitive term and turn
> case-sensitivity off, we match the *next* instance, instead of the current one.

Yeah, I was aware of that and was going to punt it as requiring too much plumbing to fix, but thinking about it again there may well be an easy solution; I'll check on that.

Comment 15

[Stuart Morgan]

Attachment: fix case sensitivity toggle

This is an additional patch that fixes comment 11 (I didn't want to respin just to glue them together).

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Marcello, see comment 8, comment 10, comment 13, and comment 14.

Comment 17

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 303219 [details] [diff] [review]
fix case sensitivity toggle

sr=pink

Comment 18

[Marcello Testi]

(In reply to comment #16)
> Marcello, see comment 8, comment 10, comment 13, and comment 14.

I did. Not mad about it, but I'll survive.

Comment 19

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 303144 [details] [diff] [review]
fix

sr=pink. sorry, i thought i did this a few days ago.

Comment 20

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH.