Change the hover behavior in summary section with downloading file

VERIFIED FIXED in Firefox 51

Status

()

Firefox
Downloads Panel
VERIFIED FIXED
9 months ago
3 months ago

People

(Reporter: seanlee, Assigned: seanlee)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified, firefox52 verified, firefox53 verified, firefox54 verified)

Details

(Whiteboard: [CHE-MVP])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

9 months ago
(In reply to :Paolo Amadini from comment #23)
> Comment on attachment 8782840 [details]
> Bug 1269958 - Change the information details of the summary section in
> Downloads Panel.
> 
> (In reply to Sean Lee [:seanlee][:weilonge] from comment #22)
> > The patch includes the following changes:
> > * The hover behavior in Summary Section when [showingsummary] is true.
> 
> I haven't looked specifically at the styles now, but I think this part will
> be straightforward. You can even consider moving it to its own small bug to
> land sooner.
> 

Based on the above discussion (bug 1269958 comment 23), this bug focuses on the hover behavior in summary section
Comment hidden (mozreview-request)
(Assignee)

Comment 2

9 months ago
Comment on attachment 8785168 [details]
Bug 1298276 - Change the hover behavior in summary section with downloading files.,

Hey Paolo, Could you help to review this patch? Thank you!
Attachment #8785168 - Flags: review?(paolo.mozmail)

Comment 3

9 months ago
mozreview-review
Comment on attachment 8785168 [details]
Bug 1298276 - Change the hover behavior in summary section with downloading files.,

https://reviewboard.mozilla.org/r/74474/#review72364

I tested this locally, the effect is a bit jarring but it's probably just a matter of getting accustomed to it.

Part of it is a glitch for which, when the mouse is exactly over the one pixel top border of the footer, the main button is not highlighted, so if you move your mouse from top to bottom you get a flash of a lighter color.

You can solve this by putting the two <hbox> in a <stack> and check ":hover" on it instead. Note that, since you're adding a new element, you may have to check all the CSS files to see if the existing child selectors should be updated to descendant selectors.

::: browser/components/downloads/content/downloads.css:27
(Diff revision 1)
> -#downloadsFooter[showingsummary] > #downloadsFooterButtons,
> +#downloadsFooter[showingsummary]:not(:hover) > #downloadsFooterButtons:not(.subPanelShown),
> +#downloadsFooter[showingsummary]:hover > #downloadsSummary,
> +#downloadsFooter > #downloadsSummary.subPanelShown,

The \[showingsummary\] attribute should control the "display: none;" property, while for hover and \[showingsubpanel\] you can control "visibility: hidden;" instead.

::: browser/components/downloads/content/downloads.js:386
(Diff revision 1)
>      document.getElementById("downloadsFooterButtonsSplitter").classList
>        .add("downloadsDropmarkerSplitterExtend");
> +
> +    document.getElementById("downloadsSummary").classList
> +      .add("subPanelShown");
> +
> +    document.getElementById("downloadsFooterButtons").classList
> +      .add("subPanelShown");

It doesn't seem necessary to have three different classes. We can refactor this to use a single attribute. Just like we have "#downloadsFooter[showingsummary]", we can replace the three of these with "#downloadsFooter[showingdropdown]" which follows the same convention.

Note that I thought about calling this attribute "showingsubpanel" but it I realized only now that it can be confused with the lateral "subview".

::: browser/themes/shared/downloads/downloads.inc.css:254
(Diff revision 1)
> +#downloadsFooter[showingsummary] > #downloadsFooterButtons {
> +  height: var(--downloads-item-height);
> +}
> +

Using "visibility: hidden;" together with <stack> allows the space reserved for the summary to determine the height of the buttons, so you can remove this rule.
Attachment #8785168 - Flags: review?(paolo.mozmail)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

9 months ago
mozreview-review-reply
Comment on attachment 8785168 [details]
Bug 1298276 - Change the hover behavior in summary section with downloading files.,

https://reviewboard.mozilla.org/r/74474/#review72364

For the glitch, "not highlighted" issue is not fixed in this patch, because I can not figure out the solution. Could you describe the solution specifically?
Besides, the glitch issue can be reproduced as well when mouse hovers on the splitter.
(Assignee)

Comment 6

9 months ago
Comment on attachment 8785168 [details]
Bug 1298276 - Change the hover behavior in summary section with downloading files.,

Hey Paolo, could you help to see the patch again? Thank you.
Attachment #8785168 - Flags: review?(paolo.mozmail)

Comment 7

9 months ago
mozreview-review
Comment on attachment 8785168 [details]
Bug 1298276 - Change the hover behavior in summary section with downloading files.,

https://reviewboard.mozilla.org/r/74474/#review72808

::: browser/components/downloads/content/downloads.css:34
(Diff revision 2)
>  }
>  
> +#downloadsFooter[showingdropdown] > stack > #downloadsSummary,
> +#downloadsFooter[showingsummary] > stack:hover > #downloadsSummary,
> +#downloadsFooter[showingsummary]:not([showingdropdown]) > stack:not(:hover) > #downloadsFooterButtons {
> +  visibility: hidden;

