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

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P2)

Description

[Chris Peterson [:cpeterson]]

[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

Comment 1

[Marcia Knous [:marcia]]

Tracking 50+ for this downloads panel regression since it is a visible issue.

Comment 2

[Drew Willcoxon :adw]

Attachment: Bug 1280709 - Downloads panel's "Show All Downloads" menu item is no longer clickable below its text label.

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/

Comment 3

[Drew Willcoxon :adw]

(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.

Comment 4

[:Paolo Amadini]

(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 5

[:Paolo Amadini]

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.

Comment 6

[:Paolo Amadini]

And by the way, rs=me on removing the rule with "max-height: 0" if we have a generic fix.

Comment 7

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 8

[:Paolo Amadini]

Drew, do you think you'll be able to investigate the panel shrinking issue?

Comment 9

[Drew Willcoxon :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.

Comment 10

[Drew Willcoxon :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/

Comment 11

[Drew Willcoxon :adw]

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 12

[:Gijs (he/him)]

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.

Comment 13

[Drew Willcoxon :adw]

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.

Comment 14

[Drew Willcoxon :adw]

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 15

[Drew Willcoxon :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/2-3/

Comment 16

[Drew Willcoxon :adw]

That patch polls for a height change.  I don't know what else to do.

Comment 17

[:Gijs (he/him)]

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. :-\

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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.

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

Comment 19

[Drew Willcoxon :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.)

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Okay, the patch fixed the bug for me on an empty downloads panel.

Comment 21

[:Paolo Amadini]

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.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f273e4bdc67

Comment 24

[Sean Lee [:seanlee][:weilonge]]

Attachment: 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.

Comment 25

[Drew Willcoxon :adw]

Could you please file a new bug and mark it blocking this one?

Comment 26

[Sean Lee [:seanlee][:weilonge]]

Hey Drew, please see bug 1292345 .

Comment 27

[Florian Quèze [:florian]]

(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.

Comment 29

[Alexandru Simonca [:asimonca], Release Desktop QA]

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