392293 - The disabled state of the mini-buttons in DM is not handled correctly

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect, P1)

Description

[Alfred Kayser]

When a mini-button gets disabled (for example when selecting an item that cannot be opened, all 'Open' mini-buttons for all items get disabled.

This is not visible in the default theme as clearly, but as soon as one assigns icons for the disabled state for these buttons, one see this very clearly happening.

Note, another problem is thus that the mini-buttons don't have icons for their disabled state.

Comment 1

[mcdavis941 (sporadically reading bugmail)]

Reposting from https://bugzilla.mozilla.org/show_bug.cgi?id=388517#c16 

As far as I can tell, we're (incorrectly) setting disabled on these buttons in the following cases.

- active item's pause and cancel buttons when item not selected
- paused item's resume and cancel buttons when item not selected
- done item's open button when item not selected (but not its info button)

Maybe this was discussed in some other DM bug, but is that really what we want?  The buttons aren't actually disabled (you can click them, and they work).  

Also, if we don't put disabled on them, then you no longer trigger this rule

http://mxr.mozilla.org/mozilla/source/toolkit/themes/winstripe/global/button.css#125

 125 button[disabled="true"] > .button-box {
 126   padding-top: 1px !important;
 127   padding-bottom: 2px !important;
 128   -moz-padding-start: 3px !important;
 129   -moz-padding-end: 4px !important;
 130 }

which means (I think) we no longer need to use !important on this mini-button
rule

http://mxr.mozilla.org/mozilla/source/toolkit/themes/winstripe/mozapps/downloads/downloads.css#50

  49 .mini-button > .button-box {
  50   padding: 0 !important;
  51 }
  52 

which is a win IMHO.

Comment 2

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
> - active item's pause and cancel buttons when item not selected
> - paused item's resume and cancel buttons when item not selected
> - done item's open button when item not selected (but not its info button)
We are not setting them to disabled at all, it's because of this:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/downloads/content/downloads.css&rev=1.9#26

Comment 3

[Alfred Kayser]

Somehow all the mini-buttons are enabled/disabled when you select one row, and sometimes even the type of button change for all rows when you select another row.

Specifically this applies to the 'Open' mini-button. 
If I select a row with something that apparently cannot be 'opened' (why?) then ALL the .open mini-buttons of ALL rows are marked as 'disabled="TRUE"'.
If I select a row with something that apparently can be 'opened',  then ALL the .open mini-buttons of ALL rows are marked as enabled (disabled is removed).
This certainly strange.

Regarding your reference:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/downloads/content/downloads.css&rev=1.9#26
This tells me that only buttons in the current selected row can have 'focus'. While this is right in itself, buttons in other rows are still visible and enable, so still a user can click on them. Which is also ok, but they should not change which the selected row changes.

This is a 'bug', an effect which is not (yet) documented (except in this report) and which is not expected nor desired behavior.

Comment 4

[Shawn Wilsher :sdwilsh]

That makes sense since we are using commands, and how we check if a command is enabled is by testing the selected download.

Comment 5

[Alfred Kayser]

I don't understand your last statement.

As far as I can see currently, the enabling/disabling of buttons of the non-selected items is based on the current selected item. So, for example, if a failed download is selected, the 'open' mini-button of all other rows is disabled. 

This is wrong, as the mini-buttons of the others row should always be enabled, such as the 'open' button for successful downloads.


To phrase this report in the official bug report format:

To reproduce: download a few items, open the download window, abort one item, then select the aborted item. 
Expected result: open buttons of successful items still enabled.
Actual result: open buttons of successful items are disabled.
Note, one can actually press such a disabled open button, and the item is still opened. So, while the button is disabled, it effectively is still 'enabled' (because when one clicks on it, the row becomes 'selected'...)

Simple fix is to NOT disable the open button for completed downloaded items.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Another relevant question here is: should we have disabled images for the default theme, so that the user can actually see when they're disabled?

Requesting wanted-firefox3.

Comment 7

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

I think this is either blocking or invalid, based on the reporter's comment (or a bugmorph to be more about including disabled states)

I'm cc'ing Madhava for the design question, but I believe the design goal should be to remove buttons whereever possible to create a more streamlined UI, and if we can get to a zero-button UI, that would be the bestest.

Comment 8

[Alfred Kayser]

There are two issues here:
1. Some buttons don't have an icon for the disabled state (and may be it is even better to just hide disabled buttons (that is what I have done for the Addons manager))
2. The disabled state is set incorrectly. For example the 'open' mini-button of ALL rows is set to disabled if the current selected row is a failed download. This is invalid behaviour, as one can still click and open an item of another row.

Comment 9

[Ed Lee :Mardak]

sdwilsh: Do we not check "exists" on every file to avoid trying to access each file when populating the list? But the moz-icon protocol to get the file icon essentially does that anyway, so we might as well just check exists and cache that value?

Comment 10

[Shawn Wilsher :sdwilsh]

It doesn't look like we check every download - we only check when a download selection is changed.  That shouldn't happen when the richlistbox is constructed.

Comment 11

[Ed Lee :Mardak]

I suppose it wouldn't be too bad to check if the file exists when we create the UI download item once we get some way to efficiently display all downloads incrementally instead of showing them all at once in one UI update.

Comment 12

[Shawn Wilsher :sdwilsh]

What about if they delete it during the run of the program?  We wouldn't know...

Comment 13

[Ed Lee :Mardak]

Sure, we could check again when the file is selected.

Is there a way to figure out which entries in view to the user? (I was thinking about something like that for displaying all entries... well only add entries when the user is near the bottom..)

Comment 14

[Shawn Wilsher :sdwilsh]

Checking when the file is selected still doesn't accomplish what people want from this bug though - that's the ability to click a button without selecting the download first.

I also don't see anything on richlistitem or richlistbox that would help us.

Comment 15

[Ed Lee :Mardak]

Btw, the open icon is gone after bug 405886. But potentially we might want some way to indicate to the user at a quick glance (gray text) that the file isn't there. (See attachment 291678 [details] for a sample list without icons.)

Comment 16

[Ed Lee :Mardak]

Well, I suppose not the place to discuss, but another sample attachment 291680 [details]. Most downloads will end up gray if we do gray out downloads that don't have a file...

Comment 17

[Ed Lee :Mardak]

What icon are we trying to disable? There's no longer an open button.

There's cancel, pause, resume, retry.

The only one that might potentially be disabled is pause.. but not right now. ;)

