Fix backspace behavior in search

VERIFIED FIXED in Firefox 4.0

Status

Firefox Graveyard
Panorama
P2
minor
VERIFIED FIXED
8 years ago
2 years ago

People

(Reporter: GPHemsley, Assigned: ttaubert)

Tracking

Trunk
Firefox 4.0

Details

(Whiteboard: [good first bug][search])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

8 years ago
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.)
Priority: -- → P4
Whiteboard: [good first bug]
Assigning to Aza for UX design.
Assignee: nobody → aza

Comment 2

8 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WORKSFORME
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. :/
(Reporter)

Comment 4

8 years ago
(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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Comment 5

8 years ago
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.
Priority: P4 → P2
Summary: Hitting backspace in search box with zero or one characters makes box disappear → Fix backspace behavior in search
Whiteboard: [good first bug] → [good first bug][search]
Target Milestone: --- → Firefox 4.0

Comment 6

8 years ago
Created attachment 478840 [details] [diff] [review]
backspace behavior of backspace

Updated

8 years ago
Assignee: aza → nobody
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

8 years ago
Created attachment 478959 [details] [diff] [review]
new patch as per ian's comment
Attachment #478959 - Flags: feedback?(ian)
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!
Attachment #478959 - Flags: feedback?(ian) → feedback-
Attachment #478840 - Attachment is obsolete: true

Comment 10

8 years ago
Created attachment 479339 [details] [diff] [review]
new patch as per ian's comment#9
Attachment #479339 - Flags: feedback?(ian)
Attachment #479339 - Attachment is patch: true
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.
Attachment #479339 - Flags: feedback?(ian) → feedback+
Attachment #478959 - Attachment is obsolete: true

Comment 12

8 years ago
ok i'll write a test too. please give me some guidelines and tutorials.
(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
Blocks: 598154
Hrishikesh, how goes the test writing?

Comment 15

8 years ago
Ian,

i was busy for whole week. will do it today. I;ve installed the mozilla build tools.
Hrishikesh, nudge. :)
@ Hrishikesh, do you have time to write a test for your patch?
(Assignee)

Comment 18

8 years ago
Created attachment 499745 [details] [diff] [review]
patch updated to work with trunk, added browser chrome test
Attachment #499745 - Flags: feedback?(ian)
(Assignee)

Comment 19

8 years ago
Created attachment 499748 [details] [diff] [review]
v2, corrected diff format
Attachment #499745 - Attachment is obsolete: true
Attachment #499748 - Flags: feedback?(ian)
Attachment #499745 - Flags: feedback?(ian)
(Assignee)

Updated

8 years ago
Attachment #499748 - Flags: feedback?(ian) → review?(ian)
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
Attachment #499748 - Flags: review?(ian) → review+
Attachment #479339 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #499748 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Attachment #499748 - Flags: approval2.0?
(Assignee)

Comment 21

8 years ago
Created attachment 501361 [details] [diff] [review]
patch v3 (test now correctly cleans up its environment)
Attachment #499748 - Attachment is obsolete: true
Attachment #501361 - Flags: approval2.0?
(Assignee)

Comment 22

8 years ago
Pushed to try today. Passed.
Assignee: nobody → tim.taubert

Comment 23

8 years ago
bugspam (moving b9 to b10)
Blocks: 608028

Comment 24

8 years ago
bugspam (removing b9)
No longer blocks: 598154
Comment on attachment 501361 [details] [diff] [review]
patch v3 (test now correctly cleans up its environment)

a=beltzner
Attachment #501361 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 26

8 years ago
Created attachment 503243 [details] [diff] [review]
patch for checkin

Passed try.
Attachment #501361 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/57424c3ce816
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
verified with nightly build of minefield.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.