1398346 - Entry for 'File moved or missing' in Library > Downloads Panel jumps around on hover

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[Bruce]

Attachment: Jumping Panel.gif

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170903140023

Steps to reproduce:

1. Complete a download.
2. Delete / move the downloaded file.
3. Open Library button > Downloads subview and hover over the entry corresponding to the file.


Actual results:

Size of panel changes on hovering this item. See attachment.


Expected results:

It shouldn't.

Comment 1

[:Gijs (he/him)]

This should be fixed in bug 1398349.

Comment 2

[Mike de Boer [:mikedeboer]]

Not fixed, I'll take this.

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1398346 - Hide the 'Open File' status label for failed downloads and/ or downloads where the file went missing for items in the Downloads subview in the Library panel.

Review commit: https://reviewboard.mozilla.org/r/183246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183246/

Comment 5

[:Paolo Amadini]

Comment on attachment 8911862 [details]
Bug 1398346 - Hide the 'Open File' status label for failed downloads and/ or downloads where the file went missing for items in the Downloads subview in the Library panel.

https://reviewboard.mozilla.org/r/183246/#review188726

It's quite difficult to evaluate the correctness of these rules, which was a concern in the original review as well.

The comments are useful in describing what the attributes mean, but for the rest they repeat the rules without describing what process was used to come up with them.

I guess this process starts with the conditions that determine which individual label should be visible, with a priority between states. Then, the rules are inverted so that they specify when the labels should be hidden, which happens when any of the other labels should be visible, so each rule ends up specifying all the conditions except the one relevant to it. Finally, there are some simplifications based on implicit assumptions of dependencies between the attributes.

Can you make this process explicit? Because of how you structured this code to have one rule per final selector, a single comment before all the label rules would probably fit the structure better. (The final two rules are logically separate and their comment looks good to me.)

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8911862 [details]
Bug 1398346 - Hide the 'Open File' status label for failed downloads and/ or downloads where the file went missing for items in the Downloads subview in the Library panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183246/diff/1-2/

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8911862 [details]
Bug 1398346 - Hide the 'Open File' status label for failed downloads and/ or downloads where the file went missing for items in the Downloads subview in the Library panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183246/diff/2-3/

Comment 9

[:Paolo Amadini]

Comment on attachment 8911862 [details]
Bug 1398346 - Hide the 'Open File' status label for failed downloads and/ or downloads where the file went missing for items in the Downloads subview in the Library panel.

https://reviewboard.mozilla.org/r/183246/#review190088

Thanks, this is a lot easier to read! Just some small tweaking required.

::: browser/components/downloads/content/downloads.css:231
(Diff revision 3)
> +
> +/* When a Download is _not_ hovered, display the full status message. */
> +.subviewbutton.download:not(:hover) > .toolbarbutton-text > .status-full,
> +/* When a Download is hovered when the file doesn't exist and cannot be retried,
> +   keep showing the full status message. */
> +.subviewbutton.download:hover:not([exists]):not([retryLabel]) > .toolbarbutton-text > .status-full,

This should be ":not([openLabel][exists])" to be the exact opposite of the conditions below and not leave any case uncovered, correct?

::: browser/components/downloads/content/downloads.css:235
(Diff revision 3)
> +/* When a Download is hovered - its action button explicitly - and the file can
> +   be opened, show the 'Open File' status label. */

The "- its action button explicitly -" in this comment is a leftover.

Suggestion: group the rule variants for "[buttonover]" and ":not([buttonover])" under the same comment. The [retryLabel][buttonover] rule at the end should be moved before this one anyways for better ordering.

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8911862 [details]
Bug 1398346 - Hide the 'Open File' status label for failed downloads and/ or downloads where the file went missing for items in the Downloads subview in the Library panel.

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

Comment 11

[Mike de Boer [:mikedeboer]]

Thanks for the helpful suggestions, Paolo! This looks much more readable now, indeed ;-)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df9b7bac03c2
Hide the 'Open File' status label for failed downloads and/ or downloads where the file went missing for items in the Downloads subview in the Library panel. r=Paolo

