450322 - Broken windows animation in Vista Aero with 3.1 Alpha and nightly

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Recall]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.9.0.1) Gecko/2008070208 Firefox
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.9.0.1) Gecko/2008070208 Firefox

Animation in for windows in the 3.1 alpha and windows nightly is broken. When opening firefox or any windows they appear instant, when they should fade in to view.

This works fine in 3.01, but in 3.1 the fade in animation is broken. It only animates on close. This affects the opening of the main firefox window, and any subsequent ones such as options /bookmarks / new windows etc.

I will attach two videos to show the problem.

Reproducible: Always

Steps to Reproduce:
1. Load up Firefox 3.1
2. Open options

Actual Results:  
Windows do not fade in.

Expected Results:  
Windows should fade in.

Comment 1

[Recall]

Here is a video with 3.01 animating the windows

http://www.uploading.com/files/J67SGR0R/3.01.avi.html

Comment 2

[Recall]

Here is a video of 3.1 NOT animating the windows

http://www.uploading.com/files/UJGRVS9T/3.1.avi.html

Comment 3

[Recall]

Got a regression range:

Works:

07.08.08 http://forums.mozillazine.org/viewtopic.php?f=23&t=784265

Fails

08.08.08 http://forums.mozillazine.org/viewtopic.php?f=23&t=785975

Comment 4

[redsign]

CONFIRMED as described above!

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/20080817033619 Minefield/3.1a2pre 

Comment 5

[u88484]

Karl, it is definitely is not a blocker, critical or a major bug.  It is a very minor bug.

Comment 6

[Recall]

Is there going to be any movement on this? Since it was busted down to minor, nobody is taking notice of it. With the first beta not far off, would it not be wise to implement a fix?

Comment 7

[Rob Arnold [:robarnold]]

I looked at it. There definitely is a regression between fx3 and trunk, but I didn't see anything in the regression range that looked like it was the cause, but I'm not too familiar with why the windows choose to animate or not.

Comment 8

[Recall]

Well still broken, and no sign of anyone attempting to fix it? How would I go about getting more exposure to this bug?

Comment 9

[Rob Arnold [:robarnold]]

Can you narrow it down to the particular changeset the broke it?

Comment 10

[u88484]

