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

VERIFIED FIXED in Firefox 57

Status

()

P1
trivial
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: bruce.bugz, Assigned: mikedeboer)

Tracking

57 Branch
Firefox 58
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57+ verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Posted image 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.
(Reporter)

Updated

2 years ago
Component: Untriaged → Menus

Updated

2 years ago
Blocks: 1354155
Severity: normal → trivial
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

2 years ago
Blocks: 1354532
No longer blocks: 1354155
Whiteboard: [photon-structure][triage]

Comment 1

2 years ago
This should be fixed in bug 1398349.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1398349
Whiteboard: [photon-structure][triage]
(Assignee)

Comment 2

2 years ago
Not fixed, I'll take this.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Updated

2 years ago
Assignee: nobody → mdeboer
Status: REOPENED → ASSIGNED
Priority: -- → P1
Whiteboard: [photon-structure]
(Assignee)

Updated

2 years ago
Flags: qe-verify+
(Assignee)

Updated

2 years ago
status-firefox57: --- → affected
tracking-firefox57: --- → ?
QA Contact: gwimberly
Whiteboard: [photon-structure] → [reserve-photon-structure]
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1402129
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox58: --- → affected
status-firefox-esr52: --- → unaffected
tracking-firefox57: ? → +

Comment 5

2 years ago
mozreview-review
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.)
Attachment #8911862 - Flags: review?(paolo.mozmail)

Updated

2 years ago
Duplicate of this bug: 1403913
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
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.
Attachment #8911862 - Flags: review?(paolo.mozmail) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 11

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

Comment 12

2 years ago
Pushed by mdeboer@mozilla.com:
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

2 years ago
mozreview-review
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?
(Assignee)

Comment 14

2 years ago
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]))` ?
Flags: needinfo?(paolo.mozmail)

Comment 15

2 years ago
(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.
Flags: needinfo?(paolo.mozmail)

Comment 16

2 years ago
(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!)
Attachment #8913670 - Flags: checkin?(ryanvm)

Comment 18

2 years ago
Pushed by ryanvm@gmail.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/df9b7bac03c2
https://hg.mozilla.org/mozilla-central/rev/93fb7bb278c3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
(Assignee)

Comment 20

2 years ago
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.
Attachment #8911862 - Flags: approval-mozilla-beta?
(Assignee)

Comment 21

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

See comment 20 for the Approval Request Comment.
Attachment #8913670 - Flags: approval-mozilla-beta?
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
Attachment #8911862 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8913670 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
status-firefox58: fixed → verified
You need to log in before you can comment on or make changes to this bug.