Closed
Bug 1280709
Opened 8 years ago
Closed 8 years ago
Downloads panel's "Show All Downloads" menu item is no longer clickable below its text label
Categories
(Firefox :: Downloads Panel, defect, P2)
Tracking
()
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)
Comment 1•8 years ago
|
||
Tracking 50+ for this downloads panel regression since it is a visible issue.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
Assignee | ||
Updated•8 years ago
|
Priority: -- → P2
Whiteboard: [fxprivacy]
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
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)
Comment 6•8 years ago
|
||
And by the way, rs=me on removing the rule with "max-height: 0" if we have a generic fix.
Comment 7•8 years ago
|
||
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•8 years ago
|
||
Drew, do you think you'll be able to investigate the panel shrinking issue?
Flags: needinfo?(adw)
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
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)
Assignee | ||
Comment 11•8 years ago
|
||
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•8 years ago
|
||
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)
Assignee | ||
Comment 13•8 years ago
|
||
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.
Assignee | ||
Comment 14•8 years ago
|
||
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.
Assignee | ||
Comment 15•8 years ago
|
||
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)
Assignee | ||
Comment 16•8 years ago
|
||
That patch polls for a height change. I don't know what else to do.
Updated•8 years ago
|
Attachment #8765695 -
Flags: review?(jaws)
Comment 17•8 years ago
|
||
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+
Comment 18•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/67292/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67292/
Attachment #8774912 -
Flags: review?(adw)
Assignee | ||
Updated•8 years ago
|
Attachment #8774912 -
Flags: review?(adw)
Assignee | ||
Comment 19•8 years ago
|
||
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•8 years ago
|
||
Okay, the patch fixed the bug for me on an empty downloads panel.
Comment 21•8 years ago
|
||
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•8 years ago
|
||
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
Comment 23•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•8 years ago
|
Attachment #8765695 -
Flags: review?(jaws)
Comment 24•8 years ago
|
||
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)
Assignee | ||
Comment 25•8 years ago
|
||
Could you please file a new bug and mark it blocking this one?
Flags: needinfo?(adw) → needinfo?(selee)
Updated•8 years ago
|
Comment 27•8 years ago
|
||
(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
Updated•8 years ago
|
Iteration: --- → 50.4 - Aug 1
Updated•8 years ago
|
Flags: qe-verify+
Comment 29•8 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•