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
We should take this anyway though.
Attachment #314281 - Flags: review?(dietrich)
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040807 Minefield/3.0pre Works normally here.
Comment on attachment 314281 [details] [diff] [review] does this help? r=me
Attachment #314281 - Flags: review?(dietrich) → review+
this got lost, can we push for 3.1 since the fix is still good to take?
Target Milestone: --- → Firefox 3.1
Summary: error adding bookmarking into a New Folder → Errors creating a New Folder in the bookmarks dialogs
Marco, any reason you didn't set the the blocking request flag ?
because it's not blocking, hwv i think we'll push this as soon as the tree is fully open
please request approval for the patch then. it can't land if not blocking+ or a+.
unbitrot and changed a bit (so reasking review to Asaf), any reason why we should not allow creating a new folder in unfiled bookmarks?
forgot parentheses :\
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).
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.
Comment on attachment 351391 [details] [diff] [review] patch Alex, what do you think about above comments?
Attachment #351391 - Flags: ui-review?(faaborg)
Whiteboard: [has patch][has reviews]
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 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-
Summary: Errors creating a New Folder in the bookmarks dialogs → Disable creating a New Folder in the bookmarks dialogs if insertionPoint is invalid
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)
auto-review comment, i can do .disabled = (if condition)
Attachment #351391 - Attachment is obsolete: true
Attachment #314281 - Attachment is obsolete: true
Attachment #356523 - Flags: review?(dietrich) → review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.2a1
Comment on attachment 356523 [details] [diff] [review] patch v2.1 asking approval, polish bug with low risk.
Attachment #356523 - Flags: approval1.9.1?
Attachment #356523 - Flags: approval1.9.1? → approval1.9.1+
Also, should this not have tests? I would be happier if it had tests.
this really really should have a test...
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)
Attachment #369409 - Flags: review?(dietrich) → review+
test pushed on m-c http://hg.mozilla.org/mozilla-central/rev/459c8e138144
Flags: in-testsuite? → in-testsuite+
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?
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.
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.
Comment on attachment 370426 [details] [diff] [review] updated chrome test nice, r=me
Attachment #370426 - Flags: review?(dietrich) → review+
test on trunk http://hg.mozilla.org/mozilla-central/rev/479d0bfb14cb
Flags: in-testsuite? → in-testsuite+
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
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.