414443 - New addons UI cancel icon doesn't always change back to magnifying glass icon

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012811 Minefield/3.0b3pre ID:2008012811

1. New profile, start Firefox
2. Tools > Add-ons > Get Add-ons
3. Search for something. Note in the search box, the magnifying glass changes to a round (x)
4a. Whilst the search is being carried out (Throbber + "Searching add-ons") click the 'Cancel' button (or)
4b. After you get results back, click the 'Clear Results' button. (or)
4c. After you get no results back ("No Matching Add-ons") click the 'Ok' button. (or)
4d. You get an error ("Minefield couldn't retreive add-ons") click the 'Cancel' button.

Expected
- Search box query is removed so box is blank, (x) icon changes back to magnifying glass

Actual:
- Search box query is removed so box is blank, (x) icon remains.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch rev 1

When setting the value we must persist it's value to the attribute and update the search buttons.

Also covers bug 414441

Comment 4

[Robert Strong (they/them - no direct email)]

Comment on attachment 301261 [details] [diff] [review]
patch rev 1

>diff -r 4363e9539cb5 toolkit/mozapps/extensions/content/extensions.xml
>--- a/toolkit/mozapps/extensions/content/extensions.xml	Mon Feb 04 01:31:05 2008 -0800
>+++ b/toolkit/mozapps/extensions/content/extensions.xml	Mon Feb 04 13:05:02 2008 +0000
>@@ -880,18 +880,30 @@
>       <field name="_searchButton">
>         document.getAnonymousElementByAttribute(this, "class", "searchbox-search");
>       </field>
> 
>       <field name="_cancelButton">
>         document.getAnonymousElementByAttribute(this, "class", "searchbox-cancel");
>       </field>
> 
>-      <property name="value" onget="return this.textbox.value"
>-                             onset="this.textbox.value = val"/>
>+      <property name="value" onget="return this.textbox.value">
>+        <setter>
>+          this.textbox.value = val;
>+          this.setAttribute("value", this.value);
curious... why not just use val instead of this.value here? Why isn't setting the textbox's value enough?

>+          if (this.value) {
>+            this._cancelButton.hidden = false;
>+            this._searchButton.hidden = true;
>+          }
>+          else {
>+            this._cancelButton.hidden = true;
>+            this._searchButton.hidden = false;
>+          }
could you simplify this?

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 301261 [details] [diff] [review]
patch rev 1

gonna minus until the questions are answered

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Yeah not quite sure what I was thinking when I wrote that patch. This is the same just much simpler.

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 309558 [details] [diff] [review]
patch rev 2

>diff --git a/toolkit/mozapps/extensions/content/extensions.xml b/toolkit/mozapps/extensions/content/extensions.xml
>--- a/toolkit/mozapps/extensions/content/extensions.xml
>+++ b/toolkit/mozapps/extensions/content/extensions.xml
>@@ -885,18 +885,24 @@
>       <field name="_searchButton">
>         document.getAnonymousElementByAttribute(this, "class", "searchbox-search");
>       </field>
> 
>       <field name="_cancelButton">
>         document.getAnonymousElementByAttribute(this, "class", "searchbox-cancel");
>       </field>
> 
>-      <property name="value" onget="return this.textbox.value"
>-                             onset="this.textbox.value = val"/>
>+      <property name="value" onget="return this.textbox.value">
>+        <setter>
>+          this.textbox.value = val;
>+          this.setAttribute("value", val);
>+          this._cancelButton.hidden = !val;
>+          this._searchButton.hidden = val;
I'm not positive what the standard is but I wasn't able to find any other places in toolkit/content/widgets where a string value was used in quite this way to set a boolean... usually val would be compared to evaluate to true or false instead. Could you run this by someone like Gavin?

Comment 8

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

(blocking as this will fix the blocker bug 414441)

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 3

This is what Gavin suggested

Comment 10

[Dave Townsend [:mossop]]

Checking in toolkit/mozapps/extensions/content/extensions.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xml,v  <--  extensions.xml
new revision: 1.63; previous revision: 1.62
done

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Same happend for OS X builds => Verified.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008040204 Minefield/3.0pre ID:2008040204

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040204 Minefield/3.0pre ID:2008040204

Comment 12

[Tony Chung [:tchung]]

Altered litmus testcase: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=6796 to reflect this behavior.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Tony, meanwhile this is covered by the automated test for the search widget. So no need to have this in Litmus. Can you update the Litmus test and remove this part? Thanks.

See:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/widgets/test_textbox_search.xul#75

Comment 14

[Marcia Knous [:marcia]]

For me clicking the magnifying glass in addons does nothing, so I guess this test case needs to be updated.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #14)
> For me clicking the magnifying glass in addons does nothing, so I guess this
> test case needs to be updated.

When you are using OS X clicking on the magnifying glass doesn't start the search. Correct. That's given by the HIG.

Comment 16

[Aakash Desai [:aakashd]]

Litmus testcases have this incorporated thanks to Tony:

https://litmus.mozilla.org/show_test.cgi?id=8825
https://litmus.mozilla.org/show_test.cgi?id=6796


I see no problem with having redundancy in our testing when the difference is between a manual test framework and an automated one. The testcases are going to stay as is. I'm plussing in-testsuite as well.