Comment 13

[:Paolo Amadini]

Comment on attachment 8911862 [details]
Bug 1398346 - Hide the 'Open File' status label for failed downloads and/ or downloads where the file went missing for items in the Downloads subview in the Library panel.

https://reviewboard.mozilla.org/r/183246/#review190094

::: browser/components/downloads/content/downloads.css:231
(Diff revisions 3 - 4)
>  
>  /* When a Download is _not_ hovered, display the full status message. */
>  .subviewbutton.download:not(:hover) > .toolbarbutton-text > .status-full,
>  /* When a Download is hovered when the file doesn't exist and cannot be retried,
>     keep showing the full status message. */
> -.subviewbutton.download:hover:not([exists]):not([retryLabel]) > .toolbarbutton-text > .status-full,
> +.subviewbutton.download:hover:not([openLabel]):not([exists]):not([retryLabel]) > .toolbarbutton-text > .status-full,

Wait, won't this ":not([openLabel]):not([exists])" miss the case where either but not both are set?

Comment 14

[Mike de Boer [:mikedeboer]]

AFAIK, [exists] and [openLabel] always come in pairs, so we should be fine... but I can push a follow-up that changes this to `:not(-moz-any([openLabel],[exists]))` ?

Comment 15

[:Paolo Amadini]

(In reply to Mike de Boer [:mikedeboer] from comment #14)
> AFAIK, [exists] and [openLabel] always come in pairs, so we should be
> fine... but I can push a follow-up that changes this to
> `:not(-moz-any([openLabel],[exists]))` ?

I think we should push this follow-up. It may be true that they are normally set together, but as far as I can tell the code doesn't guarantee this, so we may have edge cases where they're not. Otherwise, we could likely just have dropped the [exists] check and specified in a comment that [openLabel] implied it.

Comment 16

[:Paolo Amadini]

(By the way, I remember the interaction between :not, :moz-any, and concatenation was not obvious, so please check that the syntax we use does indeed have the intended behavior!)

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Follow-up patch to correct one selector slightly

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93fb7bb278c3
Follow up patch to correct one of the selectors to be more precise and avoid potential issues. rs=Paolo

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/df9b7bac03c2
https://hg.mozilla.org/mozilla-central/rev/93fb7bb278c3

Comment 20

[Mike de Boer [:mikedeboer]]

Comment on attachment 8911862 [details]
Bug 1398346 - Hide the 'Open File' status label for failed downloads and/ or downloads where the file went missing for items in the Downloads subview in the Library panel.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1354532.
[User impact if declined]: Users will see specific panel buttons jump up and down while they hover them, which is visually jarring.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, STR in comment 0.
[List of other uplifts needed for the feature/fix]: There are two changesets that have landed for this bug to be fixed, please take them both to beta.
[Is the change risky?]: no.
[Why is the change risky/not risky?]: Because it's a visual change and changes CSS selectors that were quite wrong in the original feature.
[String changes made/needed]: n/a.

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8913670 [details] [diff] [review]
Follow-up patch to correct one selector slightly

See comment 20 for the Approval Request Comment.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8911862 [details]
Bug 1398346 - Hide the 'Open File' status label for failed downloads and/ or downloads where the file went missing for items in the Downloads subview in the Library panel.

Polish Photon + new regression, taking it.
Should be in 57b5

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d932d07adaf0
https://hg.mozilla.org/releases/mozilla-beta/rev/8e780926710a

Comment 24

[Vlad Bacia-Mociran [:VladB]]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171002220204

I can reproduce this issue with Firefox Nightly 57.0a1, Build ID: 20170908220146 on Windows 8.1 x64.

This issue has been verified on latest Firefox Nightly Build ID: 20171002220204 and Firefox Beta 57.0b4 (Build ID: 20170928180207) on Windows 8.1 x64, Mac OS 10.12 and Ubuntu 14.04 and it's not reproducible. Now, when hovering on entries from "Library > Downloads Panel" that were removed or moved from the download location, the panel is fixed and does not jump.