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

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Downloads Panel
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Optimizer, Assigned: Gijs)

Tracking

unspecified
Firefox 28
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox25 affected, firefox26 affected, firefox27 affected, firefox28 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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
Can you reproduce this in a Nightly without Australis (pre-Australis or Holly) or on Aurora?
Blocks: 939862
(Reporter)

Comment 2

4 years ago
Forgot to mention that my downloads button is on the tab strip..
(Reporter)

Comment 3

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

Thanks for checking.
No longer blocks: 939862
Summary: Tab toolbar height changes weirdly in maximized state. → Tab toolbar height changes weirdly in maximized state when download button is on the tabstrip
(Assignee)

Updated

4 years ago
Summary: Tab toolbar height changes weirdly in maximized state when download button is on the tabstrip → Tab toolbar height changes weirdly in maximized state when download button is on the tabstrip and a download completes
(Assignee)

Comment 5

4 years ago
The tab height stays constant throughout, it's the toolbox that changes y coordinates. I don't know why.
Summary: Tab toolbar height changes weirdly in maximized state when download button is on the tabstrip and a download completes → Toolbox moves vertically in maximized state when download button is on the tabstrip and a download completes
(Assignee)

Updated

4 years ago
Component: Theme → Downloads Panel
(Assignee)

Comment 6

4 years ago
This is probably somehow caused by the animation having been moved out of the button.
Blocks: 922847

Updated

4 years ago
status-firefox26: --- → unaffected
status-firefox27: --- → affected
tracking-firefox27: --- → ?
Keywords: regression
Tracking as this is a fallout form Bug 922847 on Fx27 and passing onto :Gijs for help here.
Assignee: nobody → gijskruitbosch+bugs
tracking-firefox27: ? → +
(Assignee)

Comment 8

4 years ago
I can't reproduce this anymore on latest holly. Can you? :-\
Flags: needinfo?(scrapmachines)
(Reporter)

Comment 9

4 years ago
I can still see it in latest nightly though. no idea about Holly.
Flags: needinfo?(scrapmachines)
(Assignee)

Comment 10

4 years ago
(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?
Flags: needinfo?(scrapmachines)
(Reporter)

Comment 11

4 years ago
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.
Flags: needinfo?(scrapmachines)
(Assignee)

Comment 12

4 years ago
(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?
(Reporter)

Comment 13

4 years ago
Nopes.
(Reporter)

Comment 14

4 years ago
But when i run teh code in scratchpad, it does make the arrow glow and all. But it does not make the toolbox shift.
(Assignee)

Comment 15

4 years ago
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?
Blocks: 845408
No longer blocks: 922847
(Assignee)

Comment 16

4 years ago
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.
(Assignee)

Comment 17

4 years ago
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.
No longer blocks: 845408
status-firefox26: unaffected → affected
tracking-firefox27: + → ---
Keywords: regression
(Assignee)

Comment 18

4 years ago
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.
status-firefox25: --- → affected
Summary: Toolbox moves vertically in maximized state when download button is on the tabstrip and a download completes → Download indicator overlay load changes window's sizemode if disparate (1 maximized, 1 restored) windows are open
(Assignee)

Comment 19

4 years ago
Finally found the culprit here: bug 640158
Depends on: 640158
Summary: Download indicator overlay load changes window's sizemode if disparate (1 maximized, 1 restored) windows are open → Download button's indicator overlay load in multiple windows triggers wrong sizemode change, breaking toolbox/tabstrip styling
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...
Flags: needinfo?(mconley)
(Assignee)

Comment 21

4 years ago
Created attachment 8342487 [details] [diff] [review]
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.
Attachment #8342487 - Flags: review?(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?
Attachment #8342487 - Flags: review?(mconley)
(Assignee)

Comment 23

4 years ago
(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.
(Assignee)

Comment 24

4 years ago
Fixed by bug 640158! :-)
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox28: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Does this have or need tests?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: in-testsuite?
(Assignee)

Comment 26

4 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.