345407 - Visual refresh for searchbar

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Pam Greene]

This bug comprises several pieces, which will require some small number of separate patches.  Roughly speaking:

1. Move search-engine selection drop-down back to the engine icon
2. Update search-engine button to contain space outside the icon, to hold glows
3. Implement hover state for buttons based on textbox hover
4. Implement glowing engine button in the presence of an autodetected engine
5. Change search button style

Comment 1

[Pam Greene]

Attachment: Implements #1

Moves the search-engine selection drop-down back to the left.  

The "addengines" property is set to "true" on the outer searchbar-engine-button element when an engine is autodetected, but actually making that propagate down and show new art is left to step #4.

Neither the pinstripe nor the winstripe CSS has been tweaked for perfect alignment of every image and button, since they'll all be changing in later steps anyway, but they're usable.

Comment 2

[Pam Greene]

Attachment: Implements #1

Adding back a comment and cleaning up one XBL line.

Comment 3

[Mike Connor [:mconnor]]

Comment on attachment 230070 [details] [diff] [review]
Implements #1

Ok, that should do.

I don't think the const is worth the trouble, its not like we're going to be changing this again without more surgery on the XBL, so there's no real win there.

Comment 4

[Pam Greene]

Landing on trunk to bake.

(In reply to comment #3)
> I don't think the const is worth the trouble

Okay, I've taken it out.  For future reference, what's the downside?  My usual inclination is to avoid hard-coding strings, especially if they're used in more than once place that could get out of sync; but of course that's moderated by the performance or other impact it has.

Comment 5

[Mike Connor [:mconnor]]

Doing a quick test, hardcoding the value is much faster than evaluating the const's value.

var time = Date.now();for (var i = 0; i < 50000; i++) {"baz" + "bar"};Date.now() - time; takes 63 milliseconds
var time = Date.now();const foo = "bar";for (var i = 0; i < 50000; i++) {"baz" + foo};Date.now() - time; takes 110 milliseconds

In cases where a value is likely to change frequently, or is used in so many places that keeping it in sync becomes problematic, it might be worth the hit, but for two cases it doesn't seem to be worth it.

Comment 6

[pal-moz]

regression or intended ?

if search-engine is on the site , before the glass image changed.
but now, it does not change.

Comment 7

[Pam Greene]

It's a regression, but one we knew would happen after this first patch.  It will be fixed in step #4 of this bug.

Comment 8

[Mike Connor [:mconnor]]

-> pamg, since she's actually doing the bulk of the work on this.

Comment 9

[Mike Connor [:mconnor]]

-> pamg, since she's actually doing the bulk of the work on this.

Comment 10

[Pam Greene]

Actually, my understanding from email with Jay and Mike Glenn of Radiant Core is that Mike and his colleagues will be working on steps #2-5.  Of course I'll help out with that however I can.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 230070 [details] [diff] [review]
Implements #1

a=drivers. Please go ahead and land this on the MOZILLA_1_8_BRANCH.

Comment 12

[Michael Glenn]

(In reply to comment #10)
> Actually, my understanding from email with Jay and Mike Glenn of Radiant Core
> is that Mike and his colleagues will be working on steps #2-5.  Of course I'll
> help out with that however I can.

We'll be tackling this but will differ to Pam for her expertise if we get stuck.

Comment 13

[Pam Greene]

Step #1 landed on 1.8.1 branch.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

--> mglenn, but pam's on the hook for assist :)

Comment 15

[Mike Connor [:mconnor]]

Michael, what's left to do here?

Comment 16

[Mike Connor [:mconnor]]

Pam seems to have handled this in bug 347400 (go Pam!)

Comment 17

[Pam Greene]

We don't seem to have gotten this piece yet:

> 3. Implement hover state for buttons based on textbox hover

Do we still want it?

Comment 18

[Jay Goldman]

(In reply to comment #17)

> Do we still want it?

I'd like to see it in - the intent was to create an obvious affordance for users so that hovering over the box cues them to know that they can open the Search Provider menu by making it look more clickable (assuming I correctly understand what you're referring to).

Comment 19

[Pam Greene]

Attachment: Implements #3: hovering anywhere highlights buttons

Hovering anywhere in the searchbar, including either of the endcap buttons, highlights both buttons with the hover effect.

Comment 20

[Mike Connor [:mconnor]]

Comment on attachment 233821 [details] [diff] [review]
Implements #3: hovering anywhere highlights buttons

So, this is now the wrong thing, since we changed paths in 347400 to make the provider selection a visible button.  We just want normal button hover effects on both buttons for now.

Comment 21

[Pam Greene]

Attachment: Scrap idea #3, just add hover

After discussions with mconnor on IRC, this seems to be what's needed now: no fancy hover effect from the textbox, just adding the missing search-engine button hover to winstripe.  (Note that it's a pretty subtle effect.)

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 233887 [details] [diff] [review]
Scrap idea #3, just add hover

r+a=me, thanks Pam!

Comment 23

[Pam Greene]

Landed on branch.  Sounds like this one's done, at least for now.