So just to clarify, this bug is about being able to get the disabled state of the open button? Or in other words.. some way to determine from CSS if a download has a file that can be opened?

Comment 18

[Shawn Wilsher :sdwilsh]

Really the problem is that the command only updates when the selection changes.  I'm not sure if that really matters anymore though since we got rid of a lot of buttons...

Comment 19

[Shawn Wilsher :sdwilsh]

Attachment: 0.1

Work in progress.  Menuitems in the context menu should be working properly.  Everything else still needs work...

Comment 20

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This seems to do it for me.  I've submitted this to the try server to get some builds for others to test as well.

Comment 21

[Shawn Wilsher :sdwilsh]

Builds are here for people to test out and make sure nothing seems broken.
https://build.mozilla.org/tryserver-builds/2007-12-27_14:[e-mail]-Bug_392293/

Comment 22

[Ed Lee :Mardak]

I'm kinda confused about how the command stuff works, so perhaps someone else should review. Mano?

For all the button commands like..

oncommand="gDownloadViewController.doCommand('cmd_cancel', this.parentNode.parentNode.parentNode);"

it could be simplified to a helper function doCommand('cmd_cancel', this)

.. something along the lines of 
function doCommand(aCmd, aThing) {
// get the containing download item
while (!aThing.hasAttribute("type") || aThing.getAttribute("type") != "download")
  aThing = aThing.parentNode
gDlViewC.doCommand(aCmd, aThing);

That'll help avoid needing to fix up that code each time a button's hierarchy changes.

Similarly for commands like..
oncommand="gDownloadViewController.doCommand('cmd_pause', gDownloadsView.selectedItem);"

Maybe that should be a helper doCommandForSelected(aCmd) that can be used by almost all the menuitems.

Except for..               oncommand="gDownloadViewController.doCommand('cmd_clearList');"
.. which would be good to explicitly call with null?

About aDownloadItem, etc. I've been trying to make a distinction between aDownload and aItem where the former is a nsIDownload and the latter is a richlistitem. I know a lot of places in the code doesn't do that yet though..

Comment 23

[Shawn Wilsher :sdwilsh]

Mano will be reviewing this - you are just he first pass such that it makes his job easier.

Comment 24

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Addresses comments.  We can't use doCommand since that'd try to call nsIXULElement's doCommand method.  We could use window.doCommand instead, but I opted to go for performCommand.

I've submitted this to the try server - will post a link to the builds once it is available.

Comment 25

[Shawn Wilsher :sdwilsh]

Builds are here:
https://build.mozilla.org/tryserver-builds/2007-12-29_09:[e-mail]-Bug_392293/

Comment 26

[Ed Lee :Mardak]

Comment on attachment 294842 [details] [diff] [review]
v1.1

>             <xul:button class="pause mini-button" tooltiptext="&cmd.pause.label;"
>-                        command="cmd_pause" ondblclick="event.stopPropagation();"/>
>+                        cmd="cmd_pause" ondblclick="event.stopPropagation();"
>+                        oncommand="performCommand('cmd_pause', this);"/>
>     <menuitem id="menuitem_pause" 
>               label="&cmd.pause.label;" accesskey="&cmd.pause.accesskey;"
>-              command="cmd_pause"/>
>+              oncommand="performCommand('cmd_pause');"
>+              cmd="cmd_pause"/>
Much cleaner. :)

