463299 - Hitting Esc when text was entered in the search box shouldn't close the all tabs / ctrl-tab panel

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

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

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

As we already do in all other dialogs, the panel shouldn't be closed when text is entered in the search box and you hit Esc. In such a case the text only has to be cleared.

Steps:
1. Open at least 3 tabs
2. Open the "all tabs" or ctrl-tab panel
3. Enter some text inside the search box
4. Hit Esc

With step 4 the panel shouldn't be closed. Instead only the text should be cleared.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

Comment 2

[Dão Gottwald [:dao]]

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

>     // advance selection and close panel
>+    EventUtils.synthesizeKey("VK_TAB", {});
>     EventUtils.synthesizeKey("VK_TAB", {});
>     EventUtils.synthesizeKey("VK_RETURN", {});
>     ok(!isOpen(),
>        "Enter key closes the panel");
>     is(gBrowser.tabContainer.selectedIndex, 1,
>        "Tab key advances the selection while the panel is sticky");

Initial focus is on the second preview, but clearing the search field sets focus to the first one. Thus the extra TAB to select the third preview.

Comment 3

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

A second ESC should, however, close it.

Comment 4

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

My knowledge in key handling isn't good enough. So let me ask a question. Shouldn't this be handled by the search widget itself? IMO it doesn't make sense to check it for each instance.

Comment 5

[Dão Gottwald [:dao]]

> Shouldn't this be handled by the search widget itself?

What exactly?

Comment 6

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/40d02210c6c5

Comment 7

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

(In reply to comment #5)
> > Shouldn't this be handled by the search widget itself?
> 
> What exactly?

The topic this bug is about. But to be precise: Shouldn't the search widget handle the ESC key event itself?

(In reply to comment #3)
> A second ESC should, however, close it.

Dao, don't we have a test for that right now? I cannot find it.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> Shouldn't the search widget handle the ESC key event itself?

It does, but it can't prevent capturing event listeners from consuming the event as well.

> > A second ESC should, however, close it.
> 
> Dao, don't we have a test for that right now? I cannot find it.

There's none.

Comment 9

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

Verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081107 Minefield/3.1b2pre

After talking with Marcia on IRC it would be great to have an automated test as well for closing the panel with ESC (see comment 3). Resetting in-testsuite flag to indicate that we need one more test here.

Comment 10

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

Lets carry over the in-testsuite flag to bug 460560 where it better belongs to.