Closed Bug 1398106 Opened 4 years ago Closed 4 years ago

'Remove From History' (and other items) in the 'Downloads' context menu do not get the right state/enabled updates

Categories

(Firefox :: Toolbars and Customization, defect, P1)

57 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox58 --- verified

People

(Reporter: euthanasia_waltz, Assigned: mikedeboer)

References

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(1 file)

STR:
0. Download something
1. Open menu panel, go to Library -> Downloads
2. Right-click on the downloaded item

Expected:
'Remove From History' indicates enabled and it works normally.

Actual:
'Remove From History' indicates disabled. It is working when clicked(selected item has been removed), but context menu doesn't disappear.

This does not occur if 'Downloads' come from toolbar button 'Library'.
Blocks: 1354532
Whiteboard: [photon-structure][triage]
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
On OS X, the item is just always disabled for me (but somehow has a hover state, in what I suspect is an unrelated graphics/styling issue).

It seems to me that there's no code to update the state of the commands of this context menu, and any state it has is purely accidental from whatever it was before. Because opening the panel updates the delete command because of the edit UI (on non-OSX) this creates a different behaviour here, but really the core issue is that the subview should be updating the commands based on its state, and my attempts to do that so far have not succeeded. There are so many "update a command" or "update all the commands" methods and helper functions in the downloads code that it's not clear to me how the subview is supposed to do this.  Paolo, how is this supposed to work?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(paolo.mozmail)
Summary: 'Remove From History' on 'Downloads' indicates disabled incorrectly → 'Remove From History' (and other items) in the 'Downloads' context menu do not get the right state/enabled updates
I think the easiest fix here is to change the static updateContextMenu method to go through the items and set them as enabled or disabled based on the result of the button._shell.isCommandEnabled method. Other views use controllers instead, and update the commands in advance using goUpdateDownloadCommands when the item is selected.

In the future we could remove a lot of complexity by avoiding command dispatchers, <command> elements and calls to goDoCommand in all views, for everything except the standard delete command.
Flags: needinfo?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #2)
> In the future we could remove a lot of complexity by avoiding command
> dispatchers, <command> elements and calls to goDoCommand in all views, for
> everything except the standard delete command.

We'll need to fix the keyboard navigation in context menus to not rely on a controller present before then. Or do use a controller, but make it a light-weight proxy that links to the view implementation.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Priority: P3 → P1
Comment on attachment 8910320 [details]
Bug 1398106 - Check the disabled state for each menuitem in the context menu for a Download in the Library subview.

https://reviewboard.mozilla.org/r/181792/#review187240

Looks good, thanks for the quick fix!

::: browser/components/downloads/DownloadsViewUI.jsm:474
(Diff revision 1)
>      window.DownloadURL(this.download.source.url, targetPath, document);
>    },
>  
> +  downloadsCmd_delete() {
> +    // Alias for cmd_delete.
> +    return this.cmd_delete();

nit: command implementations don't return values.

Also, you can improve the comment by saying the reason for the alias, which you already mentioned in the commit message.
Attachment #8910320 - Flags: review?(paolo.mozmail) → review+
(In reply to :Paolo Amadini from comment #5)
> Looks good, thanks for the quick fix!

Thanks for the quick review! ;-)
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e78305aad7c6
Check the disabled state for each menuitem in the context menu for a Download in the Library subview. r=Paolo
Depends on: 1401930
Iteration: 57.3 - Sep 19 → ---
https://hg.mozilla.org/mozilla-central/rev/e78305aad7c6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
I verified this issue using Nightly 58.0a1 on Windows 10 x64, Windows 7 x32, Ubuntu 16.04, Mac OS 10.12 with Build ID 	20170927100120.
I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.