Closed Bug 475529 Opened 11 years ago Closed 11 years ago

Add button in "new folder" dialog not default anymore

Categories

(Firefox :: Bookmarks & History, defect)

3.5 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: whimboo, Assigned: mak)

References

Details

(Keywords: regression, verified1.9.1)

Attachments

(1 file, 1 obsolete file)

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.
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!
Blocks: 462662
Target Milestone: --- → Firefox 3.1
Attached patch patch v1.0 (obsolete) — Splinter Review
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
Attachment #359270 - Flags: review?(dietrich)
Whiteboard: [needs review 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
Attachment #359270 - Flags: review?(dietrich) → review+
Whiteboard: [needs review dietrich]
Attached patch patch v1.1Splinter Review
like this? would not be better a simple var than to avoid the overhead of a function on every keypress?
Attachment #359270 - Attachment is obsolete: true
Attachment #359733 - Flags: review?(dietrich)
Comment on attachment 359733 [details] [diff] [review]
patch v1.1

sure a var is fine. r=me either way.
Attachment #359733 - Flags: review?(dietrich) → review+
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.
http://hg.mozilla.org/mozilla-central/rev/ac0b1b6f5216
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.2a1
Attachment #359733 - Flags: approval1.9.1?
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)
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
Status: RESOLVED → VERIFIED
Depends on: 485458
a test is being added in bug 485458
Flags: in-testsuite?
Flags: in-testsuite? → in-testsuite+
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 :)
Attachment #359733 - Flags: approval1.9.1? → approval1.9.1+
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)
s/before going to trunk/before going to branch/
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.
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.