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

Status: VERIFIED FIXED

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

Description

[atlanto]

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

Comment 1

[:Gijs (he/him)]

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?

Comment 2

[:Paolo Amadini]

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.

Comment 3

[Mike de Boer [:mikedeboer]]

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

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1398106 - Check the disabled state for each menuitem in the context menu for a Download in the Library subview.

The 'cmd_delete' command was clashing with another controller which keeps
disabling the menuitem, so it's been renamed to 'downloadsCmd_delete' instead.

Review commit: https://reviewboard.mozilla.org/r/181792/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181792/

Comment 5

[:Paolo Amadini]

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.

Comment 6

[Mike de Boer [:mikedeboer]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181792/diff/1-2/

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to :Paolo Amadini from comment #5)
> Looks good, thanks for the quick fix!

Thanks for the quick review! ;-)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e78305aad7c6

Comment 10

[Valentina Claudia Ona]

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.