595436 - Fix backspace behavior in search

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Gordon P. Hemsley [:GPHemsley]]

If backspace is hit while there is only one character in the search box, or the search box is empty, the search box disappears.

I would say that this is unexpected (and undesired) behavior for the user. I am in the habit of hitting backspace more times that I actually need to in order to ensure a text box is clear. In no other place in my experience does that make said box disappear.

However, in Panorama, it does. And that means I have to again click on the search button to bring up the search box, which can be very frustrating.

(I'm running the nightly from 2010-09-10, build ID: 20100910030657.)

Comment 1

[Raymond Lee [:raymondlee]]

Assigning to Aza for UX design.

Comment 2

[Aza Raskin [:aza]]

This is by design. You can also just start typing, which is why there is a symmetry: type a key and search appears, delete makes it go away.

It may be worth submitting a bug to increase discountability for this.

Alternative is that if you opened search with the icon, delete doesn't hide search.

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

Wouldn't the escape key be just as, if not more, logical? I can see how delete at the beginning removing the search overlay (imagine having a long search string and holding down the delete key for a few seconds) could be unexpected. :/

Comment 4

[Gordon P. Hemsley [:GPHemsley]]

(In reply to comment #2)
> This is by design. You can also just start typing, which is why there is a
> symmetry: type a key and search appears, delete makes it go away.
Well, in this case, I clicked on the icon. I didn't know the search-as-you-type feature existed.

Also, as I said, it goes away even when there is still a character left in the box.

> It may be worth submitting a bug to increase discountability for this.
I don't know what you mean by "discountability".

> Alternative is that if you opened search with the icon, delete doesn't hide
> search.
Well, then that is what this bug is for, so it shouldn't be closed.

I'm reopening this for multiple reasons:
1) I don't think it should be closed in the first place, per above.
2) Even if it should be closed, it should be marked as INVALID or WONTFIX, not WORKSFORME.

Comment 5

[Aza Raskin [:aza]]

Apparently I can't type or choose things from a list. Sorry about that. This is what happens when I'm triaging 250+ bugs :(

Here, then, is the desired behavior:

* If you use the button to get into search, using delete never closes search
* We should fix the bug whereby when there is still a character left it closes search.

Comment 6

[Hrishikesh Kale]

Attachment: backspace behavior of backspace

Comment 7

[Ian Gilman [:iangilman]]

Comment on attachment 478840 [details] [diff] [review]
backspace behavior of backspace

This seems like a good approach, though I think I would store the variable as a property of SearchEventHandlerClass (which you can access from hideSearch() with SearchEventHandler) instead of as an attr. 

Looks like you're using tabs; we require spaces instead, two spaces per indent. 

A unit test will need to be written for this patch (as with all patches that go into Firefox). Perhaps it can be a modification of: 

/browser/base/content/test/tabview/browser_tabview_search.js

Comment 8

[Hrishikesh Kale]

Attachment: new patch as per ian's comment

Comment 9

[Ian Gilman [:iangilman]]

Comment on attachment 478959 [details] [diff] [review]
new patch as per ian's comment

Ok, we're mostly down to stylistic issues. I hope you don't feel they're too nit picky... it's important for us to maintain a uniform style. Every new person who works on the code has to learn the style. 

 function SearchEventHandlerClass() { 
+  this.initiatedBy="";
   this.init(); 
 }
 
Please move this new line into the init routine.

Also, please put spaces around the equal sign, like so: 

    this.initiatedBy = "";
    
Please do this for all = and ==. 
 
-    iQ("#search").hide().click(function(event) {
+    iQ("#searchbox").attr('initiatedBy',"");
+	iQ("#search").hide().click(function(event) {

This change is no longer necessary; please remove.
     
     iQ("#searchbutton").mousedown(function() {
+	  this.initiatedBy="buttonclick";
       ensureSearchShown(null);
       self.switchToInMode();      
     });
     
Since you're inside a handler, that should be "self" rather than "this".

Also, you've got a tab in that line.
     
     this.switchToInMode();
+    SearchEventHandler.initiatedBy="keypress";
     ensureSearchShown(event);
   },

This is still part of the SearchEventHandlerClass, so please use "this" instead of "SearchEventHandler". 	
 
     if (event.which == event.DOM_VK_ESCAPE) 
       hideSearch(event);
-    if (event.which == event.DOM_VK_BACK_SPACE && term.length <= 1) 
+    if (event.which == event.DOM_VK_BACK_SPACE && term.length <= 1 && SearchEventHandler.initiatedBy=="keypress" ) 
       hideSearch(event);

This is also inside SearchEventHandlerClass, so use "this". 

Note that hideSearch is not inside SearchEventHandlerClass, so you're doing the right thing there. 

Thanks again for getting involved!

Comment 10

[Hrishikesh Kale]

Attachment: new patch as per ian's comment#9

Comment 11

[Ian Gilman [:iangilman]]

Comment on attachment 479339 [details] [diff] [review]
new patch as per ian's comment#9

Beautiful! 

Now it needs a test. Let me know if you're interested in writing that, and if you have any questions about doing so. Otherwise one of the team will get to it when they get the chance.

Comment 12

[Hrishikesh Kale]

ok i'll write a test too. please give me some guidelines and tutorials.

Comment 13

[Raymond Lee [:raymondlee]]

(In reply to comment #12)
> ok i'll write a test too. please give me some guidelines and tutorials.

Here are some useful pages:
https://developer.mozilla.org/en/Browser_chrome_tests
https://wiki.mozilla.org/Firefox/Projects/TabCandy/Work#Tests

You can check out our tests which are located in browser/base/content/test/tabview

Comment 14

[Ian Gilman [:iangilman]]

Hrishikesh, how goes the test writing?

Comment 15

[Hrishikesh Kale]

Ian,

i was busy for whole week. will do it today. I;ve installed the mozilla build tools.

Comment 16

[Ian Gilman [:iangilman]]

Hrishikesh, nudge. :)

Comment 17

[Raymond Lee [:raymondlee]]

@ Hrishikesh, do you have time to write a test for your patch?

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch updated to work with trunk, added browser chrome test

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Attachment: v2, corrected diff format

Comment 20

[Ian Gilman [:iangilman]]

Comment on attachment 499748 [details] [diff] [review]
v2, corrected diff format

Tim, thanks for taking this and finishing it up! Looks great; I like your test style. Please get it ready to land, as per:

https://wiki.mozilla.org/Firefox/Projects/TabCandy/Work#Landing

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3 (test now correctly cleans up its environment)

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Pushed to try today. Passed.

Comment 23

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 24

[Kevin Hanes]

bugspam (removing b9)

Comment 25

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

Comment on attachment 501361 [details] [diff] [review]
patch v3 (test now correctly cleans up its environment)

a=beltzner

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Passed try.

Comment 27

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/57424c3ce816

Comment 28

[Tracy Walker [:tracy]]

verified with nightly build of minefield.