(In reply to comment #8)
> Well still broken, and no sign of anyone attempting to fix it? How would I go
> about getting more exposure to this bug?
Annoying the heck out of people isn't going to encourage them to fix it.  Someone will get around to it one day when they are bored and want to fix something that means absolulety nothing.  It is such a trivial issue that there is absoluetly no point in you bugspamming.

Comment 11

[Kai Liu]

Works: 7895ffd97cb2
Broken: ca7bb3f59be1

So bug 438987 somehow caused this...

Comment 12

[Bill Gianopoulos [:WG9s]]

Wanted request had not been carried forward when component changed form Firefox to Core.

Comment 13

[Rob Arnold [:robarnold]]

I remember some odd behavior I observed while debugging for bug 451300. The window will be change to transparent and then back to opaque. This used to not be the case. I wonder if making a window transparent causes it to not be animated by the DWM.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This is going to end up being a polish issue, so preemptively marking it P1/wanted.  Needs analysis of the changes in bug 348987...

Comment 15

[Recall]

Is there a chance of this being fixed, nobody has looked at since November.

Comment 16

[Jim Mathies [:jimm]]

Ok, I've been able to reproduce this. It is a pretty ugly polish issue, not sure why we didn't block on it. Starting in on it today... lets hope there's an easy fix.

Comment 17

[Jim Mathies [:jimm]]

Attachment: frame transparency fix v.1

Comment 18

[Jim Mathies [:jimm]]

Comment on attachment 360393 [details] [diff] [review]
frame transparency fix v.1

A slight touch up from the patch that landed in bug 438987. We snipped out some code that was needed.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This seems like a bit of a hack. There's no real reason to not treat a viewportFrame as a canvas. Can we address this more reasonably in GetFrameTransparency?

Comment 20

[Jim Mathies [:jimm]]

(In reply to comment #19)
> This seems like a bit of a hack. There's no real reason to not treat a
> viewportFrame as a canvas. Can we address this more reasonably in
> GetFrameTransparency?

I'll take a look.

Comment 21

[Jim Mathies [:jimm]]

Attachment: frame transparency fix v.2

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 360544 [details] [diff] [review]
frame transparency fix v.2

The ifdef'ing could probably be left out since code similar to this was in cross platform code for a number of years. But I figured isolating it to win couldn't hurt since it's not needed on any other platform.

The other thing I was wondering about was if there is a way to detect a newly created frame from through nsIFrame's properties rather than using the first child. I looked around and didn't find anything. First child does works well though and was used this way prior to bug 438987 landing.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let's not use #ifdefs. Cross platform consistency is important.

So when the window changes from transparent to opaque, are we actually notifying the widget, and is that notifying Vista?

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I mean before this patch.

I guess I'm trying to understand if we're working around a Vista bug here.

Comment 25

[Jim Mathies [:jimm]]

(In reply to comment #23)
> Let's not use #ifdefs. Cross platform consistency is important.

No problem, I'll take it out.

> So when the window changes from transparent to opaque, are we actually
> notifying the widget, and is that notifying Vista?

Previously IsCanvasFrame would return false as long as the view had no children. In that case, nsContainerFrame's SyncFrameViewGeometryDependentProperties quit processing before setting transparency on the window. I'm guessing the default was opaque.

After IsCanvasFrame started returning true for an empty view, the transparency was set to transparent through calls to nsLayoutUtils::GetFrameTransparency and widget->SetTransparencyMode(mode) -

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.cpp#485

that caused a call into nsWindow that added WS_EX_LAYERED extended style, which killed the fade effect vista uses on conventional windows. Once the view's content filled in, the transparency was set back to opaque and the window popped into view.

The change I made returns opaque when there are no children (a good test for a newly created window?) so that the layering never gets turned on, and the fade effect takes place.

I'm not sure I'd characterize this as a vista bug, skipping off the fade on layered windows is probably by design.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

But we remove the WS_EX_LAYERED bit *before* the fade effect is triggered, right? Or is this a fade-in effect?

Comment 27

[Jim Mathies [:jimm]]

(In reply to comment #26)
> But we remove the WS_EX_LAYERED bit *before* the fade effect is triggered,
> right? Or is this a fade-in effect?

It's a fade-in effect on windows we create like the main window and dialogs for things like preferences. The first time we set transparency to transparent occurs on the very first call into nsXULDocument::StartLayout, and we set it back to opaque just a short while later. This patch prevents that. The set in StartLayout seems to be enough for vista to give up doing a fade-in effect and instead just display the complete window. That results in a sort of old school 'pop' when the window displays. On older operating systems there wouldn't be a difference, but on vista it's pretty obvious.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 360544 [details] [diff] [review]
frame transparency fix v.2

Alright, let's do this. The comment should explain that we need an uninitialized window to be treated as opaque because doing otherwise confuses some platforms, specifically Vista.

Comment 29

[Jim Mathies [:jimm]]

Attachment: landed patch

Comment 30

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/64bebcc65154

Comment 31

[Recall]

Verfied working and fixed on Vista X64 Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090205 Minefield/3.2a1pre. Thanks Jim!

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 360638 [details] [diff] [review]
landed patch

a1.9.1=dbaron

Comment 33

[Jim Mathies [:jimm]]

(In reply to comment #32)
> (From update of attachment 360638 [details] [diff] [review])
> a1.9.1=dbaron

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/54baaa367804

Comment 34

[Marcia Knous [:marcia]]

Adding the fixed1.9.1 keyword to this bug so that this can be tracked.

Comment 35

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P1 - Polish issue that appears in the main window, or is something that the user may encounter several times a day.

This was a significant issue with opening and closing the main window.