462662 - Pressing Enter to select tag from autocomplete closes bookmarks properties dialog

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Henrik Skupin (:whimboo) [⌚️UTC+2] (away 07/24 - 08/10)]

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

If you open the bookmarks properties dialog for a bookmark and try to add some tags, you have to hit enter to select the tag from the autocomplete list. Doing so will close the whole dialog immediately. That shouldn't happen. Instead the tag has to be completed.

Steps to reproduce:
1. Add a bookmark by clicking the star and enter a tag e.g. "bug"
2. Open the bookmarks sidebar and select properties from the newly added bookmark
3. Go to the tags field and remove the tag "bug"
4. Press "b" to show up the autocomplete list - "bug" will be selected
5. Hit Enter

After step 5 the bookmarks properties dialog should not close as what it does actually.

Comment 1

[Peter Lairo]

Perhaps there should be one key (Enter?) to select+close and another (Tab?) to just select a tab from the dropdown list (and perhaps add a comma+<space>) but not close the dialog.

That way users can do both quickly: 
1. enter one tab and be done (via Enter).
2. enter multiple tags (via Tab) and be done (via Enter).

Comment 2

[Henrik Skupin (:whimboo) [⌚️UTC+2] (away 07/24 - 08/10)]

I don't think so that Enter should close the whole dialog. See for example other autocomplete boxes, e.g. location bar. You have to hit Enter twice. And this also makes sense.

Marco, do you have time for that bug? It can only be seen in the new bookmarks dialog. Within the bookmark panel it works fine.

Comment 3

[Dietrich Ayala (:dietrich)]

The behavior should be consistent with the other autocomplete impls in the browser.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch

this fixes:
- pressing enter on any autocomplete popup in bookmarks panels won't submit (we actually have only tag autocomplete, but there's no reason to not be more generic)
- pressing enter on expanders should not submit
- moved listeners code to a common handleEvent to make code cleaner (i should have done this from the beginning really)

Comment 5

[Dietrich Ayala (:dietrich)]

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

looks good, thanks for the cleanup, r=me.

Comment 6

[Marco Bonardo [:mak]]

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

Comment 7

[Marco Bonardo [:mak]]

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

asking approval for this polish and usability bug

Comment 8

[Henrik Skupin (:whimboo) [⌚️UTC+2] (away 07/24 - 08/10)]

Verified with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre (.NET CLR 3.5.30729) ID:20090123033224

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre

Markus, is it ok that we don't react on Enter while having the focus on the expand/collapse buttons? In other applications you will hear a 'dong' that this key is not valid. For now we do nothing.

Comment 9

[Aakash Desai [:aakashd]]

Test case https://litmus.mozilla.org/show_test.cgi?id=7468 has been updated (step 5 was added) to reflect regression testing for this bug.

Comment 10

[Henrik Skupin (:whimboo) [⌚️UTC+2] (away 07/24 - 08/10)]

(In reply to comment #9)
> Test case https://litmus.mozilla.org/show_test.cgi?id=7468 has been updated
> (step 5 was added) to reflect regression testing for this bug.

I've disabled the test until the fix is checked into 1.9.1 branch.

Comment 11

[Marco Bonardo [:mak]]

a test is being added in bug 485458

Comment 12

[Henrik Skupin (:whimboo) [⌚️UTC+2] (away 07/24 - 08/10)]

Aakash, can you please remove the Litmus test? With the automated test landed on 1.9.1 too we don't have to test this manually anymore. Thanks.

Comment 13

[Aakash Desai [:aakashd]]

Litmus test case 7468 has been deleted.

Comment 14

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

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

a191=beltzner

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a1f1d53aae30

Comment 16

[Henrik Skupin (:whimboo) [⌚️UTC+2] (away 07/24 - 08/10)]

Verified fixed on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090416 Shiretoko/3.5b4pre ID:20090416030924 and an appropriate build on WinXP.

Comment 17

[Gervase Markham [:gerv] (not reading bugmail)]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv