Closed
Bug 475529
Opened 16 years ago
Closed 16 years ago
Add button in "new folder" dialog not default anymore
Categories
(Firefox :: Bookmarks & History, defect)
Tracking
()
VERIFIED
FIXED
Firefox 3.6a1
People
(Reporter: whimboo, Assigned: mak)
References
Details
(Keywords: regression, verified1.9.1)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
dietrich
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•16 years ago
|
||
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!
Assignee | ||
Comment 2•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs review dietrich]
Comment 3•16 years ago
|
||
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+
Updated•16 years ago
|
Whiteboard: [needs review dietrich]
Assignee | ||
Comment 4•16 years ago
|
||
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 5•16 years ago
|
||
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+
Assignee | ||
Comment 6•16 years ago
|
||
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.
Assignee | ||
Comment 7•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.2a1
Assignee | ||
Updated•16 years ago
|
Attachment #359733 -
Flags: approval1.9.1?
Assignee | ||
Comment 8•16 years ago
|
||
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)
Reporter | ||
Comment 9•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite? → in-testsuite+
Comment 11•16 years ago
|
||
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+
Assignee | ||
Comment 12•16 years ago
|
||
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)
Assignee | ||
Comment 13•16 years ago
|
||
s/before going to trunk/before going to branch/
Assignee | ||
Comment 14•16 years ago
|
||
Keywords: fixed1.9.1
Reporter | ||
Comment 15•16 years ago
|
||
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.
Keywords: fixed1.9.1 → verified1.9.1
Comment 16•15 years ago
|
||
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.
Description
•