Closed
Bug 427633
Opened 17 years ago
Closed 16 years ago
Disable creating a New Folder in the bookmarks dialogs if insertionPoint is invalid
Categories
(Firefox :: Bookmarks & History, defect)
Tracking
()
VERIFIED
FIXED
Firefox 3.6a1
People
(Reporter: Peter6, Assigned: mak)
Details
(Keywords: verified1.9.1)
Attachments
(2 files, 5 obsolete files)
2.00 KB,
patch
|
dietrich
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
8.24 KB,
patch
|
dietrich
:
review+
|
Details | Diff | Splinter Review |
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040623 Minefield/3.0pre ID:2008040623
repro:
Open FF
open any page
doubleclick the star in the locationbar
click on the button to the right of the folder field
select [New Folder]
result:
Error: 'Trying to expand a node that is not visible' when calling method: [nsINavHistoryResultViewer::containerOpened] = NS_ERROR_XPC_JS_THREW_STRING
Source file: chrome://browser/content/places/treeView.js
Line: 352
Warning: reference to undefined property menupopup.childNodes[i].folderId
Source file: chrome://browser/content/places/editBookmarkOverlay.js
Line: 646
Warning: reference to undefined property menupopup.childNodes[i].folderId
Source file: chrome://browser/content/places/editBookmarkOverlay.js
Line: 646
Warning: reference to undefined property menupopup.childNodes[i].folderId
Source file: chrome://browser/content/places/editBookmarkOverlay.js
Line: 646
Comment 1•17 years ago
|
||
We should take this anyway though.
Attachment #314281 -
Flags: review?(dietrich)
Comment 2•17 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040807 Minefield/3.0pre
Works normally here.
Comment 3•17 years ago
|
||
Comment on attachment 314281 [details] [diff] [review]
does this help?
r=me
Attachment #314281 -
Flags: review?(dietrich) → review+
Updated•16 years ago
|
Whiteboard: [has patch][has reviews]
Assignee | ||
Comment 4•16 years ago
|
||
this got lost, can we push for 3.1 since the fix is still good to take?
Target Milestone: --- → Firefox 3.1
Assignee | ||
Updated•16 years ago
|
Summary: error adding bookmarking into a New Folder → Errors creating a New Folder in the bookmarks dialogs
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → mano
Status: NEW → ASSIGNED
Comment 5•16 years ago
|
||
Marco, any reason you didn't set the the blocking request flag ?
Assignee | ||
Comment 6•16 years ago
|
||
because it's not blocking, hwv i think we'll push this as soon as the tree is fully open
Comment 7•16 years ago
|
||
please request approval for the patch then. it can't land if not blocking+ or a+.
Assignee | ||
Comment 8•16 years ago
|
||
unbitrot and changed a bit (so reasking review to Asaf), any reason why we should not allow creating a new folder in unfiled bookmarks?
Attachment #351389 -
Flags: review?(mano)
Assignee | ||
Comment 9•16 years ago
|
||
forgot parentheses :\
Attachment #351389 -
Attachment is obsolete: true
Attachment #351389 -
Flags: review?(mano)
Assignee | ||
Updated•16 years ago
|
Attachment #351391 -
Flags: review?(mano)
Comment 10•16 years ago
|
||
Comment on attachment 351391 [details] [diff] [review]
patch
Personally, I don't think folders match the "unfiled" concept. I think the right fix is to disable the button when a readonly is selected (per-api or conceptually).
Attachment #351391 -
Flags: review?(mano)
Assignee | ||
Comment 11•16 years ago
|
||
mh, i noticed we were using PlacesUIUtils to get unfiled's itemId, that's completely wrong it should be PlacesUtils.
So actually the "if" was not checking anything on unfiled root, we always allowed users to create folders inside Unfiled Bookmarks due to that.
I think there's nothing wrong in allowing a user to create a folder in Unfiled Bookmarks if the user selects "Unfiled Bookmarks" in the tree and clicks "New Folder", that's clearly what he wants to do, and from an impl point of view it does not create any issue to us.
Since we allowed that till now i think users could also be used to that with 3.0, and making a new folder in bookmarks menu when they want to create one in Unfiled would cause them think the behaviour is buggy (i selected a root and the new folder has been created in another one!).
about readonly check, getting the insertionPoint should already give us a good writable one or null, isn't it?
i'll ask UX for the unfiled behaviour.
Assignee | ||
Comment 12•16 years ago
|
||
Comment on attachment 351391 [details] [diff] [review]
patch
Alex, what do you think about above comments?
Attachment #351391 -
Flags: ui-review?(faaborg)
Comment 13•16 years ago
|
||
Creating subfolders under unfiled is fine, if the user selects the unfiled root we shouldn't switch them over to another root to create the folder.
Comment 14•16 years ago
|
||
Comment on attachment 351391 [details] [diff] [review]
patch
If we don't have a valid insertion point we should disable the new folder button, not switch the insertion point. However, unfiled bookmarks should be a valid insertion point.
Attachment #351391 -
Flags: ui-review?(faaborg) → ui-review-
Assignee | ||
Updated•16 years ago
|
Summary: Errors creating a New Folder in the bookmarks dialogs → Disable creating a New Folder in the bookmarks dialogs if insertionPoint is invalid
Assignee | ||
Comment 15•16 years ago
|
||
this disables the button if we have no selection or insertionpoint is invalid, hwv for sanity i also leaved in the previous fix.
Assignee: mano → mak77
Attachment #356189 -
Flags: review?(mano)
Assignee | ||
Comment 16•16 years ago
|
||
auto-review comment, i can do .disabled = (if condition)
Assignee | ||
Comment 17•16 years ago
|
||
Attachment #356189 -
Attachment is obsolete: true
Attachment #356523 -
Flags: review?(dietrich)
Attachment #356189 -
Flags: review?(mano)
Assignee | ||
Updated•16 years ago
|
Attachment #351391 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Attachment #314281 -
Attachment is obsolete: true
Updated•16 years ago
|
Attachment #356523 -
Flags: review?(dietrich) → review+
Comment 18•16 years ago
|
||
Comment on attachment 356523 [details] [diff] [review]
patch v2.1
r=me
Assignee | ||
Comment 19•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.2a1
Assignee | ||
Comment 20•16 years ago
|
||
Comment on attachment 356523 [details] [diff] [review]
patch v2.1
asking approval, polish bug with low risk.
Attachment #356523 -
Flags: approval1.9.1?
Updated•16 years ago
|
Attachment #356523 -
Flags: approval1.9.1? → approval1.9.1+
Comment 21•16 years ago
|
||
Comment on attachment 356523 [details] [diff] [review]
patch v2.1
a191=beltzner
Comment 22•16 years ago
|
||
Also, should this not have tests? I would be happier if it had tests.
Assignee | ||
Comment 24•16 years ago
|
||
first stab of a chrome test for the overlay, this is good to test this specific case, but i'll file a followup to add more test cases to this.
Attachment #369409 -
Flags: review?(dietrich)
Updated•16 years ago
|
Attachment #369409 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 25•16 years ago
|
||
test pushed on m-c
http://hg.mozilla.org/mozilla-central/rev/459c8e138144
Flags: in-testsuite? → in-testsuite+
Assignee | ||
Comment 26•16 years ago
|
||
test backed out due to failures on OS X, i actually think the problem is that i should wait for folder selector tree load event. While the test does not fail here, on a slow VM the view could not yet exist when i try to get it.
http://hg.mozilla.org/mozilla-central/rev/b39c0c0674fb
Flags: in-testsuite+ → in-testsuite?
Assignee | ||
Comment 27•16 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d5dd0c6c8936
i'm still working on the test, unluckily i cannot reproduce the issue locally, the test is timing out on tinderboxes, manual testing does not show any problem at all. Will push that apart.
Keywords: fixed1.9.1
Assignee | ||
Comment 28•16 years ago
|
||
i have been finally able to reproduce the hang i was seeing on tinderboxes.
this fixes 2 minor issues found in the overlay that were creating problems to the test.
Attachment #369409 -
Attachment is obsolete: true
Attachment #370426 -
Flags: review?(dietrich)
Comment 29•16 years ago
|
||
Comment on attachment 370426 [details] [diff] [review]
updated chrome test
nice, r=me
Attachment #370426 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 30•16 years ago
|
||
test on trunk
http://hg.mozilla.org/mozilla-central/rev/479d0bfb14cb
Assignee | ||
Comment 31•16 years ago
|
||
Flags: in-testsuite? → in-testsuite+
Comment 32•16 years ago
|
||
verified FIXED on builds: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090422 Minefield/3.6a1pre ID:20090422044118
and
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre ID:20090422042031
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Comment 33•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
•