336457 - Mark selected search engine in the drop-down list

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Pam Greene]

Indicate which search engine is currently selected in the drop-down list on the searchbar.

Comment 1

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

brainstormed ideas on how to do this, comments below:

 - overlay the search icon with a checkmark
 - replace the search icon with a checkmark (since it will be in the search box)
 - place a checkmark at the end of the search engine name string
 - always have the active engine at the top of the list
 - show the active engine as greyed out

Of these, I think one of the checkmark solutions is easiest and simplest.

Comment 2

[Pam Greene]

If possible, I'd vote for a checkmark to the left of the menu item, matching what happens in a standard system menu.

Comment 3

[Johannes Buchner]

(In reply to comment #1)
>  - replace the search icon with a checkmark (since it will be in the search
> box)
maybe not optimal ... uses like to go by icons.
>  - always have the active engine at the top of the list
User will be confused if the search engines jump around.
>  - show the active engine as greyed out
I would like this one, if it doesn't look disabled (not available, uninstalled).
Maybe if it was marked selected before user move the mouse over the list?

Maybe it looks just perfect if you bold the text? <-- I would like to see how that would look

Comment 4

[Pam Greene]

Attachment: Screenshot of selected engine in bold

True -- bold text looks nice, clean and obvious, where a checkmark plus an icon might be a bit cluttered.

Comment 5

[Pam Greene]

Attachment: Boldfaces selected engine name

The changes in this patch only do what they're described to do after the changes in the patch for bug 335441 are applied.

Until that patch is in, this one will highlight the engine that is selected when you first launch the browser.  I'd rather have done everything in series, but reviewers are busy and deadlines loom.  (Also this is a very small patch.)

Comment 6

[:Gavin Sharp [email: [e-mail]]]

IDs should be unique to an element. Can't you use the class to style the element instead?

Comment 7

[Pam Greene]

Attachment: Patch addressing Gavin's comments

Using a class rather than an ID.  Also, because of some concurrent changes to the patch for bug 335441, this patch is now independent of that one and works just as it is.

Comment 8

[Joe Hughes]

Comment on attachment 220889 [details] [diff] [review]
Patch addressing Gavin's comments

+            menuitem.setAttribute("class", "menuitem-iconic searchbar-engine-menuitem");
+            if (this._engines[i] == this.currentEngine)
+              menuitem.setAttribute("selected", "true");

If the style is keyed to the "selected" attribute, why do you need to set the class?  (Also, how does the "selected" attribute get cleared?)

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> Also, how does the "selected" attribute get cleared?

I guess that was initially done by the rebuilding of the menu in the previous patches.

Comment 10

[Pam Greene]

(In reply to comment #8)
> (From update of attachment 220889 [details] [diff] [review] [edit])
> +            menuitem.setAttribute("class", "menuitem-iconic
> searchbar-engine-menuitem");
> +            if (this._engines[i] == this.currentEngine)
> +              menuitem.setAttribute("selected", "true");
> 
> If the style is keyed to the "selected" attribute, why do you need to set the
> class?  (Also, how does the "selected" attribute get cleared?)

The class has to be set to menuitem-iconic in any case, so adding searchbar-engine-menuitem has very little cost, and would allow a theme to distinguish between these add-engine items and other items in the list.

The menu is now rebuilt when a new engine is selected, as well as when one is added or removed, which keeps the "selected" attribute in sync.  If we want, that could be done with a different function that only updates the "selected" mark on an "engine-current" notification.

Comment 11

[Pam Greene]

Attachment: Added comments explaining clearing of "selected" attribute

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 221002 [details] [diff] [review]
Added comments explaining clearing of "selected" attribute

sr=[e-mail]

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

fixed-1.8-branch, fixed-on-trunk

Comment 14

[Johannes Buchner]

cool feature.