If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

NEW
Unassigned

Status

()

Firefox
Bookmarks & History
10 years ago
4 days ago

People

(Reporter: juanb, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

User Story

See comment 14

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.

Updated

9 years ago
OS: Mac OS X → All
Hardware: PowerPC → All

Updated

9 years ago
Duplicate of this bug: 475755

Updated

9 years ago
Whiteboard: [good first bug]

Updated

9 years ago
Summary: disable OK button if no folder is selected to move folder into → disable OK button if no folder is selected in Organize > Move... dialog

Comment 2

9 years ago
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

9 years ago
Created attachment 361128 [details] [diff] [review]
Disables OK button if no folder is selected

Updated

9 years ago
Assignee: nobody → lameiro

Updated

9 years ago
Attachment #361128 - Flags: review?(mak77)

Updated

9 years ago
Attachment #361128 - Flags: review?(mak77) → review-
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

9 years ago
Created attachment 362693 [details] [diff] [review]
Second version of the patch, addressing Marco's comments.

Second version of the patch, addressing Marco's comments.
Attachment #361128 - Attachment is obsolete: true

Updated

9 years ago
Duplicate of this bug: 478547

Updated

8 years ago
Attachment #362693 - Flags: review?(mak77)
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
Attachment #362693 - Flags: review?(mak77) → review-
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
Hey Leandro - are you planning on fixing this bug?  If not, we have someone else who may be interested in fixing it.

Comment 10

6 years ago
How are we going on this issue? Could I try to fix it?
(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

6 years ago
Created attachment 619833 [details] [diff] [review]
patch to disable OK button for move bookmark dialog

Comment 13

6 years ago
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.

Updated

5 years ago
Attachment #619833 - Flags: review?(lameiro)

Updated

5 years ago
Attachment #619833 - Flags: review?(mak77)
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 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
Attachment #619833 - Flags: review?(mak77)
Attachment #619833 - Flags: review?(lameiro)
Attachment #619833 - Flags: review-
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?
Flags: needinfo?(mak77)
probably not as easy as a good first bug, due to comment 14
Flags: needinfo?(mak77)
Whiteboard: [good first bug]
Assignee: lameiro → nobody
User Story: (updated)
Duplicate of this bug: 423793
You need to log in before you can comment on or make changes to this bug.