>@@ -555,23 +546,30 @@ function buildContextMenu(aEvent)
>   if (aEvent.target.id != "downloadContextMenu")
>     return false;
> 
>   var popup = document.getElementById("downloadContextMenu");
>   while (popup.hasChildNodes())
>     popup.removeChild(popup.firstChild);
Should we do the clone and copy here as well? Do we ever set any attributes on the popup?
>   if (gDownloadsView.selectedItem) {
>-    var idx = parseInt(gDownloadsView.selectedItem.getAttribute("state"));
>+    let dl = gDownloadsView.selectedItem;
>+    let idx = parseInt(dl.getAttribute("state"));
>     if (idx < 0)
>       idx = 0;
> 
>     var menus = gContextMenus[idx];
>-    for (var i = 0; i < menus.length; ++i)
>-      popup.appendChild(document.getElementById(menus[i]).cloneNode(true));
>+    for (let i = 0; i < menus.length; ++i) {
>+      let menuitem = document.getElementById(menus[i]).cloneNode(true);
>+      let cmd = menuitem.getAttribute("cmd");
>+      if (cmd)
>+        menuitem.disabled = !gDownloadViewController.isCommandEnabled(cmd, dl);
>+
>+      popup.appendChild(menuitem);
>+    }
> 
>     return true;
>   }
I only briefly looked at the conversation, but I believe Mano was saying that we could prebuild each set of context menus instead of every time? We would prebuiltMenu.cloneNode(true) and fix up the disabled states?

>+  isCommandEnabled: function(aCommand, aItem)
...
>-    var dl = gDownloadsView.selectedItem;
>+    var dl = aItem;
Might as well use let.

>+function performCommand(aCmd, aItem)
>+{
>+  let elm = aItem;
Any reason why you don't just use aItem? Because it's not always a richlistitem?

>@@ -1068,17 +1082,17 @@ function doDefaultForSelected()
>-  gDownloadViewController.doCommand(menuitem.command);
>+  gDownloadViewController.doCommand(menuitem.command, item);
Does menuitem.command still work now that they're "cmd" and "oncommand"?
>-              command="cmd_pause"/>
>+              oncommand="performCommand('cmd_pause');"
>+              cmd="cmd_pause"/>

Comment 27

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

(In reply to comment #26)
> Should we do the clone and copy here as well? Do we ever set any attributes on
> the popup?
I thought about it, but this isn't perf sensitive, and it's outside the scope of this bug.  I'm trying not to change any more code than I have to with us getting so close to shipping.

> Any reason why you don't just use aItem? Because it's not always a
> richlistitem?
Yeah, just for code readability really.  aItem implies it is what was passed in, so if we change that, it could be wrong.

> I only briefly looked at the conversation, but I believe Mano was saying that
> we could prebuild each set of context menus instead of every time? We would
> prebuiltMenu.cloneNode(true) and fix up the disabled states?
We decided that fixing these context menu's the "right" way should probably be done after Firefox 3.

> Does menuitem.command still work now that they're "cmd" and "oncommand"?
Whoops - yeah.  Clearly I didn't test this case.

Comment 28

[Ed Lee :Mardak]

Comment on attachment 294925 [details] [diff] [review]
v1.2

r=Mardak

Comment 29

[Asaf Romano (gone)]

Comment on attachment 294925 [details] [diff] [review]
v1.2

so, mpa=mano, but please make sure to file a bug on re-using command controllers here.

Comment 30

[Shawn Wilsher :sdwilsh]

Attachment: v1.2.1

Yay bitrot...

Comment 31

[Shawn Wilsher :sdwilsh]

Mano - I can't recall what we talked about specifically with those.  Can you elaborate so I can file the follow-up?

Checking in toolkit/mozapps/downloads/content/DownloadProgressListener.js;
new revision: 1.37; previous revision: 1.36
Checking in toolkit/mozapps/downloads/content/download.xml;
new revision: 1.51; previous revision: 1.50
Checking in toolkit/mozapps/downloads/content/downloads.js;
new revision: 1.129; previous revision: 1.128
Checking in toolkit/mozapps/downloads/content/downloads.xul;
new revision: 1.43; previous revision: 1.42

Comment 32

[Dão Gottwald [:dao]]

This caused bug 411172.

Comment 33

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3) Gecko/2008020418 Firefox/3.0b3.   buttons are gone.