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

VERIFIED FIXED in mozilla1.3alpha

Status

SeaMonkey
Find In Page
P3
enhancement
VERIFIED FIXED
15 years ago
9 years ago

People

(Reporter: Matthew Miller, Assigned: Aaron Leventhal)

Tracking

Trunk
mozilla1.3alpha
x86
All

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

15 years ago
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.
(Reporter)

Updated

15 years ago
Blocks: 30088
This has happened *many* times to me, and is quite annoying =)

Comment 2

15 years ago
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
Severity: minor → enhancement
(Reporter)

Comment 3

15 years ago
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

15 years ago
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.
(Reporter)

Comment 5

15 years ago
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....
(Assignee)

Updated

15 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → mozilla1.3alpha
(Assignee)

Comment 6

15 years ago
Created attachment 104740 [details] [diff] [review]
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 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
Attachment #104740 - Flags: review+
(Assignee)

Comment 8

15 years ago
Created attachment 104778 [details] [diff] [review]
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=
(Assignee)

Updated

15 years ago
Attachment #104740 - Attachment is obsolete: true
(Assignee)

Comment 9

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

Carrying r= forward.
Attachment #104778 - Flags: review+
Comment on attachment 104778 [details] [diff] [review]
Fix comments for caillon.

sr=bzbarsky
Attachment #104778 - Flags: superreview+
(Assignee)

Comment 11

15 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
hm, wonder if this caused the regression bug 180937?
Hardware: All → PC
blah. nevermind comment 12 --can no longer repro.

anyhow, this looks fine: tested with 2002.11.19.04 comm trunk bits on win2k.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 14

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

Comment 15

15 years ago
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?
(Reporter)

Comment 16

15 years ago
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

15 years ago
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.
(Reporter)

Comment 18

15 years ago
Looks like there was a lot of politics, etc. about backspace earlier -- see bug
#69981, resolved wontfix.
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.
(Assignee)

Comment 20

15 years ago
*** Bug 187270 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 21

15 years ago
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.

Updated

15 years ago
Component: Keyboard: Navigation → Keyboard: Find as you Type

Comment 22

15 years ago
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?
(Assignee)

Comment 23

15 years ago
Tpowell, yeah that's reasonable. Probably better than my original fix.
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.
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.