Disable creating a New Folder in the bookmarks dialogs if insertionPoint is invalid

VERIFIED FIXED in Firefox 3.6a1

Status

()

defect
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Peter6, Assigned: mak)

Tracking

({verified1.9.1})

Trunk
Firefox 3.6a1
x86
Windows XP
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

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
Posted patch does this help? (obsolete) — Splinter Review
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+
Whiteboard: [has patch][has reviews]
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
Assignee: nobody → mano
Status: NEW → ASSIGNED
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+.
Posted patch unbitrot (obsolete) — Splinter Review
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)
Posted patch patch (obsolete) — Splinter Review
forgot parentheses :\
Attachment #351389 - Attachment is obsolete: true
Attachment #351389 - Flags: review?(mano)
Attachment #351391 - Flags: review?(mano)
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)
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)
Keywords: uiwanted
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
Keywords: uiwanted
Posted patch patch v2 (obsolete) — Splinter Review
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)
Posted patch patch v2.1Splinter Review
Attachment #356189 - Attachment is obsolete: true
Attachment #356523 - Flags: review?(dietrich)
Attachment #356189 - Flags: review?(mano)
Attachment #351391 - Attachment is obsolete: true
Attachment #314281 - Attachment is obsolete: true
Attachment #356523 - Flags: review?(dietrich) → review+
http://hg.mozilla.org/mozilla-central/rev/23273773e452
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...
Flags: in-testsuite?
Posted patch chrome-test (obsolete) — Splinter Review
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.
Keywords: fixed1.9.1
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 on attachment 370426 [details] [diff] [review]
updated chrome test

nice, r=me
Attachment #370426 - Flags: review?(dietrich) → review+
test on branch
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c73db24dbcf1
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
Status: RESOLVED → VERIFIED
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.