Closed Bug 1292345 Opened 3 years ago Closed 3 years ago

Downloads panel didn't shrink to the fit height after all items are downloaded.

Categories

(Firefox :: Downloads Panel, defect, P1)

Unspecified
macOS
defect

Tracking

()

VERIFIED FIXED
Firefox 51
Iteration:
51.3 - Sep 19
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 + verified
firefox51 --- verified

People

(Reporter: selee, Assigned: adw)

References

Details

(Keywords: regression, Whiteboard: [fxprivacy])

Attachments

(1 file, 1 obsolete file)

After the items in Summary section are downloaded, the panel panelmultiview#downloadsPanel-multiView did not shrink to the fit height. (see attachment 8777683 [details])
Blocks: 1280709
No longer depends on: 1280709
[Tracking Requested - why for this release]:
User interface regression with a new feature developed for Firefox 50 in bug 1252509.
Paolo, do you know who can fix this? Is it Drew (author of patches in bug 1252509)?
Flags: needinfo?(paolo.mozmail)
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(paolo.mozmail)
We should try the solution in bug 1294435 comment 1 to solve this issue and the Control Center issues.

Drew, let me know if you want to try that, or I can also work on it maybe a bit more slowly.
Depends on: 1294435
I tried fixing this, and am still trying, but it's not as simple as setting/unsetting flex.  When you set the spacer's flex to zero, you end up with white space below the footer buttons in either or both the main view and subview.

