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

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

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

After the items in Summary section are downloaded, the panel panelmultiview#downloadsPanel-multiView did not shrink to the fit height. (see attachment 8777683 [details])

Comment 1

[:Paolo Amadini]

[Tracking Requested - why for this release]:
User interface regression with a new feature developed for Firefox 50 in bug 1252509.

Comment 2

[Andrew Overholt [:overholt]]

Paolo, do you know who can fix this? Is it Drew (author of patches in bug 1252509)?

Comment 3

[:Paolo Amadini]

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.

Comment 4

[Drew Willcoxon :adw]

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.

Comment 5

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

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

Comment 6

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for Fx50 since this is a regression in a new feature.

Comment 7

[:Paolo Amadini]

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

Comment 8

[:Paolo Amadini]

Attachment: Bug 1292345 - Proof of concept.

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

Comment 9

[:Paolo Amadini]

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

Comment 10

[:Gijs (out for now; he/him)]

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

Comment 11

[:Paolo Amadini]

Comment on attachment 8784094 [details]
Bug 1292345 - Proof of concept.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73658/diff/1-2/

Comment 12

[:Paolo Amadini]

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

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 14

[Justin Dolske [: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.

Comment 15

[Justin Dolske [: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.

Comment 16

[:Paolo Amadini]

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

Comment 17

[:Paolo Amadini]

Moving to bug 1009116.

Comment 18

[Drew Willcoxon :adw]

(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 :-)

Comment 19

[:Paolo Amadini]

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.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 21

[Drew Willcoxon :adw]

Attachment: Bug 1292345 - Downloads panel didn't shrink to the fit height after all items are downloaded.

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

Comment 22

[Drew Willcoxon :adw]

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 23

[:Paolo Amadini]

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.

Comment 24

[Drew Willcoxon :adw]

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7af509f4eb3b
Downloads panel didn't shrink to the fit height after all items are downloaded. r=me

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7af509f4eb3b

Comment 27

[Drew Willcoxon :adw]

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

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Sean, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d6db9615d16b

Comment 31

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

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.

Comment 32

[Paul Silaghi, QA [:pauly]]

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

Comment 33

[:Paolo Amadini]

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

Comment 34

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

Comment 35

[Drew Willcoxon :adw]

No, it's getting called but the height isn't actually changing.

Comment 36

[Drew Willcoxon :adw]

Because the panel isn't open in this case, I bet.

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 38

[:Paolo Amadini]

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

Comment 39

[Paul Silaghi, QA [:pauly]]

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.