433795 - disable OK button if no folder is selected in Organize > Move... dialog

Status: RESOLVED WONTFIX

(Firefox :: Bookmarks & History, defect)

Description

[juan becerra [:juanb]]

While testing Fx3rc1 on 10.5.2, if you select the Mozilla Firefox folder in the Bookmarks Menu in the Library and select the Move option in the organizer you will get an error in the console if you click on the OK button without selecting a folder to move to:

Error: ASSERT: selectedNode must be set in a single-selection tree with initial selection set

The button should not be clickable, unless a valid folder is selected to move the item to.

Comment 2

[Leandro Lameiro]

The following patch fixes this bug by disabling the OK button when the Move Bookmark dialog is loaded, and enables it back when a folder is selected.

Comment 3

[Leandro Lameiro]

Attachment: Disables OK button if no folder is selected

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 361128 [details] [diff] [review]
Disables OK button if no folder is selected

you should get the insertionPoint from the view and check it for null or AllBookmarks (or generically a read-only container)

Comment 5

[Leandro Lameiro]

Attachment: Second version of the patch, addressing Marco's comments.

Second version of the patch, addressing Marco's comments.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 362693 [details] [diff] [review]
Second version of the patch, addressing Marco's comments.

looks overly complex, the asserts are not good, and the dialog, if possible, should be unified to use editBookmarkOverlay.js

Comment 8

[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

Comment 9

[Shawn Wilsher :sdwilsh]

Hey Leandro - are you planning on fixing this bug?  If not, we have someone else who may be interested in fixing it.

Comment 10

[Rodrigo Magalhaes]

How are we going on this issue? Could I try to fix it?

Comment 11

[Marco Bonardo [:mak]]

(In reply to Rodrigo Magalhaes from comment #10)
> How are we going on this issue? Could I try to fix it?

feel free to

Comment 12

[random.devstuff]

Attachment: patch to disable OK button for move bookmark dialog

Comment 13

[random.devstuff]

I noticed there hadn't been any updates to this bug in a while and I was looking for a starter bug to work on. I attached a diff that is similar to the previous one but uses an xml default instead of the logic added to the init method.

Comment 14

[Marco Bonardo [:mak]]

the problem is that we should remove this special dialog and unify the use of editBookmarkOverlay.xul, or directly use bookmarksPanel.xul. Due to that I don't think that adding more complication to moveBookmarks.xul is worth it.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 619833 [details] [diff] [review]
patch to disable OK button for move bookmark dialog

Review of attachment 619833 [details] [diff] [review]:
-----------------------------------------------------------------

That said, thanks for the contribution, if you want to finalize it we could do. As I said it's not a priority or a target, but we don't refuse good contributions just based on that and the review may still be useful for documentaty purposes.

::: browser/components/places/content/moveBookmarks.js
@@ +49,5 @@
>  
> +  _okButton: null,
> +  get okButton() {
> +    if (!this._okButton) {
> +      this._okButton = document.getElementById("moveBookmarkDialog").getButton("accept");

document.documentElement.getButton("accept");

@@ +50,5 @@
> +  _okButton: null,
> +  get okButton() {
> +    if (!this._okButton) {
> +      this._okButton = document.getElementById("moveBookmarkDialog").getButton("accept");
> +      NS_ASSERT(this._okButton, "this._okButton must be non-null");

not useful, please omit

@@ +54,5 @@
> +      NS_ASSERT(this._okButton, "this._okButton must be non-null");
> +    }
> +
> +    return this._okButton;
> +  },

may be simplified to just replace the getter with the value since won't change (delete this.okButton; this.okButton = ...)

@@ +89,5 @@
>      }
>    },
>  
> +  treeSelected: function MBD_treeSelected() {
> +    if (this.foldersTree.insertionPoint && this.okButton && this.okButton.disabled)

I don't think the OK button may even be lacking, and there is no need to check disabled before setting it, indeed this could just be

this.okButton.disabled = !this.foldersTree.insertionPoint

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

mak, should this still be tagged as a good first bug? Also, if we don't intend to fix it because another solution is going to do away with the need to fix it, maybe we should either connect it to another bug as a blocker or resolve it. What do you think?

Comment 17

[Marco Bonardo [:mak]]

probably not as easy as a good first bug, due to comment 14

Comment 19

[Mark Banner (:standard8)]

The move dialog has been removed in bug 1432112 in FF 60, hence this bug is not needed any more.