597399 - Panorama: Typing in a digit when the search box isn't displayed produces letter in the box

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Rain [:rain1] (inactive)]

STR:
1. Bring up the Panorama interface
2. Type in a digit without bringing up the search box first

What shows up is "P" + the digit (so "P" for 0, right up to "Y" for 9).

The bug is at <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/search.js#385>: it assumes that the first character is an alphabet.

Comment 2

[Rain [:rain1] (inactive)]

Make that <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/search.js#533> now.

Comment 3

[Kevin Hanes]

Reassigning to Aza for coding, as the bug that was duped already had been.

Comment 4

[Raul Nicolae Malea]

Confirm this bug. Example: 7 is W.

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 6

[Ian Gilman [:iangilman]]

Comment on attachment 487623 [details] [diff] [review]
v1

>-        var keyCode = event.which + (event.shiftKey ? 0 : 32);
>+      if (event != null) {
>+        let keyCode;
>+        // for 0 - 9
>+        if (48 <= event.which && event.which <= 57)
>+          keyCode = event.which;
>+        else
>+          keyCode = event.which + (event.shiftKey ? 0 : 32);

What if we use keypress for beforeSearchKeyHandler (rather than keydown) so we don't have to do this character manipulation?

>+  // the ensureSearchShown() has a setTimeout() so we need to a delay to avoid
>+  // race condition.
>+  setTimeout(function() { 
>+    let searchBox = contentWindow.document.getElementById("searchbox");
>+    is(searchBox.value, number, "The seach box matches the number: " + number);
>+    contentWindow.hideSearch(null);
>+  }, 1);

Is that timeout going to be enough? One technique is to keep polling and calling setTimeout until the search box is shown.

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: v1

Using the keypress for beforeSearchKeyHandler doesn't make any difference because the keypress event occurs when a key is pressed and released over an element.

Updated the test.

Comment 8

[Ian Gilman [:iangilman]]

(In reply to comment #7)
> Using the keypress for beforeSearchKeyHandler doesn't make any difference
> because the keypress event occurs when a key is pressed and released over an
> element.

I'm not sure I understand... shouldn't that be fine?

Maybe using keypress would even get rid of the need for that setTimeout that's probably causing bug 595533.

> Updated the test.

Nicely fixed!

Comment 9

[Raymond Lee [:raymondlee]]

Using keypress and keydown are the same. After the first keystroke is pressed, the keypress and keydown are fired, then you go into the search mode and then the search input field gets the focus.  In other words, the first keystroke never enters into the search input field so we need to do it manually.

Comment 10

[Ian Gilman [:iangilman]]

(In reply to comment #9)
> Using keypress and keydown are the same. After the first keystroke is pressed,
> the keypress and keydown are fired, then you go into the search mode and then
> the search input field gets the focus.  In other words, the first keystroke
> never enters into the search input field so we need to do it manually.

So there's no reason not to use keypress? My suggestion is to use keypress because then we don't need to monkey with the keycode to convert it to a character code. The way we're doing it now (converting it manually) seems like there may be edge cases we're not covering.

Comment 11

[Raymond Lee [:raymondlee]]

(In reply to comment #10)
> So there's no reason not to use keypress? My suggestion is to use keypress
> because then we don't need to monkey with the keycode to convert it to a
> character code. The way we're doing it now (converting it manually) seems like
> there may be edge cases we're not covering.

I am not sure how we can use the keypress event and we don't need to convert keycode manually.  Could you clarify it more please?

Comment 12

[Ian Gilman [:iangilman]]

(In reply to comment #11)
> I am not sure how we can use the keypress event and we don't need to convert
> keycode manually.  Could you clarify it more please?

https://developer.mozilla.org/en/DOM/event.charCode

Keypress gives you a converted character, whereas keyup and keydown need to be converted manually.

Comment 13

[Raymond Lee [:raymondlee]]

Attachment: v1

Use keypress instead of keydown event.

Comment 14

[Ian Gilman [:iangilman]]

Comment on attachment 489193 [details] [diff] [review]
v1

Looks great... What do you think, is it an improvement?

Comment 15

[Raymond Lee [:raymondlee]]

(In reply to comment #14)
> Comment on attachment 489193 [details] [diff] [review]
> v1
> 
> Looks great... What do you think, is it an improvement?

Yeah, it looks better!

Comment 18

[Raymond Lee [:raymondlee]]

Sent it to try 93135af980e9 and waiting for the result

Comment 19

[Raul Nicolae Malea]

This bug is fixed for my Minefield beta8pre. Build 20101116042306, Windows 7 x86. Perhaps you setting Fixed this bug?

Comment 20

[Raymond Lee [:raymondlee]]

(In reply to comment #18)
> Sent it to try 93135af980e9 and waiting for the result

Didn't pass try. Working on a fix.

Comment 21

[Raymond Lee [:raymondlee]]

(In reply to comment #19)
> This bug is fixed for my Minefield beta8pre. Build 20101116042306, Windows 7
> x86. Perhaps you setting Fixed this bug?

I don't see that this is fixed on Mac so a patch is needed

Comment 22

[youwantmyemailhuh]

Not fixed in my Minefield(the latest build). 64bit Windows 7, 32bit Minefield installation.

Comment 23

[Raymond Lee [:raymondlee]]

Attachment: v1

Updated the patch.  It passed try now.

Comment 24

[Ian Gilman [:iangilman]]

Comment on attachment 491194 [details] [diff] [review]
v1

Thanks for fixing the test :)

Comment 25

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 26

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/cf9ba871de20