Fix Find toolbar appearance/UI

RESOLVED FIXED in Camino1.6

Status

Camino Graveyard
Toolbars & Menus
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: Stuart Morgan)

Tracking

({fixed1.8.1.13})

unspecified
Camino1.6
PowerPC
Mac OS X
fixed1.8.1.13

Details

Attachments

(5 attachments)

fix
11.12 KB, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
6.72 KB, application/octet-stream
Smokey Ardisson (offline for a while; not following bugs - do not email)
: review+
Details
6.98 KB, application/octet-stream
Smokey Ardisson (offline for a while; not following bugs - do not email)
: review+
Details
17.75 KB, image/png
Details
3.93 KB, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
"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?
Flags: camino1.6b1+
(Assignee)

Updated

10 years ago
Duplicate of this bug: 408532
(Assignee)

Comment 2

10 years ago
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.
(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.
(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.
So, what's left here after bug 408536 is really only comment 2.
(Assignee)

Comment 6

10 years ago
Created attachment 303144 [details] [diff] [review]
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.
Assignee: nobody → stuart.morgan
Status: NEW → ASSIGNED
Attachment #303144 - Flags: superreview?(mikepinkerton)
(Assignee)

Comment 7

10 years ago
Created attachment 303147 [details]
updated 10.3/10.4 nib

Minor edits as explained above, using old-school IB.
Attachment #303147 - Flags: review?(alqahira)
(Assignee)

Comment 8

10 years ago
Created attachment 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.

The "Match case" button is deliberately wacky in the nib, per the code comment and IRC discussion.
Attachment #303149 - Flags: review?(alqahira)
(Assignee)

Comment 9

10 years ago
Created attachment 303152 [details]
screenshot

Screenshot of the new Leopard style, for those following along at home.
(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 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.
Attachment #303149 - Flags: review?(alqahira) → review+
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
Attachment #303147 - Flags: review?(alqahira) → review+
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?
(Assignee)

Comment 14

10 years ago
(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.
(Assignee)

Comment 15

10 years ago
Created attachment 303219 [details] [diff] [review]
fix case sensitivity toggle

This is an additional patch that fixes comment 11 (I didn't want to respin just to glue them together).
Attachment #303219 - Flags: superreview?(mikepinkerton)
Marcello, see comment 8, comment 10, comment 13, and comment 14.
Comment on attachment 303219 [details] [diff] [review]
fix case sensitivity toggle

sr=pink
Attachment #303219 - Flags: superreview?(mikepinkerton) → superreview+

Comment 18

10 years ago
(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 on attachment 303144 [details] [diff] [review]
fix

sr=pink. sorry, i thought i did this a few days ago.
Attachment #303144 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 20

10 years ago
Landed on trunk and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.8.1.13
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 418323
No longer depends on: 418323
You need to log in before you can comment on or make changes to this bug.