Closed Bug 345407 Opened 16 years ago Closed 16 years ago

Visual refresh for searchbar

Categories

(Firefox :: Toolbars and Customization, defect)

2.0 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 2 beta2

People

(Reporter: pamg.bugs, Assigned: pamg.bugs)

References

Details

(Keywords: fixed1.8.1, Whiteboard: [Fx2 theme change])

Attachments

(2 files, 2 obsolete files)

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
Attached patch Implements #1 (obsolete) — Splinter Review
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.
Attachment #230068 - Flags: review?(mconnor)
Blocks: 328065
No longer blocks: 345406
Attached patch Implements #1Splinter Review
Adding back a comment and cleaning up one XBL line.
Attachment #230068 - Attachment is obsolete: true
Attachment #230070 - Flags: review?(mconnor)
Attachment #230068 - Flags: review?(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.
Attachment #230070 - Flags: review?(mconnor) → review+
Flags: blocking-firefox2+
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.
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.
regression or intended ?

if search-engine is on the site , before the glass image changed.
but now, it does not change.
It's a regression, but one we knew would happen after this first patch.  It will be fixed in step #4 of this bug.
-> pamg, since she's actually doing the bulk of the work on this.
Assignee: nobody → pamg.bugs
-> pamg, since she's actually doing the bulk of the work on this.
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.
Attachment #230070 - Flags: approval1.8.1?
Comment on attachment 230070 [details] [diff] [review]
Implements #1

a=drivers. Please go ahead and land this on the MOZILLA_1_8_BRANCH.
Attachment #230070 - Flags: approval1.8.1? → approval1.8.1+
(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.

Step #1 landed on 1.8.1 branch.
--> mglenn, but pam's on the hook for assist :)
Assignee: pamg.bugs → mglenn
Blocks: 346888
Michael, what's left to do here?
Status: NEW → ASSIGNED
Whiteboard: [Fx2 theme change]
Assignee: mglenn → jgoldman
Status: ASSIGNED → NEW
Depends on: 347961
Pam seems to have handled this in bug 347400 (go Pam!)
Keywords: fixed1.8.1
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?
(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).
Hovering anywhere in the searchbar, including either of the endcap buttons, highlights both buttons with the hover effect.
Assignee: jgoldman → pamg.bugs
Status: NEW → ASSIGNED
Attachment #233821 - Flags: review?(mconnor)
Attachment #233821 - Flags: approval1.8.1?
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.
Attachment #233821 - Flags: review?(mconnor)
Attachment #233821 - Flags: review-
Attachment #233821 - Flags: approval1.8.1?
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.)
Attachment #233821 - Attachment is obsolete: true
Attachment #233887 - Flags: review?(mconnor)
Attachment #233887 - Flags: approval1.8.1?
Comment on attachment 233887 [details] [diff] [review]
Scrap idea #3, just add hover

r+a=me, thanks Pam!
Attachment #233887 - Flags: review?(mconnor)
Attachment #233887 - Flags: review+
Attachment #233887 - Flags: approval1.8.1?
Attachment #233887 - Flags: approval1.8.1+
Landed on branch.  Sounds like this one's done, at least for now.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.