Closed Bug 597399 Opened 14 years ago Closed 14 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect, P3)

defect

Tracking

(blocking2.0 final+)

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: rain1, Assigned: raymondlee)

References

Details

(Keywords: polish)

Attachments

(1 file, 4 obsolete files)

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.
blocking2.0: --- → ?
Priority: -- → P3
blocking2.0: ? → final+
Keywords: polish
Blocks: 598154
Reassigning to Aza for coding, as the bug that was duped already had been.
Assignee: nobody → aza
Confirm this bug. Example: 7 is W.
Assignee: aza → raymond
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
Attachment #487623 - Flags: feedback?(ian)
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.
Attachment #487623 - Flags: feedback?(ian) → feedback-
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #487623 - Attachment is obsolete: true
Attachment #487886 - Flags: feedback?(ian)
(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!
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.
(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.
(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?
(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.
Attached patch v1 (obsolete) — Splinter Review
Use keypress instead of keydown event.
Attachment #487886 - Attachment is obsolete: true
Attachment #489193 - Flags: feedback?(ian)
Attachment #487886 - Flags: feedback?(ian)
Comment on attachment 489193 [details] [diff] [review]
v1

Looks great... What do you think, is it an improvement?
Attachment #489193 - Flags: feedback?(ian) → feedback+
Attachment #489193 - Flags: review?(dolske)
(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!
Summary: Panorama: Typing in a digit when the search box isn't displayed produces that digit + "P" in the box → Panorama: Typing in a digit when the search box isn't displayed produces letter in the box
Attachment #489193 - Flags: review?(dolske) → review+
Sent it to try 93135af980e9 and waiting for the result
This bug is fixed for my Minefield beta8pre. Build 20101116042306, Windows 7 x86. Perhaps you setting Fixed this bug?
(In reply to comment #18)
> Sent it to try 93135af980e9 and waiting for the result

Didn't pass try. Working on a fix.
(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
Not fixed in my Minefield(the latest build). 64bit Windows 7, 32bit Minefield installation.
Attached patch v1 (obsolete) — Splinter Review
Updated the patch.  It passed try now.
Attachment #489193 - Attachment is obsolete: true
Attachment #491194 - Flags: review?(ian)
Comment on attachment 491194 [details] [diff] [review]
v1

Thanks for fixing the test :)
Attachment #491194 - Flags: review?(ian) → review+
Attachment #491194 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/cf9ba871de20
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: