Closed Bug 1282664 Opened 8 years ago Closed 8 years ago

Redesign the right-click menu for each item in downloads panel

Categories

(Firefox :: Downloads Panel, defect, P2)

Product:
Firefox
Component:
Downloads Panel
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- verified

People

(Reporter: selee, Assigned: selee, NeedInfo)

References

Details

(Whiteboard: [CHE-MVP])

Attachments

(5 files)

Menu Item list
159.62 KB, image/png
Details
Bug 1282664 - Redesign the right-menu on a download item in Downloads Panel.;
58 bytes, text/x-review-board-request
Paolo
: review+
Details
Right clicks on download thread_20161124.pdf
53.10 KB, application/pdf
Details
Malware rename
9.37 MB, video/quicktime
Details
The context menu of Unwanted/Uncommon/Malware items in existence or nonexistence cases
1.98 MB, image/png
Details 
We need to consider the 9 kinds of state when an item is clicked with right-click. This is based on UX spec https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255338 .

9 kinds of state
* Starting/Downloading
* Completed
* Cancelled
* Paused
* Deleted
* Failed
* Malware
* Uncommon Download
Whiteboard: [CHE-MVP]
Priority: -- → P2
Based on UX spec, Bryant helps to come out the major changes for the new design in the right-click menu case:

* Starting/Downloading
  - Remove: clear list
* Completed
  - Remove: remove from history/show in finder/ clear list
  - Add: clear download item
* Cancelled
  - Remove: remove from history/clear list
  - Add: clear download item
* Paused
  - Remove: cancel/ clear list
  - Add: clear download item
* Deleted
  - Remove: remove from history/show in finder/ clear list
  - Add: clear download item
* Failed
  - Remove: cancel/ remove from history/ clear list
  - Add: clear download item
* Malware
  - Remove: remove from history/ clear list
  - Add: show in finder/clear download item
Attached image Menu Item list — 
The attachment is a diagram for the menu item list in the new UX spec.

[1] is relative to the menu items in each state. "Cancel" and "Clear Preview Panel" are never used, so these menu items will be removed.

There are some items should be confirmed:
1. "Deleted" state in spec should be considered, and the downloaded file should be detected. Create a new state if needed.
2. "Scanning" state is not defined in spec.

[1] http://searchfox.org/mozilla-central/source/browser/components/downloads/content/downloads.css#72
Hey Paolo,

Since the spec has a "Deleted" state, I am not sure how difficult to add a new state for handling a deleted file. Probably there are other solutions better than adding a new state. Could you provide your thoughts?
Flags: needinfo?(paolo.mozmail)
Hey Morpheus,

There is a new item "Always download and open this file type" in the latest spec. This feature seems have to change a file-type's RDF configuration.
The scope in this bug is to change the original items to new arrangement based on the previous spec.
I suppose the change in this bug is still worth to do even without "Always download and open this file type" feature.

How do you think let's create a new bug to discuss and implement "Always download and open this file type"?

Thank you.
Flags: needinfo?(mochen)
(In reply to Sean Lee [:seanlee][:weilonge] from comment #4)
> Since the spec has a "Deleted" state, I am not sure how difficult to add a
> new state for handling a deleted file. Probably there are other solutions
> better than adding a new state. Could you provide your thoughts?

We're moving away from considering just one numeric state for the download, which is a legacy of when the downloads were stored in an actual database where the state was a single field.

Now we simply use the properties of the Download object to control the appearance of the item. For example, "download.target.exists" is currently used to control the "exists" attribute and for example you can use the same property to determine if some options should be displayed or not in the context menu, if it is actually appropriate.

One thing we should do in this bug is to control the menu items via JavaScript or via understandable attribute names, rather than using the opaque "state" attribute from CSS.
Flags: needinfo?(paolo.mozmail)
(In reply to Sean Lee [:seanlee][:weilonge] from comment #5)
> Hey Morpheus,
> 
> There is a new item "Always download and open this file type" in the latest
> spec. This feature seems have to change a file-type's RDF configuration.
> The scope in this bug is to change the original items to new arrangement
> based on the previous spec.
> I suppose the change in this bug is still worth to do even without "Always
> download and open this file type" feature.
> 
> How do you think let's create a new bug to discuss and implement "Always
> download and open this file type"?
> 
> Thank you.

We totally agree with your idea to separate "always download and open this file type"( ADOFY for short) as a stand alone bug since it's a new feature. However, please ensure that this feature should be shipped after current user's configurations are applied as ADOFY. We can discuss more details once you create a bug for this feature. Thanks.
Flags: needinfo?(mochen)
Blocks: 1307064
This bug is no longer pending on bug 1301708, as we will keep right-menu items with only string change per offline discussion with UX Morpheus.

Sean will resume implementing this bug.
Assignee: nobody → selee
No longer depends on: 1269964
Comment on attachment 8812116 [details]
Bug 1282664 - Redesign the right-menu on a download item in Downloads Panel.;

Hi Paolo,

This patch includes the right-click menu changes relative to the menu item rearrangement [1].
Could you take a look and give a feedback? Thank you.

[1] https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255338
Attachment #8812116 - Flags: feedback?(paolo.mozmail)
Comment on attachment 8812116 [details]
Bug 1282664 - Redesign the right-menu on a download item in Downloads Panel.;

https://reviewboard.mozilla.org/r/93998/#review94532

I know you have simply looked at the specification when doing these changes, but it should only be used as a starting point, and there are reasons not to implement quite a few of the changes in this patch.

The remaining ones are really minor changes, but I don't have a good picture of what they are yet.

::: browser/components/downloads/content/downloads.css:105
(Diff revision 3)
> +.download-state:not(:-moz-any([state="-1"],/* Starting (initial) */
> +                              [state="0"], /* Downloading        */
> +                              [state="1"], /* Finished           */
> +                              [state="4"], /* Paused             */
> +                              [state="5"], /* Starting (queued)  */
>                                [state="6"], /* Blocked (parental) */
>                                [state="8"], /* Blocked (dirty)    */
>                                [state="9"]) /* Blocked (policy)   */)
> -                                           .downloadRemoveFromHistoryMenuItem,
> +                                           .downloadShowMenuItem,
> +.download-state[state="1"]:not([exists])
> +                                           .downloadShowMenuItem,

The option to show the file in the system's file manager cannot be applied to blocked downloads. If you try it, you will see that you only get access to a folder where the blocked file is present with a temporary extension.

Not only the file cannot be easily accessed, but we also shouldn't have an option to see the temporary file without the user being informed by our blocked download warning.

::: browser/components/downloads/content/downloads.css:139
(Diff revision 3)
> +.download-state:not(:-moz-any([state="1"], /* Finished           */
> +                              [state="2"], /* Failed             */
> +                              [state="3"], /* Canceled           */
> +                              [state="6"], /* Blocked (parental) */
> +                              [state="8"], /* Blocked (dirty)    */
> +                              [state="9"]) /* Blocked (policy)   */)
> +                                           .downloadClearPreviewPanelMenuItem

I can see the point of showing the "Clear Preview Panel" option only on the items that are affected by it. On the other hand, it is a global operation that doesn't depend on the selected item, so there is also an argument for showing it consistently on right click on all items.

Maybe we should simply remove the item if and when we decide to go ahead and enable the new dropdown menu in the footer. I suggest to keep the current behavior of the "Clear Preview Panel" item here, and do the rest together in a different bug.

::: browser/components/downloads/content/downloadsOverlay.xul:64
(Diff revision 3)
> +        <menuitem command="downloadsCmd_unblock"
> +                  class="downloadUnblockMenuItem"
> +                  label="&cmd.unblock3.label;"
> +                  accesskey="&cmd.unblock3.accesskey;"/>
> +        <menuseparator class="downloadBlockSeparator"/>

Given the other comments, you can move this item back to its old position in the source code. In fact, it's never displayed together with the other nearby items.

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd:72
(Diff revision 3)
> -<!ENTITY cmd.unblock2.label               "Allow Download">
> -<!ENTITY cmd.unblock2.accesskey           "o">
> +<!ENTITY cmd.unblock3.label               "Unblock Download">
> +<!ENTITY cmd.unblock3.accesskey           "o">

We just changed the terminology from "Unblock" to "Allow Download" following a UX recommendation during the redesign we did for malware downloads.

https://hg.mozilla.org/mozilla-central/diff/a850056f0527/browser/locales/en-US/chrome/browser/downloads/downloads.dtd

Although I don't have a reference to the original rationale at hand, I don't think it makes sense to switch it back. And anyways, there are other places where we say "Allow", so this change as done here would make the UI inconsistent.
Attachment #8812116 - Flags: feedback?(paolo.mozmail) → feedback-
Comment on attachment 8812116 [details]
Bug 1282664 - Redesign the right-menu on a download item in Downloads Panel.;

https://reviewboard.mozilla.org/r/93998/#review94532

> The option to show the file in the system's file manager cannot be applied to blocked downloads. If you try it, you will see that you only get access to a folder where the blocked file is present with a temporary extension.
> 
> Not only the file cannot be easily accessed, but we also shouldn't have an option to see the temporary file without the user being informed by our blocked download warning.

Need UX's comment here.

> I can see the point of showing the "Clear Preview Panel" option only on the items that are affected by it. On the other hand, it is a global operation that doesn't depend on the selected item, so there is also an argument for showing it consistently on right click on all items.
> 
> Maybe we should simply remove the item if and when we decide to go ahead and enable the new dropdown menu in the footer. I suggest to keep the current behavior of the "Clear Preview Panel" item here, and do the rest together in a different bug.

Need UX's comment here.

> Given the other comments, you can move this item back to its old position in the source code. In fact, it's never displayed together with the other nearby items.

Sorry that I don't get your point here. Do you mean .downloadBlockSeparator in CSS or XUL?
menuseparator.downloadBlockSeparator is between "Allow Download" and "View in Finder" items, so the separator is controlled by s specific rule.
BTW, the order of CSS rules is the same with the order of XUL items. Please correct me if there is anything wrong.

> We just changed the terminology from "Unblock" to "Allow Download" following a UX recommendation during the redesign we did for malware downloads.
> 
> https://hg.mozilla.org/mozilla-central/diff/a850056f0527/browser/locales/en-US/chrome/browser/downloads/downloads.dtd
> 
> Although I don't have a reference to the original rationale at hand, I don't think it makes sense to switch it back. And anyways, there are other places where we say "Allow", so this change as done here would make the UI inconsistent.

Need UX's comment here.
Hi Morpheus,

Could you see Paolo's comment 13? There are three items relative to UX spec:
1. "View in Finder" for Blocked Items.
2. "Clear Preview Panel"
3. The string changes from "Allow Download" to "Unblock Download"
Flags: needinfo?(mochen)
(In reply to :Paolo Amadini from comment #13)
> Comment on attachment 8812116 [details]
> Bug 1282664 - Redesign the right-menu on a download item in Downloads Panel.
> 
> https://reviewboard.mozilla.org/r/93998/#review94532
> 
> I know you have simply looked at the specification when doing these changes,
> but it should only be used as a starting point, and there are reasons not to
> implement quite a few of the changes in this patch.
> 
> The remaining ones are really minor changes, but I don't have a good picture
> of what they are yet.
> 
> ::: browser/components/downloads/content/downloads.css:105
> (Diff revision 3)
> > +.download-state:not(:-moz-any([state="-1"],/* Starting (initial) */
> > +                              [state="0"], /* Downloading        */
> > +                              [state="1"], /* Finished           */
> > +                              [state="4"], /* Paused             */
> > +                              [state="5"], /* Starting (queued)  */
> >                                [state="6"], /* Blocked (parental) */
> >                                [state="8"], /* Blocked (dirty)    */
> >                                [state="9"]) /* Blocked (policy)   */)
> > -                                           .downloadRemoveFromHistoryMenuItem,
> > +                                           .downloadShowMenuItem,
> > +.download-state[state="1"]:not([exists])
> > +                                           .downloadShowMenuItem,
> 
> The option to show the file in the system's file manager cannot be applied
> to blocked downloads. If you try it, you will see that you only get access
> to a folder where the blocked file is present with a temporary extension.
> 
> Not only the file cannot be easily accessed, but we also shouldn't have an
> option to see the temporary file without the user being informed by our
> blocked download warning.
> 
For the context menu of blocked files (malware, unwanted and uncommon), we naturally included the design spec from Sevaan (https://people-mozilla.org/~sfranks/Malware/ ) as we thought it has already been implemented when we starting this project. 

As I understand it, when a suspicious file has been downloaded, Firefox will apply a temporary file name to it to prevent the user from direct access to it.  However, I wonder why we need to overwrite the entire file name instead of just replace the extension? It does not make sense to me as the user might lose track of the file or even unable to recognize it at all. 

Moreover, our user can still access the suspicious file from the system finder even if we remove the "View in Finder" from the context menu. So the tricky thing is that should we even show the file in the directory first if we actually want to block it? 

My thoughts are, however, if a user did download a file from the web and it was marked as a suspicious file by the Firefox. We should either hide it entirely until user unblocks the file, or, allow the user to quickly locate and map the suspicious file with the one the download panel referred so that they could deal with it immediately instead of panicking about the dangerous missing file. 

> ::: browser/components/downloads/content/downloads.css:139
> (Diff revision 3)
> > +.download-state:not(:-moz-any([state="1"], /* Finished           */
> > +                              [state="2"], /* Failed             */
> > +                              [state="3"], /* Canceled           */
> > +                              [state="6"], /* Blocked (parental) */
> > +                              [state="8"], /* Blocked (dirty)    */
> > +                              [state="9"]) /* Blocked (policy)   */)
> > +                                           .downloadClearPreviewPanelMenuItem
> 
> I can see the point of showing the "Clear Preview Panel" option only on the
> items that are affected by it. On the other hand, it is a global operation
> that doesn't depend on the selected item, so there is also an argument for
> showing it consistently on right click on all items.
> 
> Maybe we should simply remove the item if and when we decide to go ahead and
> enable the new dropdown menu in the footer. I suggest to keep the current
> behavior of the "Clear Preview Panel" item here, and do the rest together in
> a different bug.
> 

I agree with you. So I suggest that we should keep the current design first, and remove the "clear list" and "remove from history" from the menu all together once the new dropdown menu is enabled.

> ::: browser/components/downloads/content/downloadsOverlay.xul:64
> (Diff revision 3)
> > +        <menuitem command="downloadsCmd_unblock"
> > +                  class="downloadUnblockMenuItem"
> > +                  label="&cmd.unblock3.label;"
> > +                  accesskey="&cmd.unblock3.accesskey;"/>
> > +        <menuseparator class="downloadBlockSeparator"/>
> 
> Given the other comments, you can move this item back to its old position in
> the source code. In fact, it's never displayed together with the other
> nearby items.
> 
> ::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd:72
> (Diff revision 3)
> > -<!ENTITY cmd.unblock2.label               "Allow Download">
> > -<!ENTITY cmd.unblock2.accesskey           "o">
> > +<!ENTITY cmd.unblock3.label               "Unblock Download">
> > +<!ENTITY cmd.unblock3.accesskey           "o">
> 
> We just changed the terminology from "Unblock" to "Allow Download" following
> a UX recommendation during the redesign we did for malware downloads.
> 
> https://hg.mozilla.org/mozilla-central/diff/a850056f0527/browser/locales/en-
> US/chrome/browser/downloads/downloads.dtd
> 
> Although I don't have a reference to the original rationale at hand, I don't
> think it makes sense to switch it back. And anyways, there are other places
> where we say "Allow", so this change as done here would make the UI
> inconsistent.

For the terminology, I think we should consult with our copywriter Michelle Heubusch first. (Although she already reviewed it once) Can you tell me where the UX recommendation came from? Also, about the other places where we say "Allow," do you have any example that we can reference? Has the word been used under some block context?
Flags: needinfo?(mochen)
Based on Bryant's comment, I've attached the updated spec for context menu, which follows the descoped schedule only. Let me know if any questions, thanks.
Hi Paolo, Please see comment 16 and comment 17. Thanks.
Flags: needinfo?(paolo.mozmail)
(In reply to Bryant Mao [:bryantmao] from comment #16)
> > The option to show the file in the system's file manager cannot be applied
> > to blocked downloads. If you try it, you will see that you only get access
> > to a folder where the blocked file is present with a temporary extension.
> > 
> > Not only the file cannot be easily accessed, but we also shouldn't have an
> > option to see the temporary file without the user being informed by our
> > blocked download warning.
> > 
> For the context menu of blocked files (malware, unwanted and uncommon), we
> naturally included the design spec from Sevaan
> (https://people-mozilla.org/~sfranks/Malware/ ) as we thought it has already
> been implemented when we starting this project.

I'm quite sure the "show in finder" you see in Sevaan's mockup is just an oversight.

> However, I wonder why we need to overwrite the entire file name instead
> of just replace the extension?

We don't, we keep the base file name provided by the site currently.

> My thoughts are, however, if a user did download a file from the web and it
> was marked as a suspicious file by the Firefox. We should either hide it
> entirely until user unblocks the file, or, allow the user to quickly locate
> and map the suspicious file with the one the download panel referred so that
> they could deal with it immediately instead of panicking about the dangerous
> missing file. 

We already provide an option to deal with the file from within Firefox, by deleting it or renaming it to the final name. There is no need for the user to do this manually from the file manager. Currently, if the user renames or moves the file from outside Firefox, we lose track of the file and can't locate it when the command to delete or unblock is chosen. I don't think we should make it easier to do this.

> For the terminology, I think we should consult with our copywriter Michelle
> Heubusch first. (Although she already reviewed it once) Can you tell me
> where the UX recommendation came from?

From copywriter Matej Novak. He reviewed the strings when we implemented protection from uncommon and potentially unwanted downloads. Scroll to the end of this document:

https://docs.google.com/document/d/1P0vP0UJhkXF-jDjTc_lskprhV_uMuqIx_DcdebG8JCw

(I'm glad I found it. Google documents have a tendency to get lost forever.)

> Also, about the other places where we
> say "Allow," do you have any example that we can reference? Has the word
> been used under some block context?

It's still in the context of downloads, in the confirmation dialog shown after the action.
Flags: needinfo?(paolo.mozmail)
Attached video Malware rename — 
(In reply to :Paolo Amadini from comment #19)
> 
> > However, I wonder why we need to overwrite the entire file name instead
> > of just replace the extension?
> 
> We don't, we keep the base file name provided by the site currently.
> 
Hi Paolo, I tried to download a file "badrep.exe" from testsafebrowsing.appspot.com. Before clicking allow download, the file name in Finder is "ivQiEH8A.exe.part". The file is changed to "badrep.exe" after clicking allow download. I just wonder if I misunderstand any details. Thanks.
(In reply to :Paolo Amadini from comment #19)
> (In reply to Bryant Mao [:bryantmao] from comment #16)
> > > The option to show the file in the system's file manager cannot be applied
> > > to blocked downloads. If you try it, you will see that you only get access
> > > to a folder where the blocked file is present with a temporary extension.
> > > 
> > > Not only the file cannot be easily accessed, but we also shouldn't have an
> > > option to see the temporary file without the user being informed by our
> > > blocked download warning.
> > > 
> > For the context menu of blocked files (malware, unwanted and uncommon), we
> > naturally included the design spec from Sevaan
> > (https://people-mozilla.org/~sfranks/Malware/ ) as we thought it has already
> > been implemented when we starting this project.
> 
> I'm quite sure the "show in finder" you see in Sevaan's mockup is just an
> oversight.
> 
Do you mean that included the "show in finder" option in the design of context menu was an accident? 
> > However, I wonder why we need to overwrite the entire file name instead
> > of just replace the extension?
> 
> We don't, we keep the base file name provided by the site currently.
> 
I'm confused, it's conflict with what I tested on this site: https://testsafebrowsing.appspot.com/

Currently, the test case goes like this (please compare each step between the two flows) 
* Note: DL - download folder

Firefox: 
Download malicious file (site) > Check "test" file (DL) > Click and see warning message (DL/Slide in) > Click "open" (DL/Slide in) 

OS download folder:  
N/A >  show malicious file as "REqhBXsC.dms.part" > show malicious file as "REqhBXsC.dms.part" > show malicious file as "test"

So, only if the user open/unblock the malicious file on the download panel, the file name will return to its base name from a random one in the OS download folder.  Or, did I miss understood the word "base file name" ?

> > My thoughts are, however, if a user did download a file from the web and it
> > was marked as a suspicious file by the Firefox. We should either hide it
> > entirely until user unblocks the file, or, allow the user to quickly locate
> > and map the suspicious file with the one the download panel referred so that
> > they could deal with it immediately instead of panicking about the dangerous
> > missing file. 
> 
> We already provide an option to deal with the file from within Firefox, by
> deleting it or renaming it to the final name. There is no need for the user
> to do this manually from the file manager. Currently, if the user renames or
> moves the file from outside Firefox, we lose track of the file and can't
> locate it when the command to delete or unblock is chosen. I don't think we
> should make it easier to do this.
> 
If so, I suggest we hide the malicious file entirely from OS download folder until user open/unblock it on the download panel. Can we do it? 

> > For the terminology, I think we should consult with our copywriter Michelle
> > Heubusch first. (Although she already reviewed it once) Can you tell me
> > where the UX recommendation came from?
> 
> From copywriter Matej Novak. He reviewed the strings when we implemented
> protection from uncommon and potentially unwanted downloads. Scroll to the
> end of this document:
> 
> https://docs.google.com/document/d/1P0vP0UJhkXF-
> jDjTc_lskprhV_uMuqIx_DcdebG8JCw
> 
> (I'm glad I found it. Google documents have a tendency to get lost forever.)
> 
> > Also, about the other places where we
> > say "Allow," do you have any example that we can reference? Has the word
> > been used under some block context?
> 
> It's still in the context of downloads, in the confirmation dialog shown
> after the action.

Thanks for picking up the context. Since Michelle is the one who now responsible for Firefox's content strategy, I think we should include her in the discussion and sync with Matej to define the best string for this option.
Flags: needinfo?(paolo.mozmail)
(In reply to Bryant Mao [:bryantmao] from comment #21)
> Do you mean that included the "show in finder" option in the design of
> context menu was an accident? 

I think so. You can ask Sevaan to be sure. For sure we didn't ever consider that option in the context menu as part of the original design.

> I'm confused, it's conflict with what I tested on this site:
> https://testsafebrowsing.appspot.com/

I see what's happening. We're both right :-)

Firefox always starts downloading with a random file name, then moves the file to the chosen target folder with the final file base name and a ".part" extension as soon as possible during the download process.

The site you're testing from (https://testsafebrowsing.appspot.com/) serves a very short file, so Firefox does not have the time to move the file to the target folder before the download completes. If you tried with a bigger file, you would see the final file name with a ".part" extension.

Since you're testing on Mac, you see this file with the random name in the system Downloads folder, which is used as the temporary location. This does not happen on Windows, where the temporary files are actually stored in the system's temporary directory, so they are invisible.

> If so, I suggest we hide the malicious file entirely from OS download folder
> until user open/unblock it on the download panel. Can we do it? 

The problem is that we need to move the file to the target folder as soon as possible, because the temporary directory and the target folder might be on another drive, and if the target drive is slow (like a USB key) we would incur an unneeded overhead at the end of the download if we waited until the end to move the file. The chosen target folder may also have issues with permissions, in which case we want to fail early instead of discarding the download at the end.

We don't know if a download should be blocked until it's finished, so there's little we can do here.

> Thanks for picking up the context. Since Michelle is the one who now
> responsible for Firefox's content strategy, I think we should include her in
> the discussion and sync with Matej to define the best string for this option.

If there is a strong reason to change the strings back to what they were before Matej updated them, I'm obviously fine with it, but keep in mind that this creates additional work for localizers.

We did the change as part of a project that was specifically about malware downloads, and the difference between what we did at the time and what we are doing now is that, originally, we added a bunch of other strings and all of them were handed out to localizers at the same time. Now we're just fine-tuning an existing string.
Flags: needinfo?(paolo.mozmail)
Based on the discussion with the Michelle today, needinfo Michelle and Matej for clarification.
Flags: needinfo?(mheubusch)
Flags: needinfo?(matej)
Flags: needinfo?(matej)
Comment on attachment 8812116 [details]
Bug 1282664 - Redesign the right-menu on a download item in Downloads Panel.;

https://reviewboard.mozilla.org/r/93998/#review98582

::: browser/components/downloads/content/downloads.css:93
(Diff revision 6)
> -.download-state:not(:-moz-any([state="2"], /* Failed             */
> -                              [state="4"]) /* Paused             */)
> +.download-state:not(          [state="0"]  /* Downloading        */)
> +                                           .downloadPauseMenuItem,

These should be moved back to their original place as we discussed.

::: browser/components/downloads/content/downloads.css
(Diff revision 6)
> -                              [state="2"], /* Failed             */
> +                                           .downloadResumeMenuItem,
> -                              [state="3"], /* Canceled           */
> -                              [state="6"], /* Blocked (parental) */
> -                              [state="8"], /* Blocked (dirty)    */
> -                              [state="9"]) /* Blocked (policy)   */)
> -                                           .downloadRemoveFromHistoryMenuItem,

It looks like we shouldn't remove this rule?

::: browser/components/downloads/content/downloads.css:109
(Diff revision 6)
> +.download-state:not(:-moz-any([state="-1"],/* Starting (initial) */
> +                              [state="0"], /* Downloading        */
> +                              [state="1"], /* Finished           */
> +                              [state="4"], /* Paused             */
> +                              [state="5"], /* Starting (queued)  */
> +                              [state="6"], /* Blocked (parental) */
> +                              [state="8"], /* Blocked (dirty)    */
> +                              [state="9"]) /* Blocked (policy)   */)
> +                                           .downloadCommandsSeparator,

So, the rule should be that the downloadCommandsSeparator should be shown only when at least one of the commands is shown.

At first glance it looks like there may be bugs with state="6", state="9", and .temporary-block, although I've not tested them yet. Can you check?

::: browser/components/downloads/content/downloads.js:1036
(Diff revision 6)
> +    if (element.hasAttribute("exists")) {
> +      contextMenu.setAttribute("exists", "true");
> +    } else {
> +      contextMenu.removeAttribute("exists");
> +    }

This code only executes for the Downloads Panel, probably we need to set the attribute in the Library code as well for the menu item to continue to be shown there. We may need to set the attribute always to true, because we use a different logic to update the commands when the file does not exists.
Attachment #8812116 - Flags: review?(paolo.mozmail)
Comment on attachment 8812116 [details]
Bug 1282664 - Redesign the right-menu on a download item in Downloads Panel.;

https://reviewboard.mozilla.org/r/93998/#review98582

> So, the rule should be that the downloadCommandsSeparator should be shown only when at least one of the commands is shown.
> 
> At first glance it looks like there may be bugs with state="6", state="9", and .temporary-block, although I've not tested them yet. Can you check?

This rule is added to fix a nonexistent file with state=8.

.download-state[state="8"]:not(.temporary-block)
                                           .downloadCommandsSeparator
The screen shot is for showing the unwanted/uncommon/malware items in existence (left) or nonexistence cases (right).
Comment on attachment 8812116 [details]
Bug 1282664 - Redesign the right-menu on a download item in Downloads Panel.;

https://reviewboard.mozilla.org/r/93998/#review98870

r+ with the change below.

::: browser/components/downloads/content/downloads.css:121
(Diff revision 7)
> +                              [state="6"], /* Blocked (parental) */
> +                              [state="8"], /* Blocked (dirty)    */
> +                              [state="9"]) /* Blocked (policy)   */)

I have tested this locally and indeed you need to remove [state="6"] and [state="9"] from the list.

::: browser/components/downloads/content/downloads.css:130
(Diff revision 7)
> +                                           .downloadCommandsSeparator,
> +.download-state[state="8"]:not(.temporary-block)
> +                                           .downloadCommandsSeparator
>  
> -.download-state[state="7"]                 .downloadCommandsSeparator
>  

nit: there are two empty lines now, remove one of them.
Attachment #8812116 - Flags: review?(paolo.mozmail) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ae6e4137b549
Redesign the right-menu on a download item in Downloads Panel.; r=Paolo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ae6e4137b549
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I reproduced this issue using 50.0a1, build ID: 20160627030215, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx53.0a1, build ID: 20170112030301, on Windows 10 x64, Mac OS X 10.10.5 and Ubuntu 14.04 LTS.

Cheers!
status-firefox53: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: