475529 - Add button in "new folder" dialog not default anymore

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

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

The Add button is not default anymore on trunk. Means hitting Enter after giving a name does nothing. Probably a regression from bug 462662.

Steps:
1. Create a new folder on the bookmarks toolbar
2. Give a name
3. Hit the Enter key

With step 3 nothing happens and you have to manually focus the Add button to close the dialog.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

notice we are serving Enter manually, and the default button is not set by design. Still this should work for folders like it does for bookmarks!

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

i was listening on Enter only for properties containing tags, folder does not contain tags clearly.
So this listen for all dialogs, the dialog is accepted on Enter unless:
+          // - the folder tree is focused
+          // - an expander is focused
+          // - an autocomplete (eg. tags) popup is open
+          // - a menulist is open
+          // - a multiline textbox is focused
plus a minor cleanup for aEvent.target

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 359270 [details] [diff] [review]
patch v1.0


>   // nsIDOMEventListener
>   _elementsHeight: [],
>   handleEvent: function BPP_handleEvent(aEvent) {
>+    var target = aEvent.target;
>     switch (aEvent.type) {
>       case "keypress":
>         if (aEvent.keyCode == KeyEvent.DOM_VK_RETURN &&
>-            aEvent.target.localName != "tree" &&
>-            aEvent.target.className != "expander-up" &&
>-            aEvent.target.className != "expander-down" &&
>-            !aEvent.target.popupOpen) {
>-          // Accept the dialog unless the folder tree or an expander are focused
>-          // or an autocomplete popup is open.
>+            target.localName != "tree" &&
>+            target.className != "expander-up" &&
>+            target.className != "expander-down" &&
>+            !target.popupOpen &&
>+            !target.open &&
>+            !(target.localName == "textbox" &&
>+              target.getAttribute("multiline") == "true")) {

can you put all this in an inline function, and documented there?

r=me otherwise

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

like this? would not be better a simple var than to avoid the overhead of a function on every keypress?

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 359733 [details] [diff] [review]
patch v1.1

sure a var is fine. r=me either way.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

mh, rethinking the var would be parsed in any case, so i should do 
if (enter) {
  var = ...
  if (var)
   ...
}
not much cleaner than before.
so i'll retain the last version with the inline function, the cost for having an inline function unused in some case should be low.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 359733 [details] [diff] [review]
patch v1.1

asking approval 1.9.1, this is needed to fix a regression caused by changes to the dialog (those changes are not actually on 1.9.1, but are waiting approval)

Comment 9

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

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090205 Minefield/3.2a1pre ID:20090205020544

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

a test is being added in bug 485458

Comment 11

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

Comment on attachment 359733 [details] [diff] [review]
patch v1.1

a191=beltzner

This should have a test, and in fact we wouldn't have experienced this bug if a test had been in place, so I'll just hope that one appears :)

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the test is already on trunk, disabled on branch due to crash bug 487040 (too much orange randomness), but hope patch there could solve the crash and allow us to re-enable test on branch. since all patches have to bake on trunk before going to trunk test is still doing its work (but i understand this could change in future when trunk code will go far from branch code)

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

s/before going to trunk/before going to branch/

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0796c6d5a680

Comment 15

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

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 16

[Gervase Markham [:gerv]]

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