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

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Peter van der Woude [:Peter6]]

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

[Asaf Romano (gone)]

Attachment: does this help?

We should take this anyway though.

Comment 2

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040807 Minefield/3.0pre

Works normally here.

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 314281 [details] [diff] [review]
does this help?

r=me

Comment 4

[Marco Bonardo [:mak]]

this got lost, can we push for 3.1 since the fix is still good to take?

Comment 5

[Jim Jeffery not reading bug-mail 1/2/11]

Marco, any reason you didn't set the the blocking request flag ?

Comment 6

[Marco Bonardo [:mak]]

because it's not blocking, hwv i think we'll push this as soon as the tree is fully open

Comment 7

[Dietrich Ayala (:dietrich)]

please request approval for the patch then. it can't land if not blocking+ or a+.

Comment 8

[Marco Bonardo [:mak]]

Attachment: unbitrot

unbitrot and changed a bit (so reasking review to Asaf), any reason why we should not allow creating a new folder in unfiled bookmarks?

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch

forgot parentheses :\

Comment 10

[Asaf Romano (gone)]

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).

Comment 11

[Marco Bonardo [:mak]]

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 12

[Marco Bonardo [:mak]]

Comment on attachment 351391 [details] [diff] [review]
patch

Alex, what do you think about above comments?

Comment 13

[Alex Faaborg [:faaborg] (Firefox UX)]

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

[Alex Faaborg [:faaborg] (Firefox UX)]

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.

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v2

this disables the button if we have no selection or insertionpoint is invalid, hwv for sanity i also leaved in the previous fix.

Comment 16

[Marco Bonardo [:mak]]

auto-review comment, i can do .disabled = (if condition)

Comment 17

[Marco Bonardo [:mak]]

Attachment: patch v2.1

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 356523 [details] [diff] [review]
patch v2.1

r=me

Comment 19

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/23273773e452

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 356523 [details] [diff] [review]
patch v2.1

asking approval, polish bug with low risk.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 356523 [details] [diff] [review]
patch v2.1

a191=beltzner

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Also, should this not have tests? I would be happier if it had tests.

Comment 23

[Shawn Wilsher :sdwilsh]

this really really should have a test...

Comment 24

[Marco Bonardo [:mak]]

Attachment: chrome-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.

Comment 25

[Marco Bonardo [:mak]]

test pushed on m-c
http://hg.mozilla.org/mozilla-central/rev/459c8e138144

Comment 26

[Marco Bonardo [:mak]]

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

Comment 27

[Marco Bonardo [:mak]]

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.

Comment 28

[Marco Bonardo [:mak]]

Attachment: updated chrome test

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 29

[Dietrich Ayala (:dietrich)]

Comment on attachment 370426 [details] [diff] [review]
updated chrome test

nice, r=me

Comment 30

[Marco Bonardo [:mak]]

test on trunk
http://hg.mozilla.org/mozilla-central/rev/479d0bfb14cb

Comment 31

[Marco Bonardo [:mak]]

test on branch
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c73db24dbcf1

Comment 32

[Aakash Desai [:aakashd]]

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

Comment 33

[Gervase Markham [:gerv]]

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