So, I've tried the patch and the glitch you're observing is a different one, caused by the fact that the ":hover" state is not applied to element we're making visible the until the mouse is moved again.

I've worked around this using "opacity: 0;" and "-moz-user-focus: ignore;" instead, but if you cannot find a better solution, we should add a comment explaining why we're doing this.

::: browser/components/downloads/content/downloads.js:386
(Diff revision 2)
>      document.getElementById("downloadsFooterButtonsSplitter").classList
>        .add("downloadsDropmarkerSplitterExtend");

Can you reimplement the splitter using the [showingdropdown] attribute instead?

::: browser/components/downloads/content/downloadsOverlay.xul:131
(Diff revision 2)
>                         mousethrough="always">
>               &downloadsPanelEmpty.label;
>            </description>
>            <spacer flex="1"/>
>            <vbox id="downloadsFooter"
> -                class="downloadsPanelFooter">
> +                class="downloadsPanelFooter"><stack>

Please indent <stack> on the next line for the final patch.
Attachment #8785168 - Flags: review?(paolo.mozmail)

Comment 8

9 months ago
(In reply to Sean Lee [:seanlee][:weilonge] from comment #5)
> Besides, the glitch issue can be reproduced as well when mouse hovers on the
> splitter.

This is expected, it's the same behavior as the main menu, and it isn't an issue because it does not cause the same area to change rapidly across three different states.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

9 months ago
mozreview-review-reply
Comment on attachment 8785168 [details]
Bug 1298276 - Change the hover behavior in summary section with downloading files.,

https://reviewboard.mozilla.org/r/74474/#review72808

> So, I've tried the patch and the glitch you're observing is a different one, caused by the fact that the ":hover" state is not applied to element we're making visible the until the mouse is moved again.
> 
> I've worked around this using "opacity: 0;" and "-moz-user-focus: ignore;" instead, but if you cannot find a better solution, we should add a comment explaining why we're doing this.

After replacing "visibility: hidden;" with "opacity: 0;", this glitch issue is solved.
The comment is added for explaining "opacity: 0;" and "-moz-user-focus: ignore;".
I did an expreiment to compare the mouseenter event between "visibility: hidden;" and "opacity: 0;".
mouseenter handlers are added on vbox#downloadsFooter, stack, #downloadsSummary, #downloadsFooterButtons and #downloadsHistory, and I move the cursor as slowly as possible to observe the mouseenter events are triggered in 2(opacity: 0;) or 3(visibility: hidden;) steps.
The glitch I mentioned is caused by the event is triggered on #downloadsSummary in "visibility: hidden;" case, but the event won't in "opacity: 0;" case.

Could you help to check my understanding is correct or not?

Thanks a lot for the solution!
I never realize there is the event triggered difference between them.

=======  visibility: hidden;  =======
    - triggered mouseenter 1
        - vbox#downloadsFooter
    - triggered mouseenter 2
        - stack
        - hbox#downloadsSummary
    - triggered mouseenter 3
        - hbox#downloadsFooterButtons
        - button#downloadsHistory

=======  opacity: 0;  =======
    - triggered mouseenter 1
        - vbox#downloadsFooter
    - triggered mouseenter 2
        - stack
        - hbox#downloadsFooterButtons
        - button#downloadsHistory

BTW, may I know why you said this is a workaround? Is there any potential issue?
Thank you.
(Assignee)

Comment 13

9 months ago
mozreview-review
Comment on attachment 8785168 [details]
Bug 1298276 - Change the hover behavior in summary section with downloading files.,

https://reviewboard.mozilla.org/r/74474/#review73080

::: browser/components/downloads/content/downloads.js:385
(Diff revision 5)
>      if (DownloadsCommon.getData(window).canRemoveFinished) {
>        itemClearList.removeAttribute("hidden");
>      } else {
>        itemClearList.setAttribute("hidden", "true");
>      }
> +    DownloadsViewController.updateCommands();

"downloadsCmd_clearList" commmand should be updated before the panel shown or users will see a disabled menu item.
This only happens in the case that DownloadsViewController.updateCommands(); was executed previously with the determination "canRemoveFinished", but canRemoveFinished is changed when opening footer popup.
(Assignee)

Comment 14

9 months ago
Comment on attachment 8785168 [details]
Bug 1298276 - Change the hover behavior in summary section with downloading files.,

Hey Paolo, could you review it in the second round?
Attachment #8785168 - Flags: review?(paolo.mozmail)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8785168 - Flags: review?(paolo.mozmail)

Comment 16

9 months ago
(In reply to Sean Lee [:seanlee][:weilonge] from comment #12)
> I did an expreiment to compare the mouseenter event between "visibility:
> hidden;" and "opacity: 0;".

Thanks! It's really useful to see the exact reason why it's happening. It makes sense to me that mouse events (and the corresponding hover state) are only checked when the mouse moves, and not necessarily when an element is made visible under the mouse pointer.

> BTW, may I know why you said this is a workaround? Is there any potential issue?

The reason why I call this a workaround is that ideally we would have the correct hover state at all times, but I think the platform code here is designed to avoid recursion when visibility changes. I don't know the exact logic used by the rendering engine here, so as far as I know there may be a better solution, but using opacity is the best I could find.

Comment 17

9 months ago
(In reply to Sean Lee [:seanlee][:weilonge] from comment #13)
> "downloadsCmd_clearList" commmand should be updated before the panel shown
> or users will see a disabled menu item.

Thanks for finding this bug!

Comment 18

9 months ago
mozreview-review
Comment on attachment 8785168 [details]
Bug 1298276 - Change the hover behavior in summary section with downloading files.,

https://reviewboard.mozilla.org/r/74474/#review73154

I just rephrased the comment, you can land the updated patch.

::: browser/components/downloads/content/downloads.css:34
(Diff revision 6)
> +  /* When "opacity: 0;" is applied on #downloadsSummary, the mouseenter event of
> +     #downloadsSummary won't be triggered. That's the solution for removing the
> +     middle hover state that no .downloadsPanelFooterButton are highlighted.
> +
> +     "-moz-user-focus: ignore;" is to prevent the element with "opacity: 0;"
> +     still can be focused with TAB key. */

If we used "visibility: hidden;" then the mouseenter event of #downloadsHistory wouldn't be triggered immediately, and the hover styling of the button would not apply until the mouse is moved again.

"-moz-user-focus: ignore;" prevents the elements with "opacity: 0;" from being focused with the keyboard.
Attachment #8785168 - Flags: review?(paolo.mozmail) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

9 months ago
Comment on attachment 8785168 [details]
Bug 1298276 - Change the hover behavior in summary section with downloading files.,

Hey Paolo, could you give r+ again? I clear it accidentally. :P Thanks.
Attachment #8785168 - Flags: review?(paolo.mozmail)

Updated

9 months ago
Attachment #8785168 - Flags: review?(paolo.mozmail) → review+
(Assignee)

Updated

9 months ago
Keywords: checkin-needed

Comment 22

9 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/6a5af5a4c2ef
Change the hover behavior in summary section with downloading files. r=paolo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6a5af5a4c2ef
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
(Assignee)

Updated

9 months ago
Blocks: 1300947
Hi! Can you please elaborate this issue? What has changed in the hover behavior?

Thank you!
Flags: needinfo?(selee)
(Assignee)

Comment 25

4 months ago
Created attachment 8832176 [details]
Show All Downloads button

Hi Cristian,

When the downloading items are at the position after 5 downloads in Downloads Panel, you will see the summary section (see the blue box in the attachment) turns into a status with progress bar. At this moment, please hover the cursor on the summary section and you will see the summary section turn into a "Show All Downloads" button. Thanks.
Flags: needinfo?(selee)
Hi Christian:

Just to be sure, is the information Sean provided sufficient for you to verify this bug?

Per our Dev progress today[1], this is the only fixed item w/o verification (can refer to column L for the overall verification status). Or I guess you may wait for the rest items we are currently working on (targeting by end-Mar), then verify them all together?

Thanks!

[1] https://docs.google.com/spreadsheets/d/1JaP7naJhaHPgpnNPPTH6RnPKfqCgH9Y0feHNWLVdnoY/edit#gid=1507342727
Flags: needinfo?(cristian.comorasu)
Hi, sorry for the long pause.
I reproduced this issue using 51.0a1, build ID: 20160825030226, on Windows 10 x64.
I can confirm this issue is fixed, I verified Fx 54.0a1, build ID: 20170216030210 and Fx53.0a2, build ID: 20170221004019, Fx52.0b8, build ID: 20170220070057 and Fx 51.0.1, build ID: 20170125094131, on Windows 10 x64, Ubuntu 14.04 LTS and Mac OS X 10.11.

Cheers!
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
status-firefox52: --- → verified
status-firefox53: --- → verified
status-firefox54: --- → verified
Flags: needinfo?(cristian.comorasu)
Many thanks, Cristian!
You need to log in before you can comment on or make changes to this bug.