943785 - Download button's indicator overlay load in multiple windows triggers wrong sizemode change, breaking toolbox/tabstrip styling

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Girish Sharma [:Optimizer]]

STR:
1 have two windows, one restored, one maximized.
2 complete a download
3 the arrow glow animation appears and screws up the height of the tab strip in the maximized window .
4 close the restored window, with the downloads button still in green color
5 the maximized window size agains shifts, but is not the normal size
6 click on the green arrow, everything is normal again

Happens at least on latest nightly, windows 7

Comment 1

[Matthew N. [:MattN]]

Can you reproduce this in a Nightly without Australis (pre-Australis or Holly) or on Aurora?

Comment 2

[Girish Sharma [:Optimizer]]

Forgot to mention that my downloads button is on the tab strip..

Comment 3

[Girish Sharma [:Optimizer]]

.. and yes, this happens pre Australis too.

Comment 4

[Matthew N. [:MattN]]

(In reply to Girish Sharma [:Optimizer] from comment #3)
> .. and yes, this happens pre Australis too.

Thanks for checking.

Comment 5

[:Gijs (he/him)]

The tab height stays constant throughout, it's the toolbox that changes y coordinates. I don't know why.

Comment 6

[:Gijs (he/him)]

This is probably somehow caused by the animation having been moved out of the button.

Comment 7

[bhavana bajaj [:bajaj]]

Tracking as this is a fallout form Bug 922847 on Fx27 and passing onto :Gijs for help here.

Comment 8

[:Gijs (he/him)]

I can't reproduce this anymore on latest holly. Can you? :-\

Comment 9

[Girish Sharma [:Optimizer]]

I can still see it in latest nightly though. no idea about Holly.

Comment 10

[:Gijs (he/him)]

(In reply to Girish Sharma [:Optimizer] from comment #9)
> I can still see it in latest nightly though. no idea about Holly.

I tried latest Fx-Team after asking, can't repro there anymore either. Don't understand why because I remember poking at this earlier.

Can you put together more detailed steps starting from a clean profile? In particular, can you see if just firing:

window.DownloadsIndicatorView.showEventNotification("finish")

or

window.DownloadsIndicatorView.showEventNotification("start")

on either the restored or maximized window triggers the same bug?

Comment 11

[Girish Sharma [:Optimizer]]

The STR are exactly the same, I can still repro on fx-team and latest nightly.

Running the code in browser console does nothing. Not even anything to the downloads button.

Comment 12

[:Gijs (he/him)]

(In reply to Girish Sharma [:Optimizer] from comment #11)
> The STR are exactly the same, I can still repro on fx-team and latest
> nightly.
> 
> Running the code in browser console does nothing. Not even anything to the
> downloads button.

Err, that makes no sense. You don't see the arrow move/grow animation when running that code?

Comment 13

[Girish Sharma [:Optimizer]]

Nopes.

Comment 14

[Girish Sharma [:Optimizer]]

But when i run teh code in scratchpad, it does make the arrow glow and all. But it does not make the toolbox shift.

Comment 15

[:Gijs (he/him)]

This actually seems to have to do with the loading of the overlay, which would blame bug 845408 instead (same release impact). That'd explain why just triggering the animation isn't doing sod-all. I also just had this happen when clicking the button, but now that I'm trying to reproduce I'm failing miserably. :-\

CC-ing Mike who might have ideas. Maybe. Hopefully?

Comment 16

[:Gijs (he/him)]

So here's a funny thing. If I take the following steps, I can reproduce:

1. Create a new profile.
2. Customize, and put the downloads button on the tab bar.
3. Restart.
4. Maximize the current window.
5. Open a new window, and restore it.
6. Open the Browser Console from this window.
7. Execute:

let wins = []; let winEnum = Services.wm.getEnumerator("navigator:browser"); wins.push(winEnum.getNext()); wins.push(winEnum.getNext()); wins.forEach((w) => w.DownloadsIndicatorView.hasDownloads = true);

The weird thing is that the maximized window almost looks like it's getting restored window styles applied to it. So I poked about and did this:

3. Restart.
4. Maximize the current window.
5. Open a new window, and restore it.
6. Open the Browser Console from this window.
7. Execute:

let wins = []; let winEnum = Services.wm.getEnumerator("navigator:browser"); wins.push(winEnum.getNext()); wins.push(winEnum.getNext()); wins.map((w) => w.document.documentElement.getAttribute("sizemode")).join(',');

=> outputs "maximized,normal"

wins.forEach((w) => w.DownloadsIndicatorView.hasDownloads = true);

=> shows bug

wins.map((w) => w.document.documentElement.getAttribute("sizemode")).join(',');

=> outputs "normal,normal"

(this is wrong!)

I seem to recall reading code comments or a bug somewhere about how persist and loadOverlay don't play well together, but I honestly have no idea where anymore, I can't find it right now, and it's 1.45am so I'm leaving this for now... anyway, that sizemode change is what needs fixing, AFAICT.

Comment 17

[:Gijs (he/him)]

With the steps in comment #16, I can reproduce in beta. The tabstrip moves up instead of down (which is what it does in Australis; the difference is likely due to different styling applied for sizemode="maximized"), but the same sizemode change manifests. I'm going to remove the tracking, the regression keyword and the dependency because of this. I'll check release next, but I'd be surprised if the issue isn't identical there.

Comment 18

[:Gijs (he/him)]

Annnd on release the same happens. So we've been shipping this. Possibly for a while now, possibly even since the downloads button and its overlay loading was introduced.

Comment 19

[:Gijs (he/him)]

Finally found the culprit here: bug 640158

Comment 20

[Mike Conley (:mconley) (:⚙️)]

The workaround in bug 640158 might need to be used again here.

Note that we *removed* that workaround function for Australis, and it might need to go back...

Comment 21

[:Gijs (he/him)]

Attachment: workaround persistency fail by fixing up sizemode after an overlay has loaded,

While we wait for bug 640158 to get fixed, we might as well be comfortable.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8342487 [details] [diff] [review]
workaround persistency fail by fixing up sizemode after an overlay has loaded,

Review of attachment 8342487 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine - but we can probably be OK generating the modeMap ourselves, rather than generating it on-the-fly.

::: browser/components/downloads/content/downloads.js
@@ +623,5 @@
> +  _modeMap: null,
> +  _fixSizeMode: function() {
> +    if (!this._modeMap) {
> +      this._modeMap = {};
> +      for (let m of ["maximized", "minimized", "normal", "fullscreen"]) {

Why can't we just precompute and include _modeMap ourselves?

Comment 23

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #22)
> Comment on attachment 8342487 [details] [diff] [review]
> workaround persistency fail by fixing up sizemode after an overlay has
> loaded,
> 
> Review of attachment 8342487 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine - but we can probably be OK generating the modeMap
> ourselves, rather than generating it on-the-fly.
> 
> ::: browser/components/downloads/content/downloads.js
> @@ +623,5 @@
> > +  _modeMap: null,
> > +  _fixSizeMode: function() {
> > +    if (!this._modeMap) {
> > +      this._modeMap = {};
> > +      for (let m of ["maximized", "minimized", "normal", "fullscreen"]) {
> 
> Why can't we just precompute and include _modeMap ourselves?

We discussed this on IRC and figured we could leave it, but I just landed a fix for the general issue (bug 640158) on inbound, so if that sticks, I think we should mark this bug as fixed and not land this workaround. So let's wait a bit and see where we stand then.

Comment 24

[:Gijs (he/him)]

Fixed by bug 640158! :-)

Comment 25

[u279076]

Does this have or need tests?

Comment 26

[:Gijs (he/him)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #25)
> Does this have or need tests?

It was fixed by bug 640158, which has a test. I don't think this needs separate tests.