The goal is to provide a reusable widget that looks and functions in a consistent and predictable way, throughout the UI. <textbox type="search" greytext="&search.label;" clearbutton="true"/> this would add a couple of attrs: greytext: the (non-editable) string that is displayed when value="" This might just be globally useful for textboxes, but the most common case is search fields clearbutton: a button that appears when there is text entered, in order to clear the textarea. Probably not globally useful. Likely consumers: Bookmark and History Sidebar Cookie Manager Password Manager (soon) Download Manager (soon) anywhere else that has a search/filter/find function (find toolbar,, without the greytext/clear button would still be able to use it) searchbar.xml could inherit the greytext etc we could then have a global style for these fields so that we stop reinventing the wheel.

It would be nice if the attribute had a name that didn't require me to remember whether it was spelled "grey" or "gray".

inlineDesc, maybe?

The new Applications prefpane would like this too. (In reply to comment #2) > inlineDesc, maybe? How about just label or description? Either name would accurately describe the function of the attribute without specifying its appearance (which is after all an implementation detail).

Nominating for blocking, though I think that at this stage it's just kinda wanted for code and UI consistency, and probably doesn't block.

nice for code reuse, doesn't block

Going to need to do essentially one of these for the AMO integration, may as well try to make it reusable.

This really makes more sense than fixing the depending blockers separately. I'm eager to re-request blocking... Mossop: I have free cycles and guess I could take this bug if that would help you.

(In reply to comment #7) > This really makes more sense than fixing the depending blockers separately. I'm > eager to re-request blocking... > > Mossop: I have free cycles and guess I could take this bug if that would help > you. I have the widget near complete but I ended up stopping waiting to see what happened with bug 406095 since that implements part of the widget anyway. I guess I could just plow on making changes assuming that is going to land.

Passed over to Dão

Dão, is this going to happen for Firefox 3, or should we be trying to fix the list of dependent bugs independent of this?

Yes, this is going to happen as far as I can tell. I'm targeting M11 currently.

(In reply to comment #11) > Yes, this is going to happen as far as I can tell. I'm targeting M11 currently. A landing towards the end of the M11 cycle would really be too late from my point of view. This is one of the bugs that is blocking me from getting bug 404024 ready for review.

You could use a normal text box, right? We just need to add emptyText="&foo;" and type="search" once the bugs are ready. We can think about waiting with landing, but the review process could start right now.

(In reply to comment #13) > You could use a normal text box, right? We just need to add emptyText="&foo;" > and type="search" once the bugs are ready. All you have to do is listen to the 'input' event, I think. The widget type shouldn't matter for your search handler.

(In reply to comment #13) > You could use a normal text box, right? We just need to add emptyText="&foo;" > and type="search" once the bugs are ready. We can think about waiting with > landing, but the review process could start right now. Normal textbox with two buttons to start search and clear current search yes, that is what I am currently using to test. But I don't want to land with that, so I don't really want to start the review process without what I want to land. (In reply to comment #14) > All you have to do is listen to the 'input' event, I think. The widget type > shouldn't matter for your search handler. input is fired whenever the text changes, entirely unsuitable for my needs.

I'm not sure if this widget will be suitable for you then. If you look at the current potential consumers (comment 0 or depending bugs), you'll notice that they all search on input and without a submit button. There's bug 403154, but that doesn't sound like what you want either.

>I'm not sure if this widget will be suitable for you then. Madhava: does the search field in the new addons manager (http://wiki.mozilla.org/Firefox:Add-ons_Manager_UI_interim_rev2) respond to input? If we are using the same styling, it seems like we should remain consistent. Since there is a network connection, we will want a small amount of lag before going to get results, similar to search suggestions in the web search bar.

status update? This is holding up some blocking+ bugs.

waiting for bug 406095.

(In reply to comment #19) > waiting for bug 406095. so, once that is done, how soon might this be resolved?

I don't have working code yet, but it shouldn't take too long (which doesn't say anything about the review time, though). Fortunately, the serious blocking bugs (i.e. where things are broken) really depend on bug 406095 rather than this one.

Please use this mechanism in the following places. I added their current state, so you can see how different the search UI is along Firefox. * Bookmarks sidebar search (search label, no descriptive text, no clear button) * History sidebar search (search label, no descriptive text, no clear button) * Find search (search label, no descriptive text, no clear button) * Quick find search (search label, no descriptive text, no clear button) * Applications search (search label, no descriptive text, clear button) * Cookies search (search label, no descriptive text, clear button) * Extension search (search label, no clear button, search button) * Help search (search label, no descriptive text, no clear button) * Library search (no clear button) * Remembered passwords search (search label, no descriptive text, clear button)

Forgot to add: * Downloads search (no clear button) And extensions search should have been Add-ons search :P

Where do find widgets generally have a clear button? Generally the escape key clears the results (which I know at least works for the download manager).

They're generally found... on OS X widgets ;) Safari search, finder search. At least a clear button that's embedded inside the textbox itself. The cookies search's clear button is a separate clear button outside the textbox.

Also Thunderbird.

(In reply to comment #26) > Where do find widgets generally have a clear button? Generally the escape key > clears the results (which I know at least works for the download manager). > IMHO, every search instance should follow this style for consistency. In Windows XP, no native application uses something like that.

Here is the search widget on Vista, which follows the design of search widgets on OS X.

Forgot to add: * about:config search (filter label, no descriptive text, show all button) Note that giving the way search is offered now, thanks to awesomebar, filter means the same as search. And show all mean the same as clear, since clearing the filter (search entry) shows all configuration options.

For pinstripe styling, you can probably base it on the one used for the download manager. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/themes/pinstripe/mozapps/downloads/downloads.css&rev=1.25&mark=85-97#85

yeah, there's a similar styling for #search-box in textbox.css. Since the id selector is completely wrong there, I guess I can just morph that.

Comment on attachment 308490 [details] [diff] [review] patch >Index: toolkit/content/widgets/textbox.xml >+ <implementation> >+ <field name="searchIcons"> >+ document.getAnonymousElementByAttribute(this, "anonid", "search-icons"); >+ </field> >+ <method name="clearSearch"> >+ <body> >+ <![CDATA[ >+ if (this._timer) >+ clearTimeout(this._timer); >+ this.value = ""; >+ this.searchIcons.selectedIndex = 0; >+ this._fireCommand(this); Why does this fire a command event here? Clearing the search seems to me like it should just clear the field. The value setter happens to clear the timeout already, or perhaps there is a reason to clear it beforehand? If a consumer of the search textbox uses this.value = '', the icon and timer will not be updated properly. Maybe it would be better to override the value setter here. The searchIcons and clearSearch methods should be preceded by underscores anyway. >+ </handler> >+ <handler event="keypress" keycode="VK_ESCAPE"> >+ <![CDATA[ >+ this.clearSearch(); >+ event.preventDefault(); Did someone decide that handling escape should always happen? The browser search field doesn't seem to handle escape currently.

(In reply to comment #39) > Why does this fire a command event here? Clearing the search seems to me like > it should just clear the field. It should also clear the results. We're already doing this in Thunderbird, the Places Library and elsewhere. > The value setter happens to clear the timeout already, or perhaps there is a > reason to clear it beforehand? > If a consumer of the search textbox uses this.value = '', the icon and timer > will not be updated properly. Maybe it would be better to override the value > setter here. Good points. I'll just remove clearSearch and handle "" in the setter. > The searchIcons and clearSearch methods should be preceded by underscores > anyway. > > >+ </handler> > >+ <handler event="keypress" keycode="VK_ESCAPE"> > >+ <![CDATA[ > >+ this.clearSearch(); > >+ event.preventDefault(); > > Did someone decide that handling escape should always happen? The browser > search field doesn't seem to handle escape currently. The browser search field isn't a good example, since it doesn't have a live results list that you would want to clear. (It does have search suggestions though, and we close them on escape.)

Should the textbox be consuming the escape entirely? Download manager: esc in textbox -> clear and moves focus to the list; esc in list -> close the window Should there be some way for the container to handle the escape as well? Findbar: esc -> close the findbar (doesn't clear)

Propagation of the event is still happening, so there is a way for the container to handle it.

(In reply to comment #40) > (In reply to comment #39) > > Why does this fire a command event here? Clearing the search seems to me like > > it should just clear the field. > > It should also clear the results. We're already doing this in Thunderbird, the > Places Library and elsewhere. Sure, but the searchbox should only fire a command event if the user requested a search.

If the user types in something, the timed-textbox is going to fire a command on a timeout anyway. Are you talking about if the user types in something and hits escape before the timer is triggered?

(In reply to comment #44) > Are you talking about if the user types in something and hits escape before the > timer is triggered? My assumption as a user is that pressing escape or the clear button would cancel any searching. The command event being fired would imply that I has requested a search to start.

Should hitting escape just clear the search or also reset the search? E.g., in the download manager, hitting escape from the search box will clear the search and show the default list.. which is triggered by searching for ''.

Clicking the clear button is a request just like selecting all and deleting it is. It empties the search string, which means that the unfiltered list should show up.

(In reply to comment #47) > Clicking the clear button is a request just like selecting all and deleting it > is. It empties the search string, which means that the unfiltered list should > show up. > What is this 'list' you're referring to? The dropdown that appears in some of Firefox UI search field? I think I understand my confusion here, and I think Dave alludes to this above. The firefox search box has two different searches. The 'command' event should fire when the user requests to perform the actual search. On the Firefox search box, this occurs when the hourglass icon or Enter is pressed. The autocomplete part of this search box does a different search on input events. However, for some search fields, like the Thunderbird one, the search occurs on every input event. In the former case, clearing the field shouldn't fire any command events, but in the latter case, it seems reasonable that it should. It seems that this patch only implements the latter type of search mechanism.

(In reply to comment #48) > What is this 'list' you're referring to? The dropdown that appears in some of > Firefox UI search field? The downloads in the download manager or the saved passwords in the password manager, for instance. > It seems that this patch only implements the latter type of search mechanism. Exactly. I think that's what this bug is about. "search" is quite ambiguous; we might need a better name.

Comment on attachment 308689 [details] [diff] [review] patch v2 + <binding id="search-textbox" extends="chrome://global/content/bindings/textbox.xml#timed-textbox"> + <content> + <children/> + <xul:hbox class="textbox-input-box" flex="1" xbl:inherits="context,spellcheck" align="center"> + <html:input class="textbox-input" flex="1" anonid="input" + xbl:inherits="onfocus,onblur,value,type,maxlength,disabled,size,readonly,tabindex,accesskey"/> + <xul:deck class="textbox-search-icons" anonid="search-icons"> + <xul:image class="textbox-search-icon" onclick="document.getBindingParent(this).focus()"/> Maybe we could inherit 'image' here, but that can be another bug. + <xul:image class="textbox-search-clear" onclick="document.getBindingParent(this).value = ''"/> The clear icon shouldn't do anything if the textbox is disabled or readonly. The focus() call for the search icon should already handle this. + <property name="value" + onget="return this.hasAttribute('empty') ? '' : this.inputField.value;"> + <setter><![CDATA[ ... + if (this._timer) + clearTimeout(this._timer); + this._fireCommand(this); The command event should only fire when the user changes the value. I still think that the search box should use the 'search/go button starts search' rather than on input event by default, and have an attribute which switches to input event behaviour. Has there been a UI discussion about this; the original bug report doesn't mention how this should behave.

(In reply to comment #51) > I still think that the search box should use the 'search/go button starts > search' rather than on input event by default, and have an attribute which > switches to input event behaviour. Has there been a UI discussion about this; > the original bug report doesn't mention how this should behave. That would allow us to use this standard widget in far more circumstances, the addons manager for instance.

(In reply to comment #52) > That would allow us to use this standard widget in far more circumstances, the > addons manager for instance. I agree. I think the common case here is on input (download manager, add-ons manager, thunderbird to name a few).

(In reply to comment #53) > (In reply to comment #52) > > That would allow us to use this standard widget in far more circumstances, the > > addons manager for instance. > I agree. I think the common case here is on input (download manager, add-ons > manager, thunderbird to name a few). I disagree with your agreement. add-ons manager does not use search on input.

I stand corrected. I swear I saw it doing it on input at one point, but maybe I was imagining things :/

Firefox alone has like a dozen textboxes that search/filter on input. The add-ons manager is the only place where a search-on-submit widget could be used. The browser search bar needs special treatment anyway, and a clear button doesn't make sense there.

> Firefox alone has like a dozen textboxes that search/filter on input. 10 to be precise, all listed in comment 24, comment 25, and comment 31. Plus Thunderbird.

(In reply to comment #51) > + <xul:image class="textbox-search-clear" > onclick="document.getBindingParent(this).value = ''"/> > > The clear icon shouldn't do anything if the textbox is disabled or readonly. > The focus() call for the search icon should already handle this. What should the focus() call handle? disabled/readonly? > + <property name="value" > + onget="return this.hasAttribute('empty') ? '' : > this.inputField.value;"> > + <setter><![CDATA[ > ... > + if (this._timer) > + clearTimeout(this._timer); > + this._fireCommand(this); > > The command event should only fire when the user changes the value. To quote you: > If a consumer of the search textbox uses this.value = '', the icon and timer > will not be updated properly. Similarly, as we update the icon, the consumer needs to update the results list. It could do that explicitly by calling doCommand, but I don't see a use case for changing the value and not updating the results, so we can do that right away.

Support for the image attribute added, clearing handles disabled and readonly. I don't see a problem with calling focus() regardless of these states.

> It could do that explicitly by calling doCommand, but I don't see a use case > for changing the value and not updating the results, so we can do that right > away. No, the command event should only be fired as a result of a user action. We have numerous bugs already where this isn't the case. If a script wants a search to start, it should do that itself.

As I said, I don't see a use case. If you set the value without starting the search, the widget is left in a state that's not directly manageable for the user. (He has to add or remove a character to kick off the search.) Seems like a suboptimal API. So could I have a counter use case, in order to understand your concern, please? A pointer to one of the numerous bugs will do, too, assuming it's applicable to what I'm doing here and what the widget is supposed to do.

The difference is that the code using the searchbox widget can choose whether or not the command is triggered. For download manager, the command needs to be triggered on input/clear whereas add-ons does not want it triggered. But then again, download manager uses the timed-box whereas add-ons uses a textbox. Because this implementation is extending the timed-box, I think it /should/ doCommand on escape because the timed box will doCommand on a timeout anyway. Does there need to be a non-timed searchbox widget that has the appropriate styling, empty text, etc?

Comment on attachment 309265 [details] [diff] [review] patch v3 >+ <binding id="search-textbox" extends="chrome://global/content/bindings/textbox.xml#timed-textbox"> timed-searchbox extends timed-textbox searchbox extends textbox ?

(In reply to comment #62) > The difference is that the code using the searchbox widget can choose whether > or not the command is triggered. For download manager, the command needs to be > triggered on input/clear whereas add-ons does not want it triggered. The input event isn't coupled to the value setter, so we'd still search on input. > Does there need to be a non-timed searchbox widget that has the appropriate > styling, empty text, etc? As long as the add-ons manager would be the only consumer, it doesn't seem like a dedicated toolkit binding is worthwhile. In any case, that could be a separate bug. Note that emptytext support is already in place.

Here are various search boxes from comment 24, 25, 31. empty: has gray empty text? clear: has a clear button either inside box or separate box icon: has a search icon esc: hitting esc clears search without quitting window? Performs action on enter: * Add-ons - enter, YES empty, no clear, YES icon, esc quits Performs action right away: * Find - immediate, no empty, no clear, no icon, esc quits * Quick find - immediate, no empty, no clear, no icon, esc quits Performs action on timeout: * about:config - timed, no empty, YES clear, no icon, no esc * Applications - timed, no empty, YES clear, no icon, esc clears + quits * Bookmarks sidebar - timed, no empty, no clear, no icon, no esc * Cookies - timed, no empty, YES clear, no icon, YES esc * Downloads - timed, YES empty, no clear, YES icon (os x), YES esc * Help - timed, no empty, no clear, no icon, esc resets not clear * History sidebar - timed, no empty, no clear, no icon, no esc * Library - timed, YES empty, YES clear, no icon, YES esc * Remembered passwords - timed, no empty, YES clear, no icon, YES esc * Search bar - timed, YES empty, no clear, YES icon, no esc

- added support for search on submit - not extending the timed binding anymore. I think we should get rid of that after 1.9.1 since all our timed textboxes are search fields.

Question: If this were to be used in a dialog box or window that can be closed with Escape, this patch currently does not allow the window to be closed via that keystroke, correct? I'd propose this change: 1. If there's text in the textbox and the user presses Escape, clear the text as is currently. However, if there is no text and Escape is allowed to do anything else from another place, also perform that default action. This would allow keyboard users to still escape dialog box-like windows like the Add-Ons Manager if this widget is being used there, by hitting Escape twice.

- what Marco said - fixed some issues with winstripe and pinstripe

err... what Marco really said.

Could we get some tests with this please? It wouldn't be too hard to write a chrome test for it.

made more consistent with how the search-on-submit field in the add-ons manager works

- in submit button mode, don't display the clear icon when submitting an empty string - added back the left-aligned search icon in pinstripe

(I switched the review request to mano as I expect Enn to have less time due to getting married.)

Comment on attachment 331278 [details] [diff] [review] patch v7 I will still review it

Neil, can you do that soon? I'd like to get this landed as soon as possible?

Comment on attachment 331278 [details] [diff] [review] patch v7 >+ function iconClick(aIcon) { >+ var event = document.createEvent("MouseEvent"); >+ event.initMouseEvent("click", true, true, window, 1, >+ 0, 0, 0, 0, >+ false, false, false, false, >+ 0, null); >+ aIcon.dispatchEvent(event); >+ } >+ Why not just use synthesizeMouse here instead? >+ icons.selectedPanel = clearIcon; >+ textbox.value = "foo"; >+ is(icons.selectedPanel, searchIcon, "search icon displayed in submit button mode when the value is changed"); This first line here changes some internal state. You should write the test to not do this. >+ <property name="submitButton" >+ onget="return this.hasAttribute('submitbutton');" >+ onset="if (val) this.setAttribute('submitbutton', 'true'); >+ else this.removeAttribute('submitbutton'); return val;"/> I don't like the name 'submitbutton' here, which implies that the attribute controls whether a button is shown or not. However, the button can be shown in both modes. >+ <method name="_enterSearch"> >+ <body><![CDATA[ >+ if (this.submitButton && this.value) >+ this._searchIcons.selectedIndex = 1; >+ this._fireCommand(this); This can change the icon state even clicking the search icon when the textbox is disabled, no? Other things to check what happens: <textbox type="search" value="some text"/> - searchTextBox.reset() method - maybe add an emptytext to the test to ensure that this is handled properly

(In reply to comment #78) > Why not just use synthesizeMouse here instead? As far as I remember, synthesizeMouse doesn't work for anonymous elements. > I don't like the name 'submitbutton' here, which implies that the attribute > controls whether a button is shown or not. However, the button can be shown in > both modes. I can't think of a better name. The image acts as a button only in that one mode. In the other case it's merely a visual indicator; you can click it, but this doesn't do anything other than focusing the textbox. > >+ <method name="_enterSearch"> > >+ <body><![CDATA[ > >+ if (this.submitButton && this.value) > >+ this._searchIcons.selectedIndex = 1; > >+ this._fireCommand(this); > > This can change the icon state even clicking the search icon when the textbox > is disabled, no? Yeah, I guess the right thing is to allow submitting without changing the icon.

addressed everything except for the 'submitbutton' attribute name.

(In reply to comment #79) > > This can change the icon state even clicking the search icon when the textbox > > is disabled, no? > > Yeah, I guess the right thing is to allow submitting without changing the icon. Actually, it makes more sense to do this in the readOnly case, and disable submitting when the textbox is disabled. I'll update the patch to that effect later today.

see previous comment

with images

(In reply to comment #79) > (In reply to comment #78) > > Why not just use synthesizeMouse here instead? > > As far as I remember, synthesizeMouse doesn't work for anonymous elements. Do you have a testcase here or a bug which demonstrates this? I don't see why that would be the case.

Ok, I was looking at sendMouseEvent, which takes the id of the target element rather than the target element itself. Will try synthesizeMouse now.

I donu't think I can use synthesizeMouse either, as pinstripe doesn't assign a list style image in the :not([submitbutton]) case, which makes the image 0*0px big.

(In reply to comment #86) > I donu't think I can use synthesizeMouse either, as pinstripe doesn't assign a > list style image in the :not([submitbutton]) case, which makes the image 0*0px > big. Ok, this works anyway, because even if the image itself can't be clicked in that case, a click on its /position/ reaches the textbox and makes it focused as well.

synthesizeMouse only requires the target element to know what to calculate the coordinates relative to. It doesn't fire mouse events at elements, it fires mouse events at the window using the x,y coordinate, so it doesn't matter as long as you're using coordinates in the right place.

Comment on attachment 332361 [details] [diff] [review] patch v9, using synthesizeMouse in tests OK, but I think 'searchbutton' is better than 'submitbutton' as you're not submitting anything when it is pressed (you might be, but you might not be)

One Windows box doesn't like the "clicking the search icon focuses the textbox" test. I'm waiting for some more data.

The next Windows box shows the same failures. Linux looks fine. The tests also work for me locally on Windows.

http://hg.mozilla.org/index.cgi/mozilla-central/rev/03de2d980072 Said test also failed on OS X. Instead of using synthesizeMouse, I'm now dispatching the event manually again, which just made qm-win2k3-moz2-01 go green: http://hg.mozilla.org/index.cgi/mozilla-central/rev/04a6b7cf2929 Suggestions as to why synthesizeMouse failed on the Windows and OS X boxes would be appreciated.

Verified based on search widget inside the Add-ons manager with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/20080808031528 Minefield/3.1a2pre ID:20080808031528 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080808120213 Minefield/3.1a2pre ID:20080808120213

Will this feature parody Safari's implementation? http://weblogs.mozillazine.org/hyatt/archives/2004_07.html#005890

(In reply to comment #95) > Will this feature parody Safari's implementation? > http://weblogs.mozillazine.org/hyatt/archives/2004_07.html#005890 It has some similar features, but with a different API. Most importantly, it's a XUL widget.