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

VERIFIED FIXED in mozilla1.9beta3

Status

()

P1
normal
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: alfredkayser, Assigned: sdwilsh)

Tracking

Trunk
mozilla1.9beta3
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

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

(Assignee)

Comment 2

11 years ago
(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
(Reporter)

Comment 3

11 years ago
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.
(Assignee)

Comment 4

11 years ago
That makes sense since we are using commands, and how we check if a command is enabled is by testing the selected download.
(Reporter)

Comment 5

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

11 years ago
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.
Flags: blocking-firefox3?

Updated

11 years ago
No longer blocks: 372972
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.
Flags: blocking-firefox3? → blocking-firefox3+
(Reporter)

Comment 8

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

Updated

11 years ago
Target Milestone: --- → Firefox 3 M10

Updated

11 years ago
Priority: -- → P2
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?
(Assignee)

Updated

11 years ago
Whiteboard: [needs patch][needs assignee]

Updated

11 years ago
Version: unspecified → Trunk
(Assignee)

Comment 10

11 years ago
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.
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.
(Assignee)

Comment 12

11 years ago
What about if they delete it during the run of the program?  We wouldn't know...
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..)
(Assignee)

Comment 14

11 years ago
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.
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.)
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...
Depends on: 405886

Updated

11 years ago
Priority: P2 → P1
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?
(Assignee)

Comment 18

11 years ago
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...
(Assignee)

Updated

11 years ago
Assignee: nobody → comrade693+bmo
Whiteboard: [needs patch][needs assignee] → [needs patch]
Target Milestone: Firefox 3 M10 → Firefox 3 M11
(Assignee)

Comment 19

11 years ago
Created attachment 294670 [details] [diff] [review]
0.1

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

Comment 20

11 years ago
Created attachment 294687 [details] [diff] [review]
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.
Attachment #294670 - Attachment is obsolete: true
Attachment #294687 - Flags: review?(edilee)
(Assignee)

Updated

11 years ago
Whiteboard: [needs patch] → [has patch][needs review Mardak]
(Assignee)

Comment 21

11 years ago
Builds are here for people to test out and make sure nothing seems broken.
https://build.mozilla.org/tryserver-builds/2007-12-27_14:22-swilsher@mozilla.com-Bug_392293/
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..
(Assignee)

Comment 23

11 years ago
Mano will be reviewing this - you are just he first pass such that it makes his job easier.
(Assignee)

Comment 24

11 years ago
Created attachment 294842 [details] [diff] [review]
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.
Attachment #294687 - Attachment is obsolete: true
Attachment #294842 - Flags: review?(edilee)
Attachment #294687 - Flags: review?(edilee)
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"/>
(Assignee)

Comment 27

11 years ago
Created attachment 294925 [details] [diff] [review]
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.
Attachment #294842 - Attachment is obsolete: true
Attachment #294925 - Flags: review?(mano)
Attachment #294925 - Flags: review?(edilee)
Attachment #294842 - Flags: review?(edilee)
Comment on attachment 294925 [details] [diff] [review]
v1.2

r=Mardak
Attachment #294925 - Flags: review?(edilee) → review+
(Assignee)

Updated

11 years ago
Whiteboard: [has patch][needs review Mardak] → [has patch][needs review Mano]
(Assignee)

Updated

11 years ago
Blocks: 410289
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.
Attachment #294925 - Flags: review?(mano) → review+
(Assignee)

Updated

11 years ago
Whiteboard: [has patch][needs review Mano] → [has patch][has review][can land]
(Assignee)

Comment 30

11 years ago
Created attachment 295679 [details] [diff] [review]
v1.2.1

Yay bitrot...
Attachment #294925 - Attachment is obsolete: true
(Assignee)

Comment 31

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review][can land]
This caused bug 411172.
Depends on: 411172

Comment 33

11 years ago
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.
Status: RESOLVED → VERIFIED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.