panelUI does a ton of stuff with heights internally.  I think any proper fix is going to have to modify that somehow.
(In reply to Drew Willcoxon :adw from comment #4)
> I tried fixing this, and am still trying, but it's not as simple as
> setting/unsetting flex.  When you set the spacer's flex to zero, you end up
> with white space below the footer buttons in either or both the main view
> and subview.
> 
> panelUI does a ton of stuff with heights internally.  I think any proper fix
> is going to have to modify that somehow.

Yeah, I think we need to take a deep look at how PanelUI is calculating heights and greatly simplify it. Bug 1252224 was a start, but it keeps the most of the code that sets the heights.

Have you also looked at using HTML flexbox instead of XUL flexbox?
Tracked for Fx50 since this is a regression in a new feature.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Yeah, I think we need to take a deep look at how PanelUI is calculating
> heights and greatly simplify it. Bug 1252224 was a start, but it keeps the
> most of the code that sets the heights.

I'm still investigating, but I'm not sure we can remove the manual height calculation. I guess the reason why we cannot leverage the layout engine is that we want a transition to occur to the new height, so we need to set the new value manually. Or maybe we cannot ignore arbitrary stack elements when doing the stack layout.

If we need the manual calculation, however, what we can do is to apply the same exact model as the subviews to the main view. Instead of a spacer, we could have a fixed header and footer around a potentially scrollable or expandable body container.

The fix in bug 1293242 would help us getting the right scroll height for the main section, that has wrapping description elements, in the Control Center.
(In reply to :Paolo Amadini from comment #8)
> Review commit: https://reviewboard.mozilla.org/r/73658/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/73658/

Check this out, just apply the patch and visit this page for a fun and smooth animated multiview:

chrome://browser/content/customizableui/test.xul

This method allows the layout engine do all the height calculations and animations.

The only limitation so far is that I haven't been able to automatically limit the panel height to the distance between the anchor and the screen border. It can only be calculated automatically if we don't lock the panel size temporarily to avoid flickering. This means that we either calculate this manually or we need another method to avoid the flickering.

Obviously the Application Menu is more complex than a list of items. Is there any reason why we would still need to lock the height or still check overflow events there? Drag and drop maybe?
Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Paolo Amadini from comment #9)
> Obviously the Application Menu is more complex than a list of items. Is
> there any reason why we would still need to lock the height or still check
> overflow events there? Drag and drop maybe?

I don't understand the question. Maybe Jared will, he's been looking at this a lot more.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Ritu Kothari (:ritu) from comment #6)
> Tracked for Fx50 since this is a regression in a new feature.

So, if the approach I'm experimenting with works, it would fix all our problems with the Downloads Panel, but it would be somewhat risky for uplift to Aurora, because it affects the main Application Menu as well. We would need manual QA soon after this lands in mozilla-central before uplifting.

On the other hand, this approach has the potential to improve the smoothness of the main panel animation, which is a good thing to have one release sooner.

The second alternative is to work around the height issues specifically for the Downloads Panel, but the panel will flicker while the height is adjusted. This would be a visible issue that would last for the duration of one release.

The third alternative is to backout the subview feature, including the base changeset and the regressions we have already uplifted. In fact there are a few other regressions related to this feature that haven't landed yet, so it may be the safer bet.

Ritu, what do you recommend from a timeline perspective with regard to uplifting a risky patch?
Flags: needinfo?(rkothari)
(In reply to :Paolo Amadini from comment #12)

> 
> Ritu, what do you recommend from a timeline perspective with regard to
> uplifting a risky patch?

Hi Paolo, I think we should either wontfix this for Fx50 since it seems like a minor issue to me. It is not ideal that the downloads panel is not resized in the attached screenshot but it is something we can live with for one release vs. uplifting a very risky patch that puts the main application menu at risk. I would rather use the first approach for Fx51 and let that ride the trains.

The other approach for Fx50 is to go for the 2nd alternative where you resize the downloads panel and cause a flicker. Would this also cause a noticeable perf regression where the flicker lasts long and makes the browser unusable for say 2-3 seconds? 

Hi Justin, would you be able to weigh in on this regression and tell us which option you think would be best? Fx50 is ~2.5 weeks away from entering Beta phase.
Flags: needinfo?(rkothari) → needinfo?(dolske)
I think it's hard to evaluate risk without an actual patch. ;-)

I've not been following the downloads panel work, so I don't have an opinion on the best approach here. It seems like the same basic experience as it's always been, so if there are other regressions it might be best to back out that work until we're more comfortable with shipping it. OTOH, doing a backout a cycle after originally landing carries its own regression risk, and attachment 8777683 [details] seems like a fairly minor UI glitch.
Flags: needinfo?(dolske)
(In reply to :Paolo Amadini from comment #12)

> So, if the approach I'm experimenting with works, it would fix all our
> problems with the Downloads Panel, but it would be somewhat risky for uplift
> to Aurora, because it affects the main Application Menu as well.

I'd probably defer a final risk decision to Gijs, but the app menu has historically been a bit fragile, so I'd be wary of non-obvious breakage.
(In reply to Ritu Kothari (:ritu) from comment #13)
> The other approach for Fx50 is to go for the 2nd alternative where you
> resize the downloads panel and cause a flicker. Would this also cause a
> noticeable perf regression where the flicker lasts long and makes the
> browser unusable for say 2-3 seconds? 

The flicker would be visible but very quick. All in all, maybe we could live with this for the Downloads Panel, which is not as commonly used as the main menu. I can move the more complex work I'm doing here to another bug and implement the best trade-off between flickering and wrong height here.
Moving to bug 1009116.
Blocks: 1009116
Attachment #8784094 - Attachment is obsolete: true
Flags: needinfo?(jaws)
(In reply to :Paolo Amadini from comment #16)
> ... and implement the best trade-off between flickering and wrong height here.

Sounds like you'll take this then?  (Which is fine with me :-)
Assignee: adw → paolo.mozmail
Ah, yes, but obviously if you have already looked into it feel free to attach a patch before I do. I probably won't start working on this until next week.
Thanks everyone for weighing in on this one. I think we are all in agreement that this is a minor UI glitch. I am setting fx50 status to "fix-optional". If a low-risk patch is ready soon, please feel free to nominate for uplift to Aurora50.
This is the very simple patch I had.  It calls setHeightToFit when the summary is hidden, just like it's called when the item count changes.  The flicker is very visible but quick, like you say in comment 16.  To be honest though I'm not sure that the extra height is worse than the flicker.
Comment on attachment 8785486 [details]
Bug 1292345 - Downloads panel didn't shrink to the fit height after all items are downloaded.

(In reply to Drew Willcoxon :adw from comment #22)
> To be honest though I'm not sure that the extra height is worse than the flicker.

I'd say let's accept the flicker and use the correct height. The former is temporary, the latter stays on the screen.
Attachment #8785486 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8785486 [details]
Bug 1292345 - Downloads panel didn't shrink to the fit height after all items are downloaded.

https://reviewboard.mozilla.org/r/74668/#review72852
Attachment #8785486 - Flags: review+
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7af509f4eb3b
Downloads panel didn't shrink to the fit height after all items are downloaded. r=me
https://hg.mozilla.org/mozilla-central/rev/7af509f4eb3b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment on attachment 8785486 [details]
Bug 1292345 - Downloads panel didn't shrink to the fit height after all items are downloaded.

Approval Request Comment
[Feature/regressing bug #]: Sliding subview for blocked download info in the downloads panel, bug 1252509
[User impact if declined]: The height of the downloads panel will remain unnecessarily large after the downloads summary is removed
[Describe test coverage new/current, TreeHerder]: Manual testing
[Risks and why]: Low risk
[String/UUID change made/needed]: None
Attachment #8785486 - Flags: approval-mozilla-aurora?
Iteration: --- → 51.3 - Sep 12
Flags: qe-verify?
Hello Sean, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(selee)
Comment on attachment 8785486 [details]
Bug 1292345 - Downloads panel didn't shrink to the fit height after all items are downloaded.

Fixes a regression, Aurora50+
Attachment #8785486 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hey Ritu, the issue is resolved after verifying on OSX with the following STR:

1. download one file [A] and pause immediately.
2. download three files until them finished, and the summary section is shown.
3. resume file [A] until it finished.
4. the downloads panel shrinks to the fit height.
Flags: needinfo?(selee)
Is it expected the "Show all downloads" footer to show up on hover over "+1 other download"? http://imgur.com/a/bcPWR
Moreover, clearing all the downloads from library view makes the downloads panel to remain very large. http://imgur.com/a/a13xJ
Flags: needinfo?(paolo.mozmail)
(In reply to Paul Silaghi, QA [:pauly] from comment #32)
> Is it expected the "Show all downloads" footer to show up on hover over "+1
> other download"? http://imgur.com/a/bcPWR

Yes, it's bug 1298276.

> Moreover, clearing all the downloads from library view makes the downloads
> panel to remain very large. http://imgur.com/a/a13xJ

Drew, don't we set the size already when the item count changes?
Assignee: paolo.mozmail → adw
Flags: needinfo?(paolo.mozmail) → needinfo?(adw)
At least setHeightToFit is called by _itemCountChanged: https://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/downloads.js#745

Maybe _itemCountChanged isn't getting called in this case?
Flags: needinfo?(adw)
No, it's getting called but the height isn't actually changing.
Because the panel isn't open in this case, I bet.
(In reply to Sean Lee [:seanlee][:weilonge] from comment #31)
> Hey Ritu, the issue is resolved after verifying on OSX with the following
> STR:
> 
> 1. download one file [A] and pause immediately.
> 2. download three files until them finished, and the summary section is
> shown.
> 3. resume file [A] until it finished.
> 4. the downloads panel shrinks to the fit height.

Great! Thanks a lot for a prompt reply.
Status: RESOLVED → VERIFIED
(In reply to Drew Willcoxon :adw from comment #36)
> Because the panel isn't open in this case, I bet.

That's now on file as bug 1300556.
Depends on: 1300556
Flags: qe-verify? → qe-verify+
QA Contact: paul.silaghi
Verified fixed FX 50.0a2 (2016-09-12) Win 7, if the downloads panel is visible.
Note this isn't completely fixed on FX 50 (if the downloads panel is hidden), until bug 1300556 lands here too.
See Also: → 1311667
You need to log in before you can comment on or make changes to this bug.