Closed Bug 1269962 Opened 8 years ago Closed 8 years ago

Implement the new features at "Show All Downloads" in Downloads panel.

Categories

(Firefox :: Downloads Panel, defect)

Product:
Firefox
Component:
Downloads Panel
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox51 --- verified

People

(Reporter: selee, Assigned: selee)

References

(Depends on 1 open bug)

Details

(Whiteboard: [CHE-BACKLOG])

Attachments

(3 files, 5 obsolete files)

Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,
58 bytes, text/x-review-board-request
Paolo
: review+
Details
Roll-over effect
23.93 KB, image/png
Details
dropmarker_screen_shot.zip
901.41 KB, application/zip
Details 
Due to bug 1269958, the summary section will be changed to a new design. "Show All Downloads" will be shown only when the cursor hovers on the summary section.
Summary: "Show All Downloads" is shown when the cursor hovers on the summary section in Downloads panel. → Implement the new features at "Show All Downloads" in Downloads panel.
Based on the UX spec [1], there are some items have to be changed:
- A new button for dropdown menu to display the three new features.
   - Clear List
   - Open Downloads Folder
   - Download Settings
- Display “Show All Downloads” section when the mouse hover on the summary section. This only happens when the extra summary section is shown.

[1] https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/156737044
Hey Paolo,

