Closed Bug 1280709 Opened 4 years ago Closed 4 years ago

Downloads panel's "Show All Downloads" menu item is no longer clickable below its text label

Categories

(Firefox :: Downloads Panel, defect, P2)

Unspecified
macOS
defect

Tracking

()

VERIFIED FIXED
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Status
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 + verified

People

(Reporter: cpeterson, Assigned: adw)

References

Details

(Keywords: regression, Whiteboard: [fxprivacy])

Attachments

(3 files)

[Tracking Requested - why for this release]:

Drew, this bug is a regression in Nightly 50 from Downloads panel bug 1252509.

STR:
1. Open Downloads panel from the toolbar.
2. Mouse over the "Show All Downloads" menu item.

RESULT:
In Aurora 49, the menu item's "hot spot" for the mouse pointer extended to the bottom of the panel's menu item, but in Nightly 50, the hot spot is no longer active on the bottom half of the panel (below the "Show All Downloads" text").


I bisected this regression to this pushlog, which includes Downloads panel bug 1252509:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=52679ce4756c53fd88054a55da482291c26ef8db&tochange=9694e371363590c8dace6629dc4d57f1af7206f2
Flags: needinfo?(adw)
Tracking 50+ for this downloads panel regression since it is a visible issue.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
Priority: -- → P2
Whiteboard: [fxprivacy]
The problem is the max-height: 0 in downloads.css.  The reason that's there is so that the panel properly shrinks when downloads are removed from it.  The PanelUI stuff prevents that from happening otherwise.  But, that interferes with the click target of the Show All Downloads button.  This patch is the best I could come up with after trying different things.

Review commit: https://reviewboard.mozilla.org/r/60950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60950/
Attachment #8765695 - Flags: review?(paolo.mozmail)
(In reply to Drew Willcoxon :adw from comment #2)
> The reason that's there

Contrary to the code comment.  Or maybe that was also true at one point, I don't remember.
(In reply to Drew Willcoxon :adw from comment #2)
> The reason that's there
> is so that the panel properly shrinks when downloads are removed from it. 
> The PanelUI stuff prevents that from happening otherwise.

Actually, the panel not shrinking after removing elements is a problem that Ricky Chien has been experiencing in bug 1203292 as well. That sounds suspiciously related. Would you be able to investigate this, and maybe if they are indeed related, get it fixed generically, here or in a separate bug? That would help Ricky with his bug as well.
Comment on attachment 8765695 [details]
Bug 1280709 - Downloads panel's "Show All Downloads" menu item is no longer clickable below its text label.

Clearing the request in the meantime.
Attachment #8765695 - Flags: review?(paolo.mozmail)
And by the way, rs=me on removing the rule with "max-height: 0" if we have a generic fix.
For the issue of panel unable to be shrunk after removing element. Here is a little information on https://bugzilla.mozilla.org/show_bug.cgi?id=1203292#c59.
Blocks: 1203292
Drew, do you think you'll be able to investigate the panel shrinking issue?
Flags: needinfo?(adw)
I'll try to look at it this week.  I think it'd get fixed way faster if someone who knows PanelUI looks at it though.
Flags: needinfo?(adw)
Comment on attachment 8765695 [details]
Bug 1280709 - Downloads panel's "Show All Downloads" menu item is no longer clickable below its text label.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60950/diff/1-2/
Attachment #8765695 - Flags: review?(gijskruitbosch+bugs)
I'm not really sure what to do about this.  From panelui's perspective, it's not doing anything wrong, I think you could say.  In this bug's case, the panel has a non-flex listbox with a flex=1 spacer after it.  When a list item is removed, the listbox shrinks and the spacer expands.  panelui doesn't "know" that I want the panel to shrink instead of the spacer expanding.

(The reason the spacer is there is the subview: it can be taller than the main view, and when it slides in, the "show all downloads" button at the bottom of the main view (below the spacer) should stick to the bottom of the panel.)

So what this patch does is add a method to panelmultiview called setHeightToFit.  (I wanted to call it sizeToFit but that's not right since only the height will change.)  What that method does is (almost) exactly what the earlier patch did: set the max-height on the multiview to 0, wait until the height is actually updated, and then remove the max-height.

Gijs, I think you worked on panelui -- what do you think?
Comment on attachment 8765695 [details]
Bug 1280709 - Downloads panel's "Show All Downloads" menu item is no longer clickable below its text label.

https://reviewboard.mozilla.org/r/60950/#review63530

::: browser/components/customizableui/content/panelUI.xml:408
(Diff revision 2)
> +          // updated, and then remove it.  If it's not removed, weird things can
> +          // happen, like widgets in the panel won't respond to clicks even
> +          // though they're visible.
> +          this.style.maxHeight = "0";
> +          setTimeout(() => {
> +            this.style.removeProperty("max-height");

This is assuming that a reflow will happen inbetween this running to completion and the timeout firing, right? Can we instead force a reflow to happen by reading some style data, and then remove the max-height after that? Feels like that would be less race-y.
Attachment #8765695 - Flags: review?(gijskruitbosch+bugs)
I'm having a hard time making a reflow happen.  Or at least making a reflow recognize the first maxHeight set.  It's as if both maxHeight sets are batched and deferred until a later turn of the event loop, and then they both happen in the same turn, canceling each other out.
this.style.maxHeight = "0";
this.getBoundingClientRect();
getComputedStyle(this);
this.scrollWidth;
this.scrollHeight;
this.clientHeight;
// ... lots of other attempts...
this.style.removeProperty("max-height");

And nothing happens.
Comment on attachment 8765695 [details]
Bug 1280709 - Downloads panel's "Show All Downloads" menu item is no longer clickable below its text label.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60950/diff/2-3/
Attachment #8765695 - Flags: review?(gijskruitbosch+bugs)
That patch polls for a height change.  I don't know what else to do.
Attachment #8765695 - Flags: review?(jaws)
Comment on attachment 8765695 [details]
Bug 1280709 - Downloads panel's "Show All Downloads" menu item is no longer clickable below its text label.

https://reviewboard.mozilla.org/r/60950/#review63758

Very reluctant r+. I'd like Jared to share thoughts if he thinks there's anything better we can do. :-\
Attachment #8765695 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8774912 - Flags: review?(adw)
Comment on attachment 8774912 [details]
Bug 1280709 - Set the max-height to none instead of 0 to properly override the max-height set elsewhere. Setting max-height to 0 means 'you get no height' which is not what was wanted here.

https://reviewboard.mozilla.org/r/67292/#review64318

Thanks Jared, but this doesn't actually seem to work.  The comment here is wrong: what this max-height:0 is really doing is shrinking the panel after you remove a download from the panel.  STR:

1. Go to http://testsafebrowsing.appspot.com/
2. Click #2 under "Desktop Download Warnings"
3. Open the downloads panel
4. Right-click the malicious download and choose remove from history

The panel's height should shrink but it doesn't.

Any other ideas?

(I copied that comment right from the control center CSS because I thought it was necessary to do what it says, but it doesn't seem like it.  But it ended up making the panel shrink when removing a download.  Anyway, the comment should be updated if that CSS chunk isn't removed.)
Okay, the patch fixed the bug for me on an empty downloads panel.
I tested the setHeightToFit method for the Control Center in bug 1203292, and it works, although there is a visible flicker when it is called. Given there doesn't seem to be a better method and we'd like to land bug 1203292 before the merge, I'll land these together now.
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/fx-team/rev/5f273e4bdc67
Downloads panel's "Show All Downloads" menu item is no longer clickable below its text label. r=gijs
https://hg.mozilla.org/mozilla-central/rev/5f273e4bdc67
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Attached image white_area.png
After the items in Summary section are downloaded, the panel panelmultiview#downloadsPanel-multiView did not shrink to the fit height.

Have this issue been opened and relative to this patch?

Thank you.
Flags: needinfo?(adw)
Could you please file a new bug and mark it blocking this one?
Flags: needinfo?(adw) → needinfo?(selee)
No longer blocks: 1292345
Depends on: 1292345
Hey Drew, please see bug 1292345 .
Flags: needinfo?(selee)
(In reply to :Paolo Amadini from comment #21)
> I tested the setHeightToFit method for the Control Center in bug 1203292,
> and it works, although there is a visible flicker when it is called.

I filed bug 1294435 for the flickering.
Depends on: 1294435
Duplicate of this bug: 1287003
Iteration: --- → 50.4 - Aug 1
Depends on: 1300556
Flags: qe-verify+
This issue is VERIFIED FIXED on the following OS's using Firefox Beta 50.0b3 (id: 20160929120120):

- Windows 10 x64
- Ubuntu 16.04 LTS x64
- Mac OS X 10.11
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.