463635 - All Tabs search doesn't always update on Enter

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Dão Gottwald [:dao]]

From bug 436304 comment 41:
> * All Tabs search doesn't update on Enter. I.e. when searching a tab and
> quickly hitting Enter, a different tab is selected than when shortly pausing
> before hitting Enter.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 346894 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser-tabPreviews.js b/browser/base/content/browser-tabPreviews.js

>       case "keydown":
>       case "keyup":

>+          if (event.keyCode == event.DOM_VK_RETURN) {
>+            // If there's a pending search, kick it off now.
>+            if (this.searchField._timer)
>+              this.search();

When wouldn't there be a search pending here?

And won't this mean we'll end up searching twice, once now and once when the timer fires? I guess selectThumbnail() closes the panel, which means the second search() will be a no-op because isOpen is false?

>+            event.stopPropagation();

Why is this needed?

Comment 3

[Dão Gottwald [:dao]]

(In reply to comment #2)
> >+          if (event.keyCode == event.DOM_VK_RETURN) {
> >+            // If there's a pending search, kick it off now.
> >+            if (this.searchField._timer)
> >+              this.search();
> 
> When wouldn't there be a search pending here?

When hitting Enter without entering a search term, or when hitting Enter after the timer has ended.

> And won't this mean we'll end up searching twice, once now and once when the
> timer fires? I guess selectThumbnail() closes the panel, which means the second
> search() will be a no-op because isOpen is false?

Yes, it would be a no-op, but this.searchField.value = "" in onPopupHidden would also cancel the timer. Also, if the keypress event reaches the textbox, the timer would be canceled and search() would be called immediately, which would be a no-op as well.

> >+            event.stopPropagation();
> 
> Why is this needed?

I think I wanted event.preventDefault() in order to suppress the keypress event, but this would leave the timer intact, so I can probably just remove that line.

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch

without event.stopPropagation()

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 346947 [details] [diff] [review]
patch

a=beltzner

Comment 6

[Dão Gottwald [:dao]]

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

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified with the following builds. The correct tab is chosen now even the search hasn't finished yet when pressing the Enter key.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081112 Minefield/3.1b2pre ID:20081112020254

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081112 Minefield/3.1b2pre ID:20081112034733