Could you help to give your comments for the changes at Comment 1? Thank you.
Flags: needinfo?(paolo.mozmail)
(In reply to Sean Lee [:seanlee][:weilonge] from comment #1)
>    - Clear List

The reason why we didn't have this as a primary interaction is that the Downloads Panel is not designed as a list to be managed, but an easy to access view for the most common tasks.

There is also a chance of confusion because "Clear List" is not the same as "Clear Downloads History". Users may think that "Clear List" removes all traces of their downloads from history, while it only removes the items from the Downloads Panel. It was kept in the context menu for what we believe are just a few users who were accustomed to the old interface that allowed the list to be cleared.

I'd recommend discussing with Firefox UX whether this has changed and this item needs to be present.

>    - Open Downloads Folder

This is very useful, especially when the list is empty and there is no "Open Containing Folder" icon for the download items. I wonder if there is a way to make it even more visible.

>    - Download Settings

We don't have similar settings buttons except for the search popup, which has probably received a special design, so again I'd double check with Firefox UX if this is actually needed.

The only setting is the preferred downloads folder. I think most users won't need to change that, but if we have reasons to believe users might want to change this frequently enough to deserve a shortcut from the panel, have you considered just moving the setting itself to a sliding view opened by a forward-pointing arrow, instead of having a down arrow?

> - Display “Show All Downloads” section when the mouse hover on the summary
> section. This only happens when the extra summary section is shown.

I'm not too convinced by this mouse hover interaction that hides the entire progress indicator, but I don't have any great suggestion. Maybe there could be a more subtle hover indication like the one visible in Stephen Horlander's visual design mockup for individual download items.
Flags: needinfo?(paolo.mozmail)
Hey Bryant, Morpheus, Could you take look Paolo's comment? Thanks.
Flags: needinfo?(mochen)
Flags: needinfo?(bmao)
Hi Paolo,

Thanks for all your valuable feedback, and we did have a conclusion with Firefox UX for a little feedback you raised. Just for your information, the progress now is that we did presented our spec to all Firefox UX designers for review, and now we are waiting for a final sign-off from Stephen for some minor details. Please find my comments inline for our conclusions with Firefox UX.

(In reply to :Paolo Amadini from comment #3)
> (In reply to Sean Lee [:seanlee][:weilonge] from comment #1)
> >    - Clear List
> 
> The reason why we didn't have this as a primary interaction is that the
> Downloads Panel is not designed as a list to be managed, but an easy to
> access view for the most common tasks.
> 
> There is also a chance of confusion because "Clear List" is not the same as
> "Clear Downloads History". Users may think that "Clear List" removes all
> traces of their downloads from history, while it only removes the items from
> the Downloads Panel. It was kept in the context menu for what we believe are
> just a few users who were accustomed to the old interface that allowed the
> list to be cleared.
> 
> I'd recommend discussing with Firefox UX whether this has changed and this
> item needs to be present.

There are several reasons why we decided to put "clear list" out of right-click menu: 1. Based on our competitor analysis, most major browsers in the market all provide "clear list" in a dominant position on download panel, such as Chrome, Opera, Safari, etc.
2. Taking multiple users into consideration, this can be a very important feature for privacy concern, especially for users in developed countries, where users often share one desktop with other family members.
3. The "clear list" on right-click menu might confuse users since some options are specific to the item they choose and some are not, especially "clear list" is not a very accurate in that context. So we decide to relocate it to the bottom of the list to indicate it's an action for the panel only.
4. Right-click menu is too hidden to find. We did ask some Firefox users if they know right-click menu exists, and the answers are all negative. That means user can hardly find "clear list" in Firefox browser, comparing to other browsers.   
   
> 
> >    - Open Downloads Folder
> 
> This is very useful, especially when the list is empty and there is no "Open
> Containing Folder" icon for the download items. I wonder if there is a way
> to make it even more visible.
> 

I am glad you like it. That's why we decided to highlight this feature. But taking priority into account, this is why we create a drop down menu to collect these features here which are less important than "show all downloads", but more important than options within right-click menu.

> >    - Download Settings
> 
> We don't have similar settings buttons except for the search popup, which
> has probably received a special design, so again I'd double check with
> Firefox UX if this is actually needed.
> 

Our intention is to provide users a shortcut to configure download settings on download panel. And we also checked with Firefox UX about the design of drop down menu. However, if there is no similar buttons across Firefox browser, and engineers need to have extra work for this, then removing it could be a better solution.  

> The only setting is the preferred downloads folder. I think most users won't
> need to change that, but if we have reasons to believe users might want to
> change this frequently enough to deserve a shortcut from the panel, have you
> considered just moving the setting itself to a sliding view opened by a
> forward-pointing arrow, instead of having a down arrow?
> 
> > - Display “Show All Downloads” section when the mouse hover on the summary
> > section. This only happens when the extra summary section is shown.
> 
> I'm not too convinced by this mouse hover interaction that hides the entire
> progress indicator, but I don't have any great suggestion. Maybe there could
> be a more subtle hover indication like the one visible in Stephen
> Horlander's visual design mockup for individual download items.

The main reason why we apply this hover interaction is simply to provide a obvious hint to inform user what's going to happen when they click, and also allow users to access to extra options which are provided when no summary.
Flags: needinfo?(mochen)
Flags: needinfo?(bmao)
(In reply to Morpheus Chen [:morpheus]  UX from comment #5)
> 2. Taking multiple users into consideration, this can be a very important
> feature for privacy concern, especially for users in developed countries,
> where users often share one desktop with other family members.

This is one of the reasons why we didn't expose Clear List out of the contextual menu.
If the user clicks Clear List and thinks his downloads history is gone, he is wrong. Clear List only clears the list in the panel, but if you open Show all downloads all the downloads history is still there (and you must use the Clear Downloads button in the Library to remove it.
So, unless you are also suggesting to change how Clear List behaves, it would be dangerous to just sell it as a privacy feature. My fear is that it has been misinterpreted in this analysis.
Hi Marco,

Our intention is to move Clear List out of right-click menu which is too hidden to find, but collect it in a drop down menu to prioritize these actions for the entire panel. As a result, we don't change the mechanism for Clear List. However, as you mentioned, this does cause the confusion to users if they try to clear not only these items on the panel, but also from the history. Besides, we also discover the behavior behind Clear List on the right-click menu is inconsistent. It seems that generally users can clear items only on the panel by Clear List button, but in certain rules, Clear List can remove the items both on the panel and in the downloads history. 

To simplify all these rules and take care of privacy issue, we might need to change the behavior from only clear list on the panel to both items on panel and history. After we created a simple prototype and test how users interact with this change. We noticed that the string impact users heavily. Therefore, we change the string from Clear List to Clear Above Items to make it more accurate and specific. Thanks for the concerns you raised. We will update the spec about the details and ask copy writer for review about the string.
Quick summary:

Based on descussion above, and latest UX spec[1], what we have in this bug now should be these 2:

1. A new button for dropdown menu to display the three new features.
   a. Clear Above Downloads (to avoid confusion)
   b. Open Downloads Folder

2. Display “Show All Downloads” section when the mouse hover on the summary

[1] attachment in https://bugzilla.mozilla.org/show_bug.cgi?id=1269956

(In reply to Sean Lee [:seanlee][:weilonge] from comment #1)
> Based on the UX spec [1], there are some items have to be changed:
> - A new button for dropdown menu to display the three new features.
>    - Clear List
>    - Open Downloads Folder
>    - Download Settings
> - Display “Show All Downloads” section when the mouse hover on the summary
> section. This only happens when the extra summary section is shown.
> 
> [1] https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/156737044
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Hey Paolo,

This patch provides a submenu to show the new features based on the latest spec [1]. I am not sure that "stack" is been used in the correct way. Could you help to give it a feedback?

Here is the list for some items have not been implemented yet:
1. L10N string.
2. Replace the earth icon
3. Gray line at left of the icon.
4. Two new features in submenu: "Clear Above Downloads" and "Open Downloads Folder" 

I will implement them in the following patches.

Thank you!

[1] https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255325
Attachment #8767923 - Flags: feedback?(paolo.mozmail)
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

(In reply to Sean Lee [:seanlee][:weilonge] from comment #10)
> I am not sure that "stack" is been used in the correct way. Could
> you help to give it a feedback?
> [1] https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255325

Each direct child of the <stack> element is rendered in the same area as the others, I'm not sure it's what you need here. The drop-down button you want seems closer to the one Jonathan is implementing in bug 1267604, you may take a look at the styles he's used there. Also, SVG seems a better choice here for the drop-down arrow.

When implementing the hover state for the status notification, however, you'll probably need a <deck> to display one of two states, either the drop-down button or the status notification. This might get tricky because you'll need to hide the button when the mouse leaves it, but not while the submenu is open. This however is covered by a separate bug if I understand correctly.
Attachment #8767923 - Flags: feedback?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #11)
> Comment on attachment 8767923 [details]
> Bug 1269962 - Implement a popup menu for showing a submenu in Downloads
> Panel Footer.
> 
> Each direct child of the <stack> element is rendered in the same area as the
> others, I'm not sure it's what you need here. The drop-down button you want

I would like to keep #downloadsHistory filling the whole #downloadsFooter  and put #downloadFooterSubPanelButton above it. "stack" is a better solution I found so far, and I will see the patch in bug 1267604 to find a better solution. Thanks for the information.
> seems closer to the one Jonathan is implementing in bug 1267604, you may
> take a look at the styles he's used there. Also, SVG seems a better choice
> here for the drop-down arrow.
May I know what are the benefits to use SVG? using the one SVG file for different DPI?

> 
> When implementing the hover state for the status notification, however,
> you'll probably need a <deck> to display one of two states, either the
> drop-down button or the status notification. This might get tricky because
> you'll need to hide the button when the mouse leaves it, but not while the
> submenu is open. This however is covered by a separate bug if I understand
> correctly.
Yes, you got it. :) The hover behavior will be implemented in bug 1269958.
Hey Paolo,

Do you mean using "popup-notifications" or "button-menubutton-dropmarker" to show the dropdown menu?
Could you help to point out the example code for dropdown menu in the patch of bug 1267604 (or others)?
Thank you!
Flags: needinfo?(paolo.mozmail)
Update the patch attachment 8769622 [details] with SVG and visual style changes
Comment on attachment 8769622 [details]
Use svg.

Hey Paolo, 
This patch uses button to show the dropdown button with a SVG icon.
Could you give a feedback for it? Thanks.
Attachment #8769622 - Flags: feedback?(paolo.mozmail)
Attached file SubMenu changes (obsolete) — 
Review commit: https://reviewboard.mozilla.org/r/63836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63836/
Attachment #8769622 - Flags: feedback?(paolo.mozmail)
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62304/diff/1-2/
Comment on attachment 8769622 [details]
Use svg.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63448/diff/1-2/
Hey Paolo,

Sorry to bother you again for so many changes.
In attachment 8770379 [details], the patch includes the features of Clear Aboe Downloads and Open Downloads Folder.
Please take a look. Thank you!
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62304/diff/2-3/
Attachment #8769622 - Attachment is obsolete: true
Attachment #8770378 - Attachment is obsolete: true
Attachment #8770379 - Attachment is obsolete: true
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Hey Paolo,

All the commits are squashed into one. All features based on [1] are implemented.
Could you give the patch a feedback? Thank you.

[1] UX spec: https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255325
Attachment #8767923 - Flags: feedback?(paolo.mozmail)
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Hey, thanks for doing a preliminary version of all the features! This helps in discussing things further.

With regard to the implementation of the dropdown itself, the general feedback is that you shouldn't use a <stack> here. Note how "Show All Downloads" is not centered in the space to the left of the arrow. Try a <button type="menu-button">.

I still have reservations about the "Clear Above Downloads" functionality. To clarify, these apply to a small percentage of users, since most users will only have few downloads in a session, but I'd like the team to try them out and tell me what you think.

It would be great if you can create a working try build with the current patch for the UX team to test, or simply test together on your computer.

Some cases to test:

1. One paused download.
2. One cancelled download.
3. Two paused downloads, followed by three different finished downloads. [*]
4. Three paused downloads, followed by three different finished downloads.
5. One paused downloads, five different finished downloads, one paused download.

[*] This means finishing three downloads first, and then starting two downloads that are immediately paused. The downloads should be from different URIs.
Flags: needinfo?(paolo.mozmail) → needinfo?(selee)
Attachment #8767923 - Flags: feedback?(paolo.mozmail)
Hey (In reply to :Paolo Amadini from comment #24)
> Comment on attachment 8767923 [details]
> 
> I still have reservations about the "Clear Above Downloads" functionality.
> To clarify, these apply to a small percentage of users, since most users
> will only have few downloads in a session, but I'd like the team to try them
> out and tell me what you think.
> 
> It would be great if you can create a working try build with the current
> patch for the UX team to test, or simply test together on your computer.
> 
> Some cases to test:
> 
> 1. One paused download.
> 2. One cancelled download.
> 3. Two paused downloads, followed by three different finished downloads. [*]
> 4. Three paused downloads, followed by three different finished downloads.
> 5. One paused downloads, five different finished downloads, one paused
> download.
> 
> [*] This means finishing three downloads first, and then starting two
> downloads that are immediately paused. The downloads should be from
> different URIs.

Hey Bryant,

Could you help to give a feedback for the five test cases? Thank you!
Flags: needinfo?(selee) → needinfo?(bmao)
Whiteboard: [CHE-MVP]
NI my self to remind me the <button type="menu-button"> case.
Flags: needinfo?(selee)
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62304/diff/3-4/
Attached image Roll-over effect — 

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Hello Paolo,

Could you help to take a look for this patch?
It changes the button to menu-button, but there are some visual style is not fine-tuned. However, I would like to know your comments for type=menu-button.

BTW, attachment 8771883 [details] is an example to show roll-over effect for cut/copy/paste. The divider will be extended to the full height.
Do you have any suggestion for this design?

Visual Design, Carol, will provide the spec later. Thank you!
Flags: needinfo?(selee)

        Attachment #8767923 -
        Flags: feedback?(paolo.mozmail)

    
    

    
  
A better roll-over example: http://people.mozilla.org/~shorlander/styleguide/ui-elements/panel-download.html
Please see the divider will be extend when the mouse hovers on a download item.

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

(In reply to Sean Lee [:seanlee][:weilonge] from comment #29)
> However, I would like to know your comments for type=menu-button.

Form a quick glance, the code looks good!

> BTW, attachment 8771883 [details] is an example to show roll-over effect for
> cut/copy/paste. The divider will be extended to the full height.
> Do you have any suggestion for this design?

It's implemented in the main menu, you can see it when you hover over the "customize" button, so you may be able to use similar styles. I don't think we need to animate the line extending like it happens in the mockup.

        Attachment #8767923 -
        Flags: feedback?(paolo.mozmail) → feedback+

    
    

    
  
Thanks a lot for your feedback! \O/
I am going to adjust the style to satisfy the visual design.

(In reply to :Paolo Amadini from comment #31)
> > BTW, attachment 8771883 [details] is an example to show roll-over effect for
> > cut/copy/paste. The divider will be extended to the full height.
> > Do you have any suggestion for this design?
> 
> It's implemented in the main menu, you can see it when you hover over the
> "customize" button, so you may be able to use similar styles. I don't think
> we need to animate the line extending like it happens in the mockup.

Sorry that I didn't describe this clear. The line extending design is requested by Visual designer, so I just wonder if we can implement the effect based on type=menu-button.
May I know your thoughts that how we can do that?

Thank you!
Assignee: nobody → selee
Status: NEW → ASSIGNED
Flags: needinfo?(paolo.mozmail)

    
    

    
  
(In reply to :Paolo Amadini from comment #31)
> Comment on attachment 8767923 [details]
> Bug 1269962 - Implement a popup menu for showing a submenu in Downloads
> Panel Footer.> 
> 
> It's implemented in the main menu, you can see it when you hover over the
> "customize" button, so you may be able to use similar styles. I don't think
> we need to animate the line extending like it happens in the mockup.

May I know your concern to do the line extending animation here? Thank you! :)

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62304/diff/4-5/

        Attachment #8767923 -
        Flags: feedback+

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Hey Paolo,

There are two weird bugs in the patch but I have no idea how to fix them. :-/
Could you give me some suggestions?

1. When there are some items in downloads panel, the click area of button[type=menu-button] is very correct. 'show all downloads' button and dropdown menu can be clicked in the normal behavior.

When there is no any item in the panel, only very small area can be clicked in button[type=menu-button]. I am not very sure if it's relative to bug 1194986 (consumeanchor), but it's my only clue to try. It doesn't work anyway.

2. When there are some items in downloads panel, I can click 'show all downloads' correctly. But the dropdown menu will be shown in very short time. Is there any way to now show the dropdown menu?

Thank you!!

        Attachment #8767923 -
        Flags: feedback?(paolo.mozmail)

    
    

    
  
(In reply to Sean Lee [:seanlee][:weilonge] from comment #33)
> (In reply to :Paolo Amadini from comment #31)
> > Comment on attachment 8767923 [details]
> > Bug 1269962 - Implement a popup menu for showing a submenu in Downloads
> > Panel Footer.> 
> > 
> > It's implemented in the main menu, you can see it when you hover over the
> > "customize" button, so you may be able to use similar styles. I don't think
> > we need to animate the line extending like it happens in the mockup.
> 
> May I know your concern to do the line extending animation here? Thank you!

Since we don't have an animation in the main menu, we should not have one here, to keep consistency. We may have a separate bug for implementing the animation in both places if the UX team really wants it.

(In reply to Sean Lee [:seanlee][:weilonge] from comment #35)
> When there is no any item in the panel, only very small area can be clicked
> in button[type=menu-button].

Could it be the same as bug 1280709? There is a patch there that has a workaround in the meantime.

> 2. When there are some items in downloads panel, I can click 'show all
> downloads' correctly. But the dropdown menu will be shown in very short
> time. Is there any way to now show the dropdown menu?

Are you saying that you see the dropdown menu flicker even if you click on the main area instead of the dropdown? I wonder if it's related to the same bug above.
Flags: needinfo?(paolo.mozmail)

    
  

        Attachment #8767923 -
        Flags: feedback?(paolo.mozmail)

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62304/diff/5-6/

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Hey Paolo,

The patch is updated with these changes:
1. Finish the visual changes and Carol has been reviewed it.
2. openDownloadsFolder function is changed for the case that no value is in the preference "browser.download.dir".

(In reply to :Paolo Amadini from comment #36)
> 
> Since we don't have an animation in the main menu, we should not have one
> here, to keep consistency. We may have a separate bug for implementing the
> animation in both places if the UX team really wants it.
After discussing with Carol (Visual Designer), she is fine to not include the animation in this patch. Eventually, she still thinks the animation should be shipped with the animation in main menu items. So the animation will be added when we work on the main menu refine.

> 
> (In reply to Sean Lee [:seanlee][:weilonge] from comment #35)
> > When there is no any item in the panel, only very small area can be clicked
> > in button[type=menu-button].
> 
> Could it be the same as bug 1280709? There is a patch there that has a
> workaround in the meantime.

Thank you very much! This patch works. However, the height of button#downloadsHistory is changed to 40px, so the bug is not that obvious now.

> 
> > 2. When there are some items in downloads panel, I can click 'show all
> > downloads' correctly. But the dropdown menu will be shown in very short
> > time. Is there any way to now show the dropdown menu?
> 
> Are you saying that you see the dropdown menu flicker even if you click on
> the main area instead of the dropdown? I wonder if it's related to the same
> bug above.

The dropdown menu flicker bug still happens after applying the patch in bug 1280709, and I am investigating if the submenu can be disabled in DownloadsPanel.showDownloadsHistory().
May I have your suggestions? Thank you.

        Attachment #8767923 -
        Flags: feedback?(paolo.mozmail)

    
  
Depends on: 1288747

    
    

    
  
(In reply to Sean Lee [:seanlee][:weilonge] from comment #25)
> Hey (In reply to :Paolo Amadini from comment #24)
> > Comment on attachment 8767923 [details]
> > 
> > I still have reservations about the "Clear Above Downloads" functionality.
> > To clarify, these apply to a small percentage of users, since most users
> > will only have few downloads in a session, but I'd like the team to try them
> > out and tell me what you think.
> > 
> > It would be great if you can create a working try build with the current
> > patch for the UX team to test, or simply test together on your computer.
> > 
> > Some cases to test:
> > 
> > 1. One paused download.
> > 2. One cancelled download.
> > 3. Two paused downloads, followed by three different finished downloads. [*]
> > 4. Three paused downloads, followed by three different finished downloads.
> > 5. One paused downloads, five different finished downloads, one paused
> > download.
> > 
> > [*] This means finishing three downloads first, and then starting two
> > downloads that are immediately paused. The downloads should be from
> > different URIs.
> 
> Hey Bryant,
> 
> Could you help to give a feedback for the five test cases? Thank you!

Sorry for the late reply, I could address some of my thoughts here.

For the "Clear Above Downloads" which now we call it "Clear Preview Panel", the aim is to allow the user to remove all finished downloads from the download panel. Under the definition, finished downloads in the summary will also be cleared since they are included in the download panel (belong to the same session). 

As I understand it, there is no sufficient data to prove that there is strong correlation between numbers of download and click rate of the clear list button. But naturally, we can make an assumption on that which lead us to think it will only apply to a small percentage of users. I believe that's why we need to build some telemetry to validate whether the assumption is true.

For the test cases mentioned above, if the "Clear Preview Panel" is clicked the result should go as below:

1. One paused download: keep the paused download
2. One canceled download: empty the panel
3. Two paused downloads, followed by three different finished downloads: keep only the two paused download
4. Three paused downloads, followed by three different finished downloads: keep only the three paused download
5. One paused downloads, five different finished downloads, one paused: keep only the two paused download
Flags: needinfo?(bmao)

    
    

    
  
for comment 39
Flags: needinfo?(paolo.mozmail)

    
    

    
  
Sounds good to me. This is in fact the current behavior of the "Clear List" option. The tricky part is how to name the functionality so that it's not too confusing. "Clear Preview Panel" is definitely closer to the actual function, although if I read it without any background information I might think it opens a new panel to offer a "clear preview" of the downloaded file.

Sean, the context menu flickering looks related to the mouse down event on the main area of the button, but I haven't had time to investigate this yet. Will look into this later together with other feedback on the patch.
Flags: needinfo?(paolo.mozmail)

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62304/diff/6-7/

        Attachment #8767923 -
        Flags: feedback?(paolo.mozmail)

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

attachment 8767923 [details] has been update with these changes:
1. Update the string to "Clear Preview Panel"
2. Update the clear rule based on comment 39

        Attachment #8767923 -
        Flags: feedback?(paolo.mozmail)

    
    

    
  
https://reviewboard.mozilla.org/r/62304/#review64458

::: browser/components/downloads/content/downloads.js:529
(Diff revision 7)
> +    var downloads = DownloadsView._downloads;
> +    downloads.forEach(download => {
> +      let state = DownloadsCommon.stateOfDownload(download);
> +      if (state === nsIDM.DOWNLOAD_FINISHED ||
> +          state === nsIDM.DOWNLOAD_CANCELED) {
> +        DownloadsCommon.removeAndFinalizeDownload(download);
> +      }
> +    });

According to the latest design, you just need to call the same method used by the Clear List command.

::: browser/components/downloads/content/downloads.js:541
(Diff revision 7)
> +    new Promise(resolve => {
> +      const PREF_DOWNLOADS_FOLDER_SETTING = "browser.download.dir";
> +      let prefSvc = Cc["@mozilla.org/preferences-service;1"].
> +                getService(Ci.nsIPrefBranch);
> +      resolve(prefSvc.getCharPref(PREF_DOWNLOADS_FOLDER_SETTING));
> +    }).catch(() => {
> +      return Downloads.getSystemDownloadsDirectory();
> +    }).then(downloadsPath => {
> +      let file = new FileUtils.File(downloadsPath);
> +      DownloadsCommon.showDownloadedFile(file);
> +      this.hidePanel();
> +      return;
> +    });

Don't you just need getPreferredDownloadsDirectory?

::: browser/components/downloads/content/downloads.js:746
(Diff revision 7)
>  
>      // If we've got some hidden downloads, we should activate the
>      // DownloadsSummary. The DownloadsSummary will determine whether or not
>      // it's appropriate to actually display the summary.
>      DownloadsSummary.active = hiddenCount > 0;
> +    DownloadsSummary.showingClearPreviewPanelItem = count > 0;

There is a method to determine if the clear list command should be enabled. The command shouldn't be enabled if the remaining downloads cannot be cleared.

    
  

        Attachment #8767923 -
        Flags: feedback?(paolo.mozmail)

    
    

    
  
I tried these two ways but the flicker issue is still there:

* Use stopPropagation in #downloadsHistory[oncommnad]
    - Snippet Code:
index 69a3257..8f56702 100644
--- a/browser/components/downloads/content/downloadsOverlay.xul
+++ b/browser/components/downloads/content/downloadsOverlay.xul
@@ -153,7 +153,7 @@
                     popup="downloadSubPanel"
                     label="&downloadsHistory.label;"
                     accesskey="&downloadsHistory.accesskey;"
-                    oncommand="DownloadsPanel.showDownloadsHistory();"/>
+                    oncommand="DownloadsPanel.showDownloadsHistory();event.stopPropagation();"/>
             <menupopup id="downloadSubPanel" position="after_end">
               <menuitem id="clearPreviewPanelButton"
                         oncommand="DownloadsPanel.clearPreviewPanel();"
 
    - RESULT
        - flicker issue is still there.

* Set ‘popup’ attribute in xul:dropmarker
    - Snippet Code:
    // These lines are applied in Browser Toolbox.
    var downloadsHistory = document.querySelector('#downloadsHistory’);
    var menuButton = document.getAnonymousElementByAttribute(downloadsHistory, 'class', 'button-menubutton-dropmarker');
    menuButton.setAttribute('popup', 'downloadSubPanel');

    - RESULT
        - popup menu won’t show.

    
    

    
  
(In reply to Sean Lee [:seanlee][:weilonge] from comment #45)
> I tried these two ways but the flicker issue is still there:

I applied the patch locally and I've been able to avoid the flicker by making the <menupopup> a child element of the button. We need to stop the propagation of the event in the menu items to prevent the main command to be called, though.

It's kind of contrived, but I guess this is just how menu buttons are implemented in the rest of the code base anyways, for example here:

https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/toolkit/content/widgets/notification.xml#500

https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/toolkit/modules/PopupNotifications.jsm#642

    
    

    
  
Hey Paolo,
The solution in comment 46 works well, and I will prepare the full patch with comment 44 later.
Thank you so much for this great help!

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62304/diff/7-8/

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Hello Paolo, Could you help to review the patch? Thank you!

        Attachment #8767923 -
        Flags: review?(paolo.mozmail)

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Neat!

I'm going to wait until next week because I'm currently working on the investigation in bug 1288747 and the review of bug 1267604, and we may be able to reuse the styles for the dropdown button in this patch.

It's also a good idea to land this change after the merge anyways, so it gets more testing in Nightly and we may be able to get other panel changes in the same release.

        Attachment #8767923 -
        Flags: feedback+

    
    

    
  
After the investigations in bug 1288747, I'm now trying to share the panel footer styles with the main menu first, which will be done in bug 1291682. This helps in determining if we can use my original suggestion of the button[type=menu-button] and get the correct styling, or we need to do something different.

I'll apply the common styling to the Downloads Panel footer first so we can use it here. Later the team working on popup notifications can inherit it as noted in bug 1267604 comment 64.

Sean, in the meantime you can start working on the regression tests, because I don't think the functionality will change substantially from what you have implemented, there might just be small adjustments if we have to change the implementation of the button.

Obviously we can test may scenarios, but we can work incrementally and only add one test in this patch. The minimum implementation should add one finished download, simulate a click to open the menu, another one to invoke the clear list command, and finally wait for the download to be removed from the list. In the same test we can also check the active/visible state of the menu item before and after the command is invoked.

You'll need a new file in the "browser/components/downloads/test/browser/" folder. The existing tests "browser_basic_functionality.js" and "browser_downloads_panel_block.js" are good starting points, they show how to add downloads, simulate clicks, and check results.
Depends on: 1291682

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

The styles in bug 1001324 have landed. You should now be able to implement the new combined button with the existing <button>, a <toolbarseparator>, and a <button type="menu"> containing the dropmarker icon. This should get the correct hover behavior for the two sections of the footer.

        Attachment #8767923 -
        Flags: review?(paolo.mozmail)

    
    

    
      
  

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Hey Paolo,

Based on comment 52, the latest patch includes the new design of Dropmarker button with the hover effect. Could you give it a feedback?

It's the plan to implement mochitest for the new features. Could you give some suggestions for this plan even the test is still in progress?
1. Verify "Open Downloads Folder" feature.
  a. Open Downloads Panel.
  b. Open the dropdown menu.
  c. Click "Open Downloads Folder"
  d. Check the folder is opened correctly.
2. Verify "Clear Preview List" feature.
  a. Add a new download item which is able to be cleared.
  b. Open Downloads Panel.
  c. Open the dropdown menu.
  d. Click "Clear Preview List"
  e. Check the item is cleared.
3. Verify "Clear Preview List" button appears at the right condition.
  a. Add a new download item [A] which is able to be cleared.
  b. Add a new download item [B] which is not able to be cleared.
  c. Open Downloads Panel.
  d. Open the dropdown menu.
  e. Click "Clear Preview List"
  f. Check item [A] is cleared and item [B] is remained.

For the plan above, there are two cases that I have no idea to handle. (- x.y. - indicates the plan item above.)
- 1.b. - I tried to use these codes to open the dropdown menu, but it didn't show. Is this a correct practice?
    let downloadsDropmarker = document.getElementById('downloadsDropmarker');
    EventUtils.sendMouseEvent({ type: "click" }, downloadsDropmarker);

- 1.d. - Is there any way to check the file manager (e.g. Finder in MacOSX) opened a folder correctly?

Thank you!

        Attachment #8767923 -
        Flags: feedback?(paolo.mozmail)

    
    

    
  
(In reply to Sean Lee [:seanlee][:weilonge] from comment #54)
> 1. Verify "Open Downloads Folder" feature.

We might not be able to test this, because as you've noted we don't have a reliable way to check that the operating system window has opened, and to close it afterwards, which would be required to restore the environment after the test.

We sometimes use mock objects to test that the method is called without opening the window. Because this can get pretty complicated, we can just skip this test for now.

> 3. Verify "Clear Preview List" button appears at the right condition.

This is quite similar to the second test case. What you need to check, in both test cases, is that the menu item is visible or invisible as expected.

> - 1.b. - I tried to use these codes to open the dropdown menu, but it didn't
> show. Is this a correct practice?
>     let downloadsDropmarker = document.getElementById('downloadsDropmarker');
>     EventUtils.sendMouseEvent({ type: "click" }, downloadsDropmarker);

This doesn't look wrong to me, assuming downloadsDropmarker is not null, but maybe menu buttons handle mouse events directly instead of handling click events. I have noticed that browser_downloads_panel_block.js also uses "EventUtils.synthesizeMouse", so you may try to see if this alternative way of simulating the click works better.

I'll take a look at the rest of the patch later, in the meantime I'll note that, if you need to reuse the same code across test files, you should feel free to move it to the "head.js" file in the same folder.

    
    

    
      
  

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Mark f? again. The new patch is for verifying windows/linux styles.

        Attachment #8767923 -
        Flags: feedback?(paolo.mozmail)

    
    

    
      
  

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Thank you for the suggestion for test! Could you give the patch a feedback?

(In reply to :Paolo Amadini from comment #55)
> (In reply to Sean Lee [:seanlee][:weilonge] from comment #54)
> > 1. Verify "Open Downloads Folder" feature.
> 
> We might not be able to test this, because as you've noted we don't have a
> reliable way to check that the operating system window has opened, and to
> close it afterwards, which would be required to restore the environment
> after the test.
> 
> We sometimes use mock objects to test that the method is called without
> opening the window. Because this can get pretty complicated, we can just
> skip this test for now.
Could you give a hint or example for how to use mock object?
Open Download Folder feature will invoke DownloadsCommon.showDownloadedFile
Can we spy or stub DownloadsCommon.showDownloadedFile in mochitest?

> 
> > 3. Verify "Clear Preview List" button appears at the right condition.
> 
> This is quite similar to the second test case. What you need to check, in
> both test cases, is that the menu item is visible or invisible as expected.
> 
> > - 1.b. - I tried to use these codes to open the dropdown menu, but it didn't
> > show. Is this a correct practice?
> >     let downloadsDropmarker = document.getElementById('downloadsDropmarker');
> >     EventUtils.sendMouseEvent({ type: "click" }, downloadsDropmarker);
> 
> This doesn't look wrong to me, assuming downloadsDropmarker is not null, but
> maybe menu buttons handle mouse events directly instead of handling click
> events. I have noticed that browser_downloads_panel_block.js also uses
> "EventUtils.synthesizeMouse", so you may try to see if this alternative way
> of simulating the click works better.
> 
> I'll take a look at the rest of the patch later, in the meantime I'll note
> that, if you need to reuse the same code across test files, you should feel
> free to move it to the "head.js" file in the same folder.

EventUtils.synthesizeMouse works!
The test pattern is created for adding more cases to verify Clear Preview Button.

        Attachment #8767923 -
        Flags: feedback?(paolo.mozmail)

    
    

    
  
(In reply to Sean Lee [:seanlee][:weilonge] from comment #59)
> Could you give a hint or example for how to use mock object?
> Open Download Folder feature will invoke DownloadsCommon.showDownloadedFile
> Can we spy or stub DownloadsCommon.showDownloadedFile in mochitest?

This is a good idea, we don't have a recommended way to spy in mochitests but some time ago we imported a version of Sinon.JS (<http://sinonjs.org/docs/>) in the tree. Currently it's used only once, but you could try to import it here as well:

https://dxr.mozilla.org/mozilla-central/source/browser/components/syncedtabs/test/browser/head.js

The alternative solution we have for Downloads.jsm is the DownloadIntegration module. We use calls to "Integration.downloads.register" to override its functions during unit tests. In the future I think we'll move all the functions that interact with the operating system from DownloadsCommon to DownloadIntegration, and we'll be able to use the same technique.

    
    

    
      
  

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Hey Paolo,

The test has been implemented with sinon.stub. DownloadsCommon.showDownloadedFile should not been called during test, so stub is a good option to verify the download folder path.

Please help to review the patch.

Thank you!

        Attachment #8767923 -
        Flags: review?(paolo.mozmail)

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Thank you! I tested locally and it looks like the styling and behavior is correct, the only thing I noticed is that the pressed state on the dropmarker should look like the main button and persist while the menu is opened, which is probably a simple fix.

There are various other code structure and code style corrections to do, I'll comment in the detailed review as soon as I can get to it in the next few days.

        Attachment #8767923 -
        Flags: feedback+

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

https://reviewboard.mozilla.org/r/62304/#review68728

The patch is quite good! There are still a lot of things to fix that are specific to Firefox coding style and knowledge, but also others that can be noticed by re-reading the code. I've tried to be very detailed in this pass to illustrate the kind of issues a reviewer is looking for.

Next time, before asking for the final review, go through the whole patch line by line and see if anything can be:
 - Renamed to match surrounding code and conventions better
 - Renamed to be specific about what a function or element does
 - Removed because it's no longer necessary
 - Simplified so it doesn't duplicate existing code
 - Likely a problem that has been solved before, and may have an existing function to do it

This may take several hours for a patch of this size, just like a review does, but is very useful. It is basically reviewing your own patch, and is a step towards gaining the required experience with Firefox code to review other people's patches.

::: browser/components/downloads/content/downloads.css:1
(Diff revision 12)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */

This is not the correct file for most of the styling - it should only have CSS that controls the visibility of elements based on state.

The "downloads.inc.css" in the "themes" folder is the right file. As always, pay attention to the order of existing rules when adding new ones. Rules tends to be in the same rough order as the elements in the XUL file.

::: browser/components/downloads/content/downloads.css:27
(Diff revision 12)
> +#downloadsPanel[hasdownloads] #downloadsHistory {
> +  padding-left: 58px !important;
> +}

This is best answered by the visual design team, but I think the text in the menu button should just be centered in the space it has. As far as I can tell we don't use this "absolute" centering in any other place, and it looks weird to me.

::: browser/components/downloads/content/downloads.css:31
(Diff revision 12)
>  
> +#downloadsPanel[hasdownloads] #downloadsHistory {
> +  padding-left: 58px !important;
> +}
> +
> +#downloadsFooter .rollOverSplitter {

Classes must be prefixed, I recommend calling this "downloadsDropmarkerSplitter".

Also you don't need the "#downloadsFooter" selector since it's always matched, and you can solve specificity issues in other ways.

::: browser/components/downloads/content/downloads.css:32
(Diff revision 12)
> +#downloadsPanel[hasdownloads] #downloadsHistory {
> +  padding-left: 58px !important;
> +}
> +
> +#downloadsFooter .rollOverSplitter {
> +  margin: 4px 0;

7px is the correct value to match the main menu and the new popup notifications.

::: browser/components/downloads/content/downloads.css:33
(Diff revision 12)
> +  width: 1px !important;
> +  -moz-appearance: none !important;
> +  border: none;
> +  background-image: none;
> +  background-color: rgb(212, 212, 212);

You don't need to duplicate these properties - just update the selector in the theme styling as I recommend below.

If the margin is ignored because of the higher specificity of the other selector, you can use an ID selector instead of a class for this rule.

::: browser/components/downloads/content/downloads.css:41
(Diff revision 12)
> +  background-image: none;
> +  background-color: rgb(212, 212, 212);
> +}
> +
> +#downloadsFooter:hover .rollOverSplitter {
> +  padding: 0;

You only need to override the margin, the padding doesn't change.

::: browser/components/downloads/content/downloads.css:45
(Diff revision 12)
> +#downloadsFooter:hover .rollOverSplitter {
> +  padding: 0;
> +  margin: 0;
> +}
> +
> +#downloadsDropmarker {

You should use the classes "plain downloadsPanelFooterButton downloadsDropmaker" in the XUL file. This way you don't have to redefine most of the properties and colors.

::: browser/components/downloads/content/downloads.css:46
(Diff revision 12)
> +  margin: 0;
> +  list-style-image: url("chrome://browser/skin/downloads/button_more.svg");
> +  fill: rgb(133, 133, 133);
> +  -moz-image-region: rect(0px, 16px, 16px, 0px);
> +  -moz-appearance: none;
> +  min-width: 0;
> +  max-width: 58px;
> +  height: 16px;
> +  border: none;
> +  background-color: transparent;
> +}
> +
> +#downloadsDropmarker dropmarker.button-menu-dropmarker {
> +  display: none;
> +}
> +
> +#downloadsDropmarker > hbox.box-inherit > hbox.box-inherit {
> +  filter: url("chrome://browser/skin/filters.svg#fill");
> +}
> +
> +#downloadsDropmarker:hover {
> +  outline: 1px solid hsla(210,4%,10%,.07);
> +  background-color: hsla(210,4%,10%,.07);
> +}

We defined the exact styles and dimensions of the dropmarker this week in bug 1267604:

https://reviewboard.mozilla.org/r/61060/diff/15#index_header

See the "popup-notification-dropmarker" additional class and the "menubutton-dropmarker-white.svg" file.

Please copy the SVG file from the other patch, call it "menubutton-dropmarker.svg" in the same directory, and remove the "fill" attribute from inside the SVG file.

Here, copy the relevant rules and selectors from the CSS file in the other patch (they're the correct ones), but add the "filter" property and try to set "fill: currentColor;".

The latter should make the dropmarker appear using the same color as the button text, which is required to support high contrast themes.

::: browser/components/downloads/content/downloads.js:81
(Diff revision 12)
> +const nsIDM = Ci.nsIDownloadManager;
> +

Leftover, not needed in the current patch.

::: browser/components/downloads/content/downloads.js:533
(Diff revision 12)
> +  clearPreviewPanel() {
> +    DownloadsCommon.getData(window).removeFinished();
> +    this.hidePanel();
> +  },
> +
> +  openDownloadsFolder() {

Place new methods related to the footer near the existing showDownloadsHistory() method.

::: browser/components/downloads/content/downloads.js:535
(Diff revision 12)
> +      let file = new FileUtils.File(downloadsPath);
> +      DownloadsCommon.showDownloadedFile(file);

nit: avoid the "file" variable and use a single line.

It turns out that showDownloadedFile is not what we need here, because ".reveal()" doesn't do what we want when called on a directory, at least on OS X. We also don't need the fallback that would show the parent directory of the downloads directory.

We should add a generic showDirectory method to DownloadsCommon that does only the ".launch()" fallback part of showDownloadedFile.

::: browser/components/downloads/content/downloads.js:537
(Diff revision 12)
> +
> +  openDownloadsFolder() {
> +    Downloads.getPreferredDownloadsDirectory().then(downloadsPath => {
> +      let file = new FileUtils.File(downloadsPath);
> +      DownloadsCommon.showDownloadedFile(file);
> +      this.hidePanel();

Move this.hidePanel() outside so it's done immediately.

::: browser/components/downloads/content/downloads.js:538
(Diff revision 12)
> +  openDownloadsFolder() {
> +    Downloads.getPreferredDownloadsDirectory().then(downloadsPath => {
> +      let file = new FileUtils.File(downloadsPath);
> +      DownloadsCommon.showDownloadedFile(file);
> +      this.hidePanel();
> +      return;

The return at the end is not needed.

::: browser/components/downloads/content/downloads.js:539
(Diff revision 12)
> +    Downloads.getPreferredDownloadsDirectory().then(downloadsPath => {
> +      let file = new FileUtils.File(downloadsPath);
> +      DownloadsCommon.showDownloadedFile(file);
> +      this.hidePanel();
> +      return;
> +    });

You need to add ".catch(Cu.reportError)" at the end of Promise chains.

::: browser/components/downloads/content/downloads.js:1332
(Diff revision 12)
> +  set showingClearPreviewPanelItem(aShowingItem) {
> +    document.getElementById("clearPreviewPanelButton").setAttribute("hidden", !aShowingItem);
> +  },

This method shouldn't be on the DownloadsSummary object, that is reserved for the "overflow" summary.

Instead, you should have a onpopupshowing="DownloadsPanel.onFooterPopupShowing();" attribute, and the method will set the visibility of the menu item.

::: browser/components/downloads/content/downloadsOverlay.xul:151
(Diff revision 12)
>                  <description id="downloadsSummaryDetails"
>                               style="width: &downloadDetails.width;"
>                               crop="end"/>
>                </vbox>
>              </hbox>
> +            <hbox id="downloadsFooterButtons" flex="1">

The flex attribute is not necessary here.

::: browser/components/downloads/content/downloadsOverlay.xul:156
(Diff revision 12)
> +            <hbox id="downloadsFooterButtons" flex="1">
> -            <button id="downloadsHistory"
> +              <button id="downloadsHistory"
> -                    class="plain downloadsPanelFooterButton"
> +                      class="plain downloadsPanelFooterButton"
> -                    label="&downloadsHistory.label;"
> +                      label="&downloadsHistory.label;"
> -                    accesskey="&downloadsHistory.accesskey;"
> +                      accesskey="&downloadsHistory.accesskey;"
> +                      flex="1"

I'm not sure if flex is required here, can you check?

::: browser/components/downloads/content/downloadsOverlay.xul:161
(Diff revision 12)
> +                      flex="1"
> -                    oncommand="DownloadsPanel.showDownloadsHistory();"/>
> +                      oncommand="DownloadsPanel.showDownloadsHistory();"/>
> +              <toolbarseparator class="rollOverSplitter"/>
> +              <button id="downloadsDropmarker"
> +                      type="menu"
> +                      flex="1"

Having flex="1" and a "max-width" is not the right solution. As you'll see in the styling for the popup notification, we can just set a fixed width or padding in the CSS.

::: browser/components/downloads/content/downloadsOverlay.xul:162
(Diff revision 12)
> +                      popup="downloadSubPanel">
> +                <menupopup id="downloadSubPanel" position="after_end">

When the popup is inside the button, you don't need an "id" attribute on the menupopup and a "popup" attribute on the button.

::: browser/components/downloads/content/downloadsOverlay.xul:164
(Diff revision 12)
> +              <button id="downloadsDropmarker"
> +                      type="menu"
> +                      flex="1"
> +                      popup="downloadSubPanel">
> +                <menupopup id="downloadSubPanel" position="after_end">
> +                  <menuitem id="clearPreviewPanelButton"

Please prefix all the identifier with "downloads". The name "button" should be avoided for elements that are not buttons.

Also, let's call this function "clearList" internally.

This menu item can be called "downloadsDropdownItemClearList". Same pattern for the other identifiers.

::: browser/components/downloads/content/downloadsOverlay.xul:165
(Diff revision 12)
> +                      type="menu"
> +                      flex="1"
> +                      popup="downloadSubPanel">
> +                <menupopup id="downloadSubPanel" position="after_end">
> +                  <menuitem id="clearPreviewPanelButton"
> +                            oncommand="DownloadsPanel.clearPreviewPanel();event.stopPropagation();"

You should use command="downloadsCmd_clearList" instead, so you don't need a separate function.

::: browser/components/downloads/content/downloadsOverlay.xul:166
(Diff revision 12)
> +                      flex="1"
> +                      popup="downloadSubPanel">
> +                <menupopup id="downloadSubPanel" position="after_end">
> +                  <menuitem id="clearPreviewPanelButton"
> +                            oncommand="DownloadsPanel.clearPreviewPanel();event.stopPropagation();"
> +                            hidden="true"

You don't need hidden="true" because you control the attribute from code.

::: browser/components/downloads/content/downloadsOverlay.xul:169
(Diff revision 12)
> +                  <menuitem id="clearPreviewPanelButton"
> +                            oncommand="DownloadsPanel.clearPreviewPanel();event.stopPropagation();"
> +                            hidden="true"
> +                            label="&clearPreviewPanelButton.label;"/>
> +                  <menuitem id="openDownloadsFolderButton"
> +                            oncommand="DownloadsPanel.openDownloadsFolder();event.stopPropagation();"

You don't need the stopPropagation workaround anymore.

::: browser/components/downloads/test/browser/browser_downloads_panel_footer.js:3
(Diff revision 12)
> +"use strict";
> +
> +add_task(function* mainTest() {

Use the naming pattern "test_nameOfFunctionality":

add_task(function* test_clearList() {
  //...
});

::: browser/components/downloads/test/browser/browser_downloads_panel_footer.js:4
(Diff revision 12)
> +"use strict";
> +
> +add_task(function* mainTest() {
> +  yield verify_openDownloadsFolder();

This should just be a separate test case (separate add_task).

::: browser/components/downloads/test/browser/browser_downloads_panel_footer.js:13
(Diff revision 12)
> +      { state: nsIDM.DOWNLOAD_NOTSTARTED },
> +      { state: nsIDM.DOWNLOAD_FINISHED },
> +      { state: nsIDM.DOWNLOAD_FAILED },
> +      { state: nsIDM.DOWNLOAD_CANCELED },
> +    ],
> +    showClearPreviewButton: true,

expectClearListShown: true,

::: browser/components/downloads/test/browser/browser_downloads_panel_footer.js:14
(Diff revision 12)
> +      { state: nsIDM.DOWNLOAD_FINISHED },
> +      { state: nsIDM.DOWNLOAD_FAILED },
> +      { state: nsIDM.DOWNLOAD_CANCELED },
> +    ],
> +    showClearPreviewButton: true,
> +    expectedItemNumber: 0

nit: keep the comma at the end of multi-line declarations:

expectedItemNumber: 0,

::: browser/components/downloads/test/browser/browser_downloads_panel_footer.js:33
(Diff revision 12)
> +  for (let pattern in TEST_PATTERN) {
> +    yield verify_clearPreviewList(TEST_PATTERN[pattern]);
> +  }

Rename and use this construct:

for (let testCase of kTestCases) {
  yield verify_clearList(testCase);
}

Move the verify_clearList function to be right after this test function.

::: browser/components/downloads/test/browser/browser_downloads_panel_footer.js:66
(Diff revision 12)
> +  });
> +
> +  yield task_resetState();
> +}
> +
> +function *verify_clearPreviewList(testData) {

nit: testCase instead of testData to match the naming above.

::: browser/components/downloads/test/browser/browser_downloads_panel_footer.js:67
(Diff revision 12)
> +
> +  yield task_resetState();
> +}
> +
> +function *verify_clearPreviewList(testData) {
> +  // a. Add the download items.

Unless it's needed because the flow does not follow the reading order, avoid enumerations like "a.", "b." in comments. They don't serve a purpose and are invalidated when code is added in between.

Also, it seems to me that all the comments in this functon are obvious from reading the code and can be removed.

::: browser/components/downloads/test/browser/browser_downloads_panel_footer.js:68
(Diff revision 12)
> +  const DownloadData = testData.downloads;
> +  yield task_addDownloads(DownloadData);

You can just inline testCase.downloas, otherwise our convention is to use "let downloads = testCase.downloads;".

::: browser/components/downloads/test/browser/browser_downloads_panel_footer.js:73
(Diff revision 12)
> +  const DownloadData = testData.downloads;
> +  yield task_addDownloads(DownloadData);
> +
> +  // b. Open Downloads Panel.
> +  yield task_openPanel();
> +  is(DownloadsView._downloads.length, DownloadData.length, "There are " + DownloadData.length + " download items.");

The test outputs the values already, no need to repeat in the description. Also, please wrap at 80 characters.

::: browser/components/downloads/test/browser/browser_downloads_panel_footer.js:78
(Diff revision 12)
> +  yield promisePolling(() => {
> +    return "true" === downloadsDropmarker.getAttribute("_moz-menuactive");
> +  });

When we can, we should wait for specific events rather than polling, for example "popupshown" here. There are helper function to wait for events in BrowserTestUtils, you can search DXR for examples.

::: browser/components/downloads/test/browser/head.js:31
(Diff revision 12)
>  gTestTargetFile.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
> +
> +// Load mocking/stubbing library, sinon
> +// docs: http://sinonjs.org/docs/
> +let loader = Cc["@mozilla.org/moz/jssubscript-loader;1"].getService(Ci.mozIJSSubScriptLoader);
> +loader.loadSubScript("resource://testing-common/sinon-1.16.1.js");

You can use "Services.scriptloader.loadSubScript". The code you copied may be older that when the method was available, or the author was unaware of its existence.

::: browser/components/downloads/test/browser/head.js:113
(Diff revision 12)
> +function promisePolling(condition) {
> +  return new Promise(resolve => {
> +    let interval = setInterval(() => {
> +      if (condition()) {
> +        clearInterval(interval);
> +        setTimeout(resolve, 1000);
> +        return;
> +      }
> +    }, 0);
> +  });
> +}

There is an existing "BrowserTestUtils.waitForCondition" function to do this, with added safeguards against long test timeouts.

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd:65
(Diff revision 12)
>  <!ENTITY cmd.goToDownloadPage.accesskey   "G">
>  <!ENTITY cmd.copyDownloadLink.label       "Copy Download Link">
>  <!ENTITY cmd.copyDownloadLink.accesskey   "L">
>  <!ENTITY cmd.removeFromHistory.label      "Remove From History">
>  <!ENTITY cmd.removeFromHistory.accesskey  "e">
> -<!ENTITY cmd.clearList.label              "Clear List">
> +<!ENTITY cmd.clearList.label              "Clear Preview Panel">

When you change the value of an entity, you should change the name too, otherwise the localizers won't see the difference. The "accesskey" must be renamed too.

You can call this "cmd.clearList2.label" and "cmd.clearList2.accesskey" for example.

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd:112
(Diff revision 12)
>       the panel at all.
>       -->
>  <!ENTITY downloadsHistory.label           "Show All Downloads">
>  <!ENTITY downloadsHistory.accesskey       "S">
>  
> +<!ENTITY downloadFooterSubPanelButton.accesskey       "P">

This is unused.

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd:114
(Diff revision 12)
>  <!ENTITY downloadsHistory.label           "Show All Downloads">
>  <!ENTITY downloadsHistory.accesskey       "S">
>  
> +<!ENTITY downloadFooterSubPanelButton.accesskey       "P">
> +
> +<!ENTITY clearPreviewPanelButton.label       "Clear Preview Panel">

No need for a different label, reuse the existing one from the context menu.

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd:116
(Diff revision 12)
>  
> +<!ENTITY downloadFooterSubPanelButton.accesskey       "P">
> +
> +<!ENTITY clearPreviewPanelButton.label       "Clear Preview Panel">
> +
> +<!ENTITY openDownloadsFolderButton.label       "Open Downloads Folder">

Just "openDownloadsFolder.label".

::: browser/themes/shared/downloads/downloads.inc.css:34
(Diff revision 12)
>    color: inherit;
>  }
>  
>  #emptyDownloads {
> -  padding: 10px 20px;
> +  height: 40px;
> +  padding: 16px 20px;

Use "padding: 0 20px;" since we have the height already set. Note that we might change the height of the footer soon, but for now this should be fine.

::: browser/themes/shared/downloads/downloads.inc.css:45
(Diff revision 12)
>  .downloadsPanelFooter {
>    background-color: hsla(210,4%,10%,.07);
>    border-top: 1px solid hsla(210,4%,10%,.14);
>  }
>  
>  .downloadsPanelFooter > toolbarseparator {

This should now become a descendant selector (remove ">"), because you added a new container in-between in the main view, which isn't present in the subview.

::: browser/themes/shared/downloads/downloads.inc.css:67
(Diff revision 12)
>  .downloadsPanelFooterButton:hover {
>    outline: 1px solid hsla(210,4%,10%,.07);
>    background-color: hsla(210,4%,10%,.07);
>  }
>  
>  .downloadsPanelFooterButton:hover:active {

You should add this selector to this rule:

.downloadsPanelFooterButton\[open="true"\]

Unfortunately, there is a glitch with the separator when the mouse is moved outside of the menu, because we cannot know if the menu is open to control the hover effect. I guess we have to add some JS code that sets or removes a class while the menu is open, to avoid the separator shrinking. We can use onpopupshowing (since we need it already) and onpopuphidden for this.

        Attachment #8767923 -
        Flags: review?(paolo.mozmail)

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8781507 -
        Attachment is obsolete: true

    
  

        Attachment #8781508 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

https://reviewboard.mozilla.org/r/62304/#review68728

> This is best answered by the visual design team, but I think the text in the menu button should just be centered in the space it has. As far as I can tell we don't use this "absolute" centering in any other place, and it looks weird to me.

When hasdownloads == false, "Show All Downlaods" string aligns center in its button self's width.
When hasdownloads == true, the string aligns center in DownloadsPanle's width.

This design is confirmed with visual designer and that's why "padding-left: 58px !important;" is here.
Could you give a suggestion that I am not sure "padding-left: 58px !important;" is a good solution?

> We defined the exact styles and dimensions of the dropmarker this week in bug 1267604:
> 
> https://reviewboard.mozilla.org/r/61060/diff/15#index_header
> 
> See the "popup-notification-dropmarker" additional class and the "menubutton-dropmarker-white.svg" file.
> 
> Please copy the SVG file from the other patch, call it "menubutton-dropmarker.svg" in the same directory, and remove the "fill" attribute from inside the SVG file.
> 
> Here, copy the relevant rules and selectors from the CSS file in the other patch (they're the correct ones), but add the "filter" property and try to set "fill: currentColor;".
> 
> The latter should make the dropmarker appear using the same color as the button text, which is required to support high contrast themes.

Does the actual color of "currentColor" change in different platform? The only concern from visual designer is that the same style should be crossing platform.

> I'm not sure if flex is required here, can you check?

flex="1" makes #downloadsDropmarker as right alignment here. The right alignment won't work if this line is removed.

> Having flex="1" and a "max-width" is not the right solution. As you'll see in the styling for the popup notification, we can just set a fixed width or padding in the CSS.

a fixed width or padding (left and right both given 21px to satisfy width 58px) doesn't work to make #downloadsDropmarker as right alignment. Probably I use them in the wrong way. Could you describe the solution more specific?

> Use "padding: 0 20px;" since we have the height already set. Note that we might change the height of the footer soon, but for now this should be fine.

Based on the visual spec, the top and bottom padding should be both 16px, but the style won't be satisfied when applying "padding: 0 20px;". Is there any solution here?

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Hey Paolo, There are some comments in mozreview. could you help to review the patch again? Thank you!

        Attachment #8767923 -
        Flags: review?(paolo.mozmail)

    
    

    
      
  

    
  

        Attachment #8767923 -
        Flags: review?(paolo.mozmail)

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

https://reviewboard.mozilla.org/r/62304/#review70166

Thanks Sean for the thorough update of the patch. This is a really good implementation and it's almost ready to land!

Please start a tryserver build after updating the tests, and verify that it succeeds.

I've described the changes you need to make to the footer in more detail, as you asked. I've tested these locally on OS X. After these final changes, please test the patch again on Windows, Mac and Linux, ensuring it's applied on the latest revision of mozilla-central, and adjust anything that's needed.

Screenshots of the result are appreciated. I'll test and verify the new patch again, and if no changes are needed for other platforms at that point, the patch will land soon!

::: browser/components/downloads/DownloadsCommon.jsm:511
(Diff revision 16)
>          try {
>            // Open the parent directory to show where the file should be.
>            parent.launch();
>          } catch (ex) {
>            // If launch also fails (probably because it's not implemented), let
>            // the OS handler try to open the parent.
>            Cc["@mozilla.org/uriloader/external-protocol-service;1"]
>              .getService(Ci.nsIExternalProtocolService)
>              .loadUrl(NetUtil.newURI(parent));
>          }

Just call "this.showDirectory(parent);" here.

::: browser/components/downloads/DownloadsCommon.jsm:526
(Diff revision 16)
>        }
>      }
>    },
>  
>    /**
> +   * Show the folder of a file in the system file manager.

"Show the specified folder in the system file manager."

::: browser/components/downloads/DownloadsCommon.jsm:535
(Diff revision 16)
> +    if (!aDirectory.isDirectory()) {
> +      throw new Error("aDirectory must be a directory");
> +    }

Since isDirectory causes unnecessary I/O, and is only used for internal verification, we should remove this check. Thanks for the thought anyways ;-)

::: browser/components/downloads/DownloadsCommon.jsm:539
(Diff revision 16)
> +    }
> +    if (!aDirectory.isDirectory()) {
> +      throw new Error("aDirectory must be a directory");
> +    }
> +    try {
> +      // Open the parent directory to show where the file should be.

Remove this comment, it's outdated and a new comment would be unnecessary.

::: browser/components/downloads/DownloadsCommon.jsm:542
(Diff revision 16)
> +    }
> +    try {
> +      // Open the parent directory to show where the file should be.
> +      aDirectory.launch();
> +    } catch (ex) {
> +      // If launch also fails (probably because it's not implemented), let

Remove the word "also".

::: browser/components/downloads/content/downloads.js:371
(Diff revision 16)
> +    document.getElementById("downloadsDropdownItemClearList").setAttribute(
> +      "hidden", !DownloadsCommon.getData(window).canRemoveFinished);

This should call setAttribute("hidden", "true") or removeAttribute as needed.

::: browser/components/downloads/content/downloadsOverlay.xul:163
(Diff revision 16)
> +              <toolbarseparator id="downloadsFooterButtonsSplitter"
> +                      class="downloadsDropmarkerSplitter"/>
> +              <button id="downloadsFooterDropmarker"
> +                      class="plain downloadsPanelFooterButton downloadsDropmarker"
> +                      type="menu"
> +                      flex="1">

Remove this flex atttribute on the dropmarker, but keep the one on the text button.

::: browser/components/downloads/content/downloadsOverlay.xul:165
(Diff revision 16)
> +              <button id="downloadsFooterDropmarker"
> +                      class="plain downloadsPanelFooterButton downloadsDropmarker"
> +                      type="menu"
> +                      flex="1">
> +                <menupopup id="downloadSubPanel"
> +                           onpopupshown="DownloadsPanel.onFooterPopupShowing(event);"

This shuld be "onpopupshowing", because we must change the visibility of the items before the popup menu is shown.

::: browser/components/downloads/test/browser/browser_downloads_panel_footer.js:6
(Diff revision 16)
> +  let downloadsDropmarker = document.getElementById("downloadsFooterDropmarker");
> +  EventUtils.synthesizeMouseAtCenter(downloadsDropmarker, {}, window);
> +
> +  let downloadSubPanel = document.getElementById("downloadSubPanel");
> +  yield BrowserTestUtils.waitForEvent(downloadSubPanel, "popupshown");

To avoid race conditions, you should do:

let popupShownPromise = BrowserTestUtils.waitForEvent(...);
// ...code that shows the popup...
yield popupShownPromise;

The same should be done in all the other cases where you wait for the result of an action - most of the checks you do should change in this way.

::: browser/components/downloads/test/browser/browser_downloads_panel_footer.js:14
(Diff revision 16)
> +      Downloads.getPreferredDownloadsDirectory().then(downloadsPath => {
> +        is(file.path, downloadsPath, "Check the download folder path.");
> +        resolve();
> +      });

Doing "resolve(functionCall.then(...))" will propagate the errors properly, while "functionCall.then(...;resolve())" will not.

In general, watch out for cases where the return value of "then" calls is ignored.

::: browser/components/downloads/test/browser/head.js
(Diff revision 16)
> -
>  function promiseFocus()

Remove this whitespace change, it's a leftover from removing the function.

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd:65
(Diff revision 16)
>  <!ENTITY cmd.clearList.label              "Clear List">
>  <!ENTITY cmd.clearList.accesskey          "a">

The old string can be removed from the file, just verify all the uses have been converted to clearList2 as needed.

::: browser/themes/shared/downloads/downloads.inc.css:33
(Diff revision 16)
> -  padding: 10px 20px;
> +  height: 40px;
> +  padding: 16px 20px;

Because the total height is the sum of the text height and the padding, you should decide if you want to have a fixed height and variable padding, or a fixed padding and variable height.

Based on your previous comment, you should remove the "height" property instead of the vertical padding as I previously suggested.

::: browser/themes/shared/downloads/downloads.inc.css:35
(Diff revision 16)
> +  margin-top: 0;
> +  margin-bottom: 0;

Do you really need to reset these two margins?

::: browser/themes/shared/downloads/downloads.inc.css:61
(Diff revision 16)
>    -moz-appearance: none;
>    background-color: transparent;
>    color: inherit;
>    margin: 0;
>    padding: 0;
>    min-height: 40px;

Add "min-width: 0;" before the min-height rule. This allows all footer buttons to shrink as needed, including the dropmarker.

::: browser/themes/shared/downloads/downloads.inc.css:85
(Diff revision 16)
>  .downloadsPanelFooterButton[default]:hover:active {
>    background-color: #0568ba;
>  }

Please move the other rules for the footer buttons right after this rule, so they're in the right section marked by the overall header comments, near other rules that control only the footer. In the future all of these rules should possibly be reordered to match the XUL order more closely, but it's better to keep things together than separated for now.

::: browser/themes/shared/downloads/downloads.inc.css:227
(Diff revision 16)
> +#downloadsPanel[hasdownloads] #downloadsHistory {
> +  padding-left: 58px !important;
> +}

The aligment looks weird to me, because the other text in the download items is not centered in the same way, but since you've confirmed this with the visual designer, this is indeed the correct way to implement it.

Just change the value to 47px for now, to match the changes below.

::: browser/themes/shared/downloads/downloads.inc.css:241
(Diff revision 16)
> +#downloadsFooter toolbarseparator.downloadsDropmarkerSplitterExtend {
> +  margin: 0;
> +}
> +
> +.downloadsDropmarker {
> +  padding: 0 15px;

Add "!important" so it's really applied.

::: browser/themes/shared/downloads/downloads.inc.css:257
(Diff revision 16)
> +}
> +
> +.downloadsDropmarker > .button-box > .button-menu-dropmarker > .dropmarker-icon {
> +  width: 16px;
> +  height: 16px;
> +  list-style-image: url("chrome://browser/skin/downloads/menubutton-dropmarker.svg");

Add the properties:

filter: url("chrome://browser/skin/filters.svg#fill");
fill: currentColor;

The color will indeed be the same color on all platforms by default, but will change to match the button text with non-default themes, including high contrast themes.

::: browser/themes/shared/downloads/downloads.inc.css:266
(Diff revision 16)
> +#downloadsFooterButtons .downloadsDropmarker {
> +  min-width: 0;
> +  max-width: 58px;
> +}

Remove this rule entirely. The button width will be automatically defined as the sum of the icon width and the padding you copied from the popup notifications.

This gives us consistency with the other panels.

We may want to make this button wider in the near future, to match the visual design spec more closely, but we will do it when we work on the panel items themselves. This is because the purpose of the spec is to align the item icons with the dropdown icon, and currently they don't align anyways if you just follow the spec in the footer and not in the items.

        Attachment #8767923 -
        Flags: review?(paolo.mozmail)

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

https://reviewboard.mozilla.org/r/62304/#review70208

::: browser/components/downloads/content/downloadsOverlay.xul:166
(Diff revision 16)
> +                      class="plain downloadsPanelFooterButton downloadsDropmarker"
> +                      type="menu"
> +                      flex="1">
> +                <menupopup id="downloadSubPanel"
> +                           onpopupshown="DownloadsPanel.onFooterPopupShowing(event);"
> +                           onpopuphidden="DownloadsPanel.onFooterPopupHiding(event);"

Ah, just noticed an additional small thing. While "onpopuphidden" is the correct handler for when the menu goes away, the called function should be named "onFooterPopupHidden" to match.

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

https://reviewboard.mozilla.org/r/62304/#review70166

> The old string can be removed from the file, just verify all the uses have been converted to clearList2 as needed.

After checking the [search result](http://searchfox.org/mozilla-central/search?q=cmd.clearList&case=false&regexp=false&path=), the changes of browser/locales/.../downloads.dtd will affect to browser/components/downloads/content/downloadsOverlay.xul, so I think downloadsOverlay.xul is the only file has to be changed.

I am wondering if [toolkit/.../downloads.dtd](http://searchfox.org/mozilla-central/source/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd#42) used by [downloads.xul](http://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/content/downloads.xul#155) should be changed as well.

Could you give me a suggestion?

> Because the total height is the sum of the text height and the padding, you should decide if you want to have a fixed height and variable padding, or a fixed padding and variable height.
> 
> Based on your previous comment, you should remove the "height" property instead of the vertical padding as I previously suggested.

The visual designer and I decide to remove "height: 40px;" and keep padding-top/bottom 16px.

> Do you really need to reset these two margins?

Since there are margin-top and margin-bottom defined [1]. IMHO, it would be more consistent with the spec that css rule is padding-top/bottom 16px and margin-top/bottom 0 here.

[1] http://searchfox.org/mozilla-central/source/toolkit/themes/osx/global/global.css#188,195

> The aligment looks weird to me, because the other text in the download items is not centered in the same way, but since you've confirmed this with the visual designer, this is indeed the correct way to implement it.
> 
> Just change the value to 47px for now, to match the changes below.

I suppose 58px should be remained here. The width of dropmarker is 58px in spec.

> Add "!important" so it's really applied.

Use "padding: 0 19px;" to make the width of .downloadsDropmarker to 58px.

    
    

    
      
  

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

https://reviewboard.mozilla.org/r/62304/#review70588

::: browser/components/downloads/content/downloads.js:1234
(Diff revision 17)
>        ];
>        return blockedSubviewCmds.indexOf(aCommand) >= 0;
>      }
>      // If the blocked subview is not showing, then determine if focus is on a
>      // control in the downloads list.
>      let element = document.commandDispatcher.focusedElement;

On MacOSX, element will be richlistbox#downloadsListBox, and supportsCommand returns true for executing "downloadsCmd_clearList".

On Windows/Linux, element will be button#downloadsFooterDropmarker, and no ancestors will be richlistbox.

It's weird that focusedElement indicates to richlistbox#downloadsListBox directly rather than button#downloadsFooterDropmarker.

Even though downloadsCmd_clearList can be executed in MacOSX, but I think button#downloadsFooterDropmarker case makes more sense to me.

Could you give a suggestion here?

::: browser/components/downloads/content/downloadsOverlay.xul:168
(Diff revision 17)
> +                <menupopup id="downloadSubPanel"
> +                           onpopupshowing="DownloadsPanel.onFooterPopupShowing(event);"
> +                           onpopuphidden="DownloadsPanel.onFooterPopupHidden(event);"
> +                           position="after_end">
> +                  <menuitem id="downloadsDropdownItemClearList"
> +                            command="downloadsCmd_clearList"

While testing multi-platform, this line only works for MacOSX but Linux/Windows.

If change this line to a workaround solution:
oncommand="DownloadsCommon.getData(window).removeFinished();

it works for Linux/Windows/MacOSX.

I investigate the function of DownloadsViewController.supportsCommand .
Please see more detail there.

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

Hey Paolo,

Thanks for your review! I do learn a lot from it. The suggestions are almost done, could you see comment 76 for my questions?

Also, I found there are some issue about "downloadsCmd_clearList" on windows/linux, could you see comment 78?

Thank you!

        Attachment #8767923 -
        Flags: review?(paolo.mozmail)

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

https://reviewboard.mozilla.org/r/62304/#review70588

> On MacOSX, element will be richlistbox#downloadsListBox, and supportsCommand returns true for executing "downloadsCmd_clearList".
> 
> On Windows/Linux, element will be button#downloadsFooterDropmarker, and no ancestors will be richlistbox.
> 
> It's weird that focusedElement indicates to richlistbox#downloadsListBox directly rather than button#downloadsFooterDropmarker.
> 
> Even though downloadsCmd_clearList can be executed in MacOSX, but I think button#downloadsFooterDropmarker case makes more sense to me.
> 
> Could you give a suggestion here?

Since we can now invoke "downloadsCmd_clearList" from outside the list, we should just update "supportsCommand" so that it always returns true for it. It's important to check the focused element for commands that start with "cmd_", like cut and paste, but for commands that start with "downloadsCmd_" we don't need to do that because they can only be invoked from the Downloads Panel.

Because "downloadsCmd_clearList" is the only global command that starts with "downloadsCmd_", we can just add these lines to the beginning of the supportsCommand function:

    if (aCommand == "downloadsCmd_clearList") {
      return true;
    }

I tested this on Windows and Linux and it seems to work.

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

https://reviewboard.mozilla.org/r/62304/#review70166

> After checking the [search result](http://searchfox.org/mozilla-central/search?q=cmd.clearList&case=false&regexp=false&path=), the changes of browser/locales/.../downloads.dtd will affect to browser/components/downloads/content/downloadsOverlay.xul, so I think downloadsOverlay.xul is the only file has to be changed.
> 
> I am wondering if [toolkit/.../downloads.dtd](http://searchfox.org/mozilla-central/source/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd#42) used by [downloads.xul](http://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/content/downloads.xul#155) should be changed as well.
> 
> Could you give me a suggestion?

The code you noticed under "toolkit" is old code that will be removed, I confirm it shouldn't be changed.

> Since there are margin-top and margin-bottom defined [1]. IMHO, it would be more consistent with the spec that css rule is padding-top/bottom 16px and margin-top/bottom 0 here.
> 
> [1] http://searchfox.org/mozilla-central/source/toolkit/themes/osx/global/global.css#188,195

Sounds good, you can use "margin: 0;" though.

> I suppose 58px should be remained here. The width of dropmarker is 58px in spec.

I've already explained that I think we shouldn't follow the spec literally in this case, because we still haven't implemented the rest of it for the download items, but I don't think the choice of this value should block the patch from landing, so I'm fine with either.

    
    

    
  
Comment on attachment 8767923 [details]
Bug 1269962 - Implement a popup menu for showing a submenu in Downloads Panel Footer.,

https://reviewboard.mozilla.org/r/62304/#review70646

After the change in comment 80 and the optional minor changes, this is ready to land! Please rebase on the latest mozilla-central, add "r=paolo" to the commit message, then start a new tryserver build and post a link to Treeherder on the bug.

        Attachment #8767923 -
        Flags: review?(paolo.mozmail) → review+

    
    

    
      
  

    
    

    
  
Hey Paolo, Thank you very much for reviewing this patch, and I learned a lot from your suggestions!
BTW, may I know if you still need the screenshots for each platform?

    
    

    
  
Hey Bryant, could you provide your thoughts from UX perspective if this patch can be landed separately without the implementation bug 1269958? Thank you.
Flags: needinfo?(bmao)

    
    

    
  
(In reply to Sean Lee [:seanlee][:weilonge] from comment #85)
> Hey Paolo, Thank you very much for reviewing this patch, and I learned a lot
> from your suggestions!

Thanks to you for writing it :-)

> BTW, may I know if you still need the screenshots for each platform?

I don't need all platforms as I've also has the opportunity to test locally, but I think at least a pair of screenshots with and without a download item visible will be useful to the visual designers, and also to those who want to see what this bug is about in the future.

    
    

    
  
(In reply to Sean Lee [:seanlee][:weilonge] from comment #86)
> Hey Bryant, could you provide your thoughts from UX perspective if this
> patch can be landed separately without the implementation bug 1269958? Thank
> you.

I strongly recommend landing this patch now. Especially for complex patches, if the code stays around for a long time without landing, it tends to become outdated (a process sometimes defined as "bitrot") and will require a significant amount of work to be integrated later.

Themes are quite an active area these days, so this is likely to impact this patch more.

    
    

    
  
Hey Paolo,

There are two perspectives relative to code landing decision:
A. UX/Visual perspective - this can be answered by UX/Visual team.
B. Preference Flag - After discussing with RM and QA, they strongly recommend that we should have a preference flag for this patch.

This bug and bug 1269958 are both the MVP of Summary Section refine in Downloads Panel, but I have no confidence the patch of bug 1269958 can be ready to land before Firefox 51 (2016-09-12).

So my idea is:
Step 1. Land the patch of bug 1269962 (for this bug) as soon as possible to prevent the concern in comment 88.
Step 2. Implement the preference patch (default ON) for Summary Section refine, and the patch MUST be landed before Firefox 51 (2016-09-12)

If we apply item B, this patch still can be landed even there are some UX concerns with pref-off.

May I know your thoughts? Thank you.
Flags: needinfo?(paolo.mozmail)

    
    

    
  
(In reply to Sean Lee [:seanlee][:weilonge] from comment #89)
> So my idea is:
> Step 1. Land the patch of bug 1269962 (for this bug) as soon as possible to
> prevent the concern in comment 88.
> Step 2. Implement the preference patch (default ON) for Summary Section
> refine, and the patch MUST be landed before Firefox 51 (2016-09-12)

Sounds good to me! Thanks for figuring out the way forward.
Flags: needinfo?(paolo.mozmail)

    
  
Keywords: checkin-needed

    
  
Blocks: 1297039

    
    

    
  
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d17dd12093e3
Implement a popup menu for showing a submenu in Downloads Panel Footer., r=Paolo
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/d17dd12093e3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago

          status-firefox51:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

    
    

    
  


  
The attachment includes the screenshot for this patch in three platform (MacOSX/Linux/Windows)

    
    

    
  
(In reply to Sean Lee [:seanlee][:weilonge] from comment #71)
> > This is best answered by the visual design team, but I think the text in the menu button should just be centered in the space it has. As far as I can tell we don't use this "absolute" centering in any other place, and it looks weird to me.
> 
> When hasdownloads == false, "Show All Downlaods" string aligns center in its
> button self's width.
> When hasdownloads == true, the string aligns center in DownloadsPanle's
> width.
> 
> This design is confirmed with visual designer and that's why "padding-left:
> 58px !important;" is here.
> Could you give a suggestion that I am not sure "padding-left: 58px
> !important;" is a good solution?

Ugh... couple of questions come to mind:
1. Does this work with right-to-left locales?
2. Does this work for languages other than English where the string has a different width?
3. Why !important?

(In reply to :Paolo Amadini from comment #64)
> > +#downloadsFooter:hover .rollOverSplitter {
> > +  padding: 0;
> > +  margin: 0;
> > +}
> > +
> > +#downloadsDropmarker {
> 
> You should use the classes "plain downloadsPanelFooterButton
> downloadsDropmaker" in the XUL file. This way you don't have to redefine
> most of the properties and colors.

I thought we had already established that the plain class is not the right way to reset styles since it uses !important. It also doesn't do anything about colors.
Flags: needinfo?(selee)
Flags: needinfo?(paolo.mozmail)

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #94)
> 1. Does this work with right-to-left locales?

Probably not, I missed this in the review. We should use "margin-inline-start".

> 2. Does this work for languages other than English where the string has a
> different width?

I think it should work correctly and wrap if needed, we can test with longer strings.

> 3. Why !important?
> 
> I thought we had already established that the plain class is not the right
> way to reset styles since it uses !important. It also doesn't do anything
> about colors.

This was an existing pattern, but I agree that as a follow-up we should replace all uses of "plain" in the panel with our own styling overrides (and this removes the need for !important above). Sean, can you file a follow-up bug to implement Dão's suggestion and also fix the "margin-inline-start"?
Flags: needinfo?(paolo.mozmail)

    
  
Depends on: 1297657

    
    

    
  
Thanks Dão and Paolo! We can discuss this issues at bug 1297657 .
Flags: needinfo?(selee)

    
    

    
  
Verified the new feature on latest Nightly 51.0a1

Build ID 	20160824030337
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
[bugday-20160824]

    
  
Blocks: 1269958
No longer depends on: 1269958

    
  
Blocks: 1298276

    
  
Status: RESOLVED → VERIFIED

          status-firefox51:
          fixed → verified

    
  
Blocks: 1300947
Flags: needinfo?(bmao)

    
    

    
  
moved to backlog (won't ship) per project discussion.
Whiteboard: [CHE-MVP] → [CHE-BACKLOG]

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: