177005 - type ahead find: backspace to edit search conflicts with backspace to go back a page

Status: VERIFIED FIXED

(SeaMonkey :: Find In Page, enhancement, P3)

Description

[Matthew Miller]

When using type ahead find to search in a page, the backspace key is used to go
back and correct the current search string. I'm not a very accurate typist, so I
do this often. I also don't necessarily *count* as I type, so it's pretty
typical for me to hit backspace once too often -- leaving me surprised as
suddenly the page changes.

Arguably, backspace is a bad choice for a "go back a page" key (since in normal
usage, it's destructive, and going back doesn't erase anything) but there's
probably so much precedent that it'd be hard to remove that binding.

Perhaps backspace should cause a single beep when pressed right after
backspacing away the last character in a type ahead find pattern, and only after
that revert to going back in history.

Comment 1

[Owen Marshall (Not reading bugmail)]

This has happened *many* times to me, and is quite annoying =)

Comment 2

[Adam Katz]

rather than a beep, i would prefer a preference to disable bksp as back.

i agree with comment 2; this DOES happen a lot.

this looks like an enhancement request...
severity minor -> enhancement

Comment 3

[Matthew Miller]

I just noticed that NS Communicator 4.x *doesn't* use backspace to go back a
page.  Must be a (bad) interface idea imported from IE. I agree that removing
that is really the best solution. (Shouldn't be a preference -- too trivial.)

Comment 4

[Eric Winer]

Actually, I use both Type Ahead Find and, occaisonally, the Backspace key for
going back in history.  When I backspace on a Type Ahead, I hold down the
backspace key.  It would be much better if the back function was disabled when I
was holding down the key from deleting a Type Ahead query.

Comment 5

[Matthew Miller]

I *don't* hold down the backspace key, at least not usually.

I don't see a big benefit in (or any rational reason for) the backspace key
being "go back". Alt-left already does that, and makes more sense -- going back
does not erase the current page....

Comment 6

[Aaron Leventhal]

Attachment: Offer some protection to backspacing to far (into previous page in histroy)

This makes it so that you have to hit backspace one extra time to go back in
history, after using it in typeaheadfind.

It will swallow the next backspace that comes right after you erased the last
typeaheadfind search string character, and beep as a warning. The next
backspace will go back in history, however.

Seeking r=/sr=

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 104740 [details] [diff] [review]
Offer some protection to backspacing to far (into previous page in histroy)

>@@ -506,7 +507,7 @@
>   PRBool isShift(PR_FALSE), isCtrl(PR_FALSE), isAlt(PR_FALSE), isMeta(PR_FALSE);
>   nsCOMPtr<nsIDOMKeyEvent> keyEvent(do_QueryInterface(aEvent));
> 
>-  // ---------- Analyze keystroke, exit early if possible --------------
>+    // ---------- Analyze keystroke, exit early if possible --------------

Why the extra indentation here?  Everything else is 2 space indent...


>@@ -518,6 +519,15 @@
>     return NS_ERROR_FAILURE;
>   }
> 
>+  // ---------- Set Backspace Protection --------------------------
>+  // mIsBackspaceProtectOn should be PR_TRUE, only if the last key 

The comma is grammatically incorrect where you have it.  Just remove it.

>+  // was a backspace and this key is also a backspace. It keeps us 
>+  // from accidentally hitting backspace too many times in a row, going 
>+  // back in history when we really just wanted to clear the find string

Add a period here to end your statement?  (Yes, I'm being anal, sorry...)

>+  if (keyCode != nsIDOMKeyEvent::DOM_VK_BACK_SPACE) {
>+    mIsBackspaceProtectOn = PR_FALSE;
>+  }
>+
>   // ---------- Check the keystroke --------------------------------
>   if ((isAlt && !isShift) || isCtrl || isMeta) {
>     // Ignore most modified keys, but alt+shift may be used for
>@@ -591,6 +601,18 @@
>       mFocusedDocSelection->GetRangeAt(0, getter_AddRefs(mStartFindRange));
>     }
>     else {
>+      if (mIsBackspaceProtectOn) {
>+        // This flag should be on only if the last key was a backspace
>+        // It keeps us from accidentally hitting backspace too many times and
>+        // going back in history when we really just wanted to clear the find string

Two statements need two periods (here especially for clarity)

>+        nsCOMPtr<nsISound> soundInterface =
>+          do_CreateInstance("@mozilla.org/sound;1");
>+        if (soundInterface) {
>+          soundInterface->Beep(); // beep to warn
>+        }
>+        mIsBackspaceProtectOn = PR_FALSE;

So we're only afforded one ill backspace?  Hmm, ok.  I'll buy that I guess. 
Though a cool feature would be to remove the above line and just make us always
beep, and never honor backspace.  :-)

>+        return PR_TRUE;
>+      }
>       return PR_FALSE;  // No find string to backspace in!

The comment on the return should probably be moved up somewhere.  It is true
for the whole of the else, not just for the return.

Pick the nits, and r=caillon

Comment 8

[Aaron Leventhal]

Attachment: Fix comments for caillon.

Let's try this. If people are still hitting backspace all the time, we might
have to make it so some other key is pressed before backspace can go back (once
typeaheadfind starts).

Seeking sr=

Comment 9

[Aaron Leventhal]

Comment on attachment 104778 [details] [diff] [review]
Fix comments for caillon.

Carrying r= forward.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 104778 [details] [diff] [review]
Fix comments for caillon.

sr=bzbarsky

Comment 11

[Aaron Leventhal]

checked in

Comment 12

[sairuh (rarely reading bugmail)]

hm, wonder if this caused the regression bug 180937?

Comment 13

[sairuh (rarely reading bugmail)]

blah. nevermind comment 12 --can no longer repro.

anyhow, this looks fine: tested with 2002.11.19.04 comm trunk bits on win2k.

Comment 14

[Aaron Leventhal]

So, how has this fix panned out? Owen, Matt, Eric ... does the protection work
well enough?

Comment 15

[Eric Winer]

Personally, I would prefer a method of disabling the backspace key as a back button altogether.  Since I appear to be the only one deleting TAF queries by holding down the backspace button, the fix doesn't do much.

Actually, what might work really well is a timer.  Just make the backspace button do nothing for, say, a second after a TAF query has been deleted.

Reopen as enhancement, possibly?

Comment 16

[Matthew Miller]

Aaron -- your fix works decently enough: the problem is no longer crippling. 
(Thanks!) I'd also really prefer the backspace key to never go back a page, but 
that's a much larger issue and shouldn't drag this down. So basically, I'm 
happy.

Comment 17

[Tim Powell]

I'd rather disable backspace as back completely. Not only does it cause me 
problems with TAF (I tend to repeatedly press backspace to clear it), but 
occasionally with forms as well (I sometimes think I have a field focused when 
I don't). I much prefer that normal text editing keys do not cause potential 
data loss.

Comment 18

[Matthew Miller]

Looks like there was a lot of politics, etc. about backspace earlier -- see bug
#69981, resolved wontfix.

Comment 19

[Owen Marshall (Not reading bugmail)]

Heh, the bksp war isn't something we should restart ;)

Aaron: This fixed my problems entirely. Thanks.

Tim: To disable bksp, see bug 108816 comment 46.

Comment 20

[Aaron Leventhal]

*** Bug 187270 has been marked as a duplicate of this bug. ***

Comment 21

[Aaron Leventhal]

My fix was to beep when backspace erases the entire search and then require 1
more backspace to go back in history. Perhaps the fix should have been to
disabled backspace when typeaheadfind starts, until typeahead is cancelled by
some other means than backspace.

Comment 22

[Tim Powell]

Owen, thanks for the bksp tip. I'll try it out.

Aaron: Could TAF require Esc to be pressed or something before 
backspace "works" again and goes back a page?

Comment 23

[Aaron Leventhal]

Tpowell, yeah that's reasonable. Probably better than my original fix.

Comment 24

[Owen Marshall (Not reading bugmail)]

Ok, so would we just gobble up the bksp key, until TAF cancels itself (i.e.
escape, times out, etc.)? That sounds like a very reasonable fix.