355177 - window.resizeTo behavior changed in cocoa widget

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P4)

Description

[(not reading, please use [e-mail] instead)]

the resize animation when switching panels in preference dialog on Mac OS X trunk (with cocoa widgets) is not smooth.

now that cocoa-widgets has turned on, when I switch panels the window appears to flash during the stages of the animation.

Comment 1

[Josh Aas]

This fix is on 347452. Though technically that bug is about the API that fixes this, so lets leave this open and make it dependent on 347452.

Comment 2

[Phil Ringnalda (:philor)]

*** Bug 362417 has been marked as a duplicate of this bug. ***

Comment 3

[Phil Ringnalda (:philor)]

*** Bug 363009 has been marked as a duplicate of this bug. ***

Comment 4

[Phil Ringnalda (:philor)]

*** Bug 363269 has been marked as a duplicate of this bug. ***

Comment 5

[Phil Ringnalda (:philor)]

*** Bug 363684 has been marked as a duplicate of this bug. ***

Comment 6

[(not reading, please use [e-mail] instead)]

Attachment: patch, updated for the trunk (with josh's patches), comments coming next...

Comment 7

[(not reading, please use [e-mail] instead)]

josh, so your patch works (sort of?) with one change:

I had to move the "aPaneElement.style.opacity = 0.0;" to the if not mac os x part of preference.xml, as the animate / fade timer is responsible for twiddling the opacity, but we no longer use those timers on mac os x.

So:

                     aPaneElement.style.opacity = 0.0;
+#ifdef XP_MACOSX
+                    var chromeDOMWindow = window.QueryInterface(Components.interfaces.nsIDOMChromeWindow);
+                    chromeDOMWindow.animatedResize = chromeDOMWindow.RESIZE_ANIMATION_SLIDE;
+                    window.innerHeight = aPaneElement.contentHeight;
+                    this._currentHeight = aPaneElement.contentHeight;
+                    chromeDOMWindow.animatedResize = chromeDOMWindow.RESIZE_ANIMATION_OFF;
+#else
                     this.animate(oldPane, aPaneElement);
+#endif

is now:

+#ifdef XP_MACOSX
+                    var chromeDOMWindow = window.QueryInterface(Components.interfaces.nsIDOMChromeWindow);
+                    chromeDOMWindow.animatedResize = chromeDOMWindow.RESIZE_ANIMATION_SLIDE;
+                    window.innerHeight = aPaneElement.contentHeight;
+                    this._currentHeight = aPaneElement.contentHeight;
+                    chromeDOMWindow.animatedResize = chromeDOMWindow.RESIZE_ANIMATION_OFF;
+#else
                     aPaneElement.style.opacity = 0.0;
                     this.animate(oldPane, aPaneElement);
+#endif

try the patch out and let me know what you think.

Comment 8

[(not reading, please use [e-mail] instead)]

the animation is smooth, but we resize to a "too small" height.  let me attach a screen shot.

Comment 9

[(not reading, please use [e-mail] instead)]

Attachment: screen shot

Comment 10

[wtallis]

Confirmed that this bug is in Gran Paradiso alpha 3. This should be a release blocker. This is a serious regression of UI behavior. 

Comment 11

[Josh Aas]

Attachment: fix v1.0

Prefs fix, sizes correctly. This depends on the API in bug 347452.

Comment 12

[(not reading, please use [e-mail] instead)]

Comment on attachment 260549 [details] [diff] [review]
fix v1.0

r=sspitzer, but:

1)  please move the try / catch from column 0 to the right column.

2)   I'd just do "catch (e) { }" (following the convention of this file, and not dump).  

note, we do have dumps in this file, but I don't think we should dump() here.

I believe the try / catch will become useful when we remove the #ifdef XP_MACOSX and use this code on other platforms that don't support AnimatedResize, right?  for those platforms, we'd throw NS_ERROR_NOT_IMPLEMENTED (according to your patch bug #347452).

speaking of removing XP_MACOSX, can you log a spin off bug about doing that?  

(also, can log a spin off bug about porting #347452 for Vista, too?)  

additionally, you mentioned today that you'd like to remove all the animation code, as it is (by default) mac only.

can you log a a spin off bug on that, too?

Comment 13

[Josh Aas]

Attachment: fix v1.0.1

Comment 14

[Josh Aas]

Note that this patch requires the patch on bug 347452, they would land at the same time.

Comment 15

[Josh Aas]

The patch in bug 347452 landed, fix v1.0.1 should work with current trunk and no other patches.

Comment 16

[wtallis]

Seems Fixed in the latest nightly (Version 2007040505 (1.2+)).

Comment 17

[Neil Deakin]

Comment on attachment 260782 [details] [diff] [review]
fix v1.0.1

>-
>+#ifdef XP_MACOSX
>+                  try {
>+                    var chromeDOMWindow = window.QueryInterface(Components.interfaces.nsIDOMChromeWindow);

No need to QI here, just use window directly instead of chromeDOMWindow.

Comment 18

[Josh Aas]

It is necessary - this doesn't work without the QI.

Comment 19

[Neil Deakin]

Works fine here. I suspect something unrelated is causing an issue.

Comment 20

[Josh Aas]

Attachment: fix v1.0.2

I made a mistake testing without QI, works for me now. Thanks!

Comment 21

[Josh Aas]

fixed on trunk

Comment 22

[Dave Townsend [:mossop]]

Now that bug 393117 has landed, effectively backing out bug 347452 we're back to this horrendous looking animation when changing panes. Note that this has blocking-1.9

Comment 23

[Colin Barrett [:cbarrett]]

The plan was to add heights to the Mac prefs panes and not have the presf window resize at all. I could have sworn I filed a follow up already, but I guess not. Filed as bug 394881.

Comment 24

[Josh Aas]

We should have separate bugs for the change in window resize behavior in cocoa widgets and the problems with the Firefox prefs window.

Since this bug has been a cocoa widget blocker, this is going to be about the cocoa widget behavior. Adjusted summary accordingly.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #24)
> Since this bug has been a cocoa widget blocker, this is going to be about the
> cocoa widget behavior. Adjusted summary accordingly.

Are you saying that this bug is not what caused the preference pane animation to be choppy?

Comment 26

[Josh Aas]

No. I'm just saying the nature of the situation makes me think we need two bugs. There is a good chance we're not going to fix this one but the prefs symptom definitely needs to be fixed.

Comment 27

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

The problem is that there is a disconnect between nsCocoaWindow and nsChildView -- the CocoaWindow is the thing that gets resized, and a Resize() is doing the right thing.  (Note: the 5-arg form of Resize that specifies x,y,w,h needs to be rewritten to do a single setFrame call, instead of calling Resize() and MoveTo(), but that's not the case here.)

However, after the resize, our nsChildView content view is in the wrong place -- the window is resized from the bottom left, which pushes the child content up, when we instead want it truncated.  (One fix might be to move the child content into the right spot after any Resize() of the nsCocoaWindow; but this is complicated because the child uses a flipped coordinate space, and so its bounds are always at origin (0,0).)

It's easy to see this effect by putting in a sleep(1) at the end of nsCocoaWindow::Resize (the 3-arg form), setting browser.preferences.animateFadeIn to true, and playing with the prefs dialog.  The problem also might be that when internally the cocoa window is resized, the window backing store graphic is shifted up, and we don't get a chance to repaint it until it's already been displayed to the screen... but that feels wrong to me.

Comment 28

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

Attachment: useful test case

run test case with -chrome; useful for debugging.

Comment 29

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

Attachment: fix

So here's a fix.  There are two main pieces:

1) We always have to set display:YES -- if we don't, the window frame isn't immediately redrawn, and so the title bar will get shown in the wrong place (because whatever image bits were there in the window just get shifted, always anchored to the bottom left, as soon as you call setFrame).

2) We have to send a gecko resize event -before- we do the actual resize -- this makes sure that the children and child content is moved into place before the resize actually happens; otherwise, again, child content will be anchored at the bottom left, and will appear at the wrong place briefly.

With this patch, the testcase shows no flicker, and setting browser.preferences.animateFadeIn to true results in smooth animation of the prefs dialog.

I'm pretty sure nothing bad happens if we fire the resize event before we actually do the reisze, because we're doing it immediately after.  We may need to do a bit more work here to make sure that the size-getting functions lie about the bounds while a resize is in progress (mBounds and GetScreenBounds I think), but this gets us part of the way there, and I haven't seen any ill effects.

Comment 30

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

Comment on attachment 287781 [details] [diff] [review]
fix

wrong patch

Comment 31

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

Attachment: real patch

The real patch this time!

Comment 32

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #29)
> With this patch, the testcase shows no flicker, and setting
> browser.preferences.animateFadeIn to true results in smooth animation of the
> prefs dialog.

Great. Does that mean we can back out the patch for bug 394881?

Comment 33

[Shawn Wilsher :sdwilsh]

If that's true, yes.  Someone should verify it first of course.

Comment 34

[Josh Aas]

Comment on attachment 287782 [details] [diff] [review]
real patch

In general this looks great! Nice work.

The problem is that this patch will cause something  similar to bug 396345. You're reporting the size event before doing the resize, which means if the setFrame: call doesn't do what you ask then you've now reported the wrong size. This, for example, happens when you ask the window to get taller than the available screen height, which is not uncommon. The window will  resize but not to the height you requested and then content will start drawing all over the chrome above it.

When we reported the size event after the resize, it then reported the *actual size of the window*, so we didn't have that problem. If you want to report the size before setFrame:, you'll probably need to check the size of the window after setFrame: and if it isn't what you asked for then send another resize notification with the actual size that resulted from setFrame: with your request.

Comment 35

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

Attachment: patch v2

Updated patch; the simple solution is to just check whether we got the frame that we wanted, and if not, do another resize.  It's not ideal, but it shouldn't be hit all that often.  Seems to work fine.  (Simple testcase:  javascript:window.resizeTo(200,3000) -- without the fix, the top of the chrome is way offscreen.)

Comment 36

[Josh Aas]

Comment on attachment 288780 [details] [diff] [review]
patch v2

-  BOOL isMoving = (mBounds.x != aX || mBounds.y != aY);
-  BOOL isResizing = (mBounds.width != aWidth || mBounds.height != aHeight);
+  nsRect windowBounds(cocoaRectToGeckoRect([mWindow frame]));
+  BOOL isMoving = (windowBounds.x != aX || windowBounds.y != aY);
+  BOOL isResizing = (windowBounds.width != aWidth || windowBounds.height != aHeight);

This change doesn't seem to make sense. mBounds should always be correct here, so why do the extra work of getting the native window frame and converting it?

+  // so if we don't do this, our child view will end up being stuck in the

The comma in this section of your comment isn't necessary.

+  if (newFrame.size.width != actualFrame.size.width ||
+      newFrame.size.height != actualFrame.size.height)
+  {

In this file we put the opening brace of the "if" statement at the end of the test, not on a new line.

+    // we didn't; the window must have been too big or otherwise invalid
+    // report -another- resize in this case, to make sure things are in

Add a period after "invalid" here so it is clear that the next word in the comment is the start of a new sentence

Comment 37

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

(In reply to comment #36)
> (From update of attachment 288780 [details] [diff] [review])
> -  BOOL isMoving = (mBounds.x != aX || mBounds.y != aY);
> -  BOOL isResizing = (mBounds.width != aWidth || mBounds.height != aHeight);
> +  nsRect windowBounds(cocoaRectToGeckoRect([mWindow frame]));
> +  BOOL isMoving = (windowBounds.x != aX || windowBounds.y != aY);
> +  BOOL isResizing = (windowBounds.width != aWidth || windowBounds.height !=
> aHeight); 
>
> This change doesn't seem to make sense. mBounds should always be correct here,
> so why do the extra work of getting the native window frame and converting it?


That's true in the case of nsChildView, but it doesn't seem to be for nsWindow.  mBounds.x and mBounds.y seem to always be 0, because nothing updates them -- I wasn't sure if that was a quirk of our widget system where the toplevel window should have mBounds set to (0,0,w,h).

Note that nsCocoaWindow::GetScreenBounds calls [mWindow frame], and doesn't just return mBounds.  Also, in ReportSizeEvent, mBounds.width/.height are set to the dimensions of the content rect, not the outer rect; nsCocoaWindow::Resize wants to deal with the outer rect (that is, the screen bounds).

On win32, Resize() explicitly sets mBounds to the passed-in arguments; so it could be that the cocoa nsCocoaWindow implementation is misusing mBounds.  But, at the same time, nsWindow::GetScreenBounds explicitly queries the window's position if there is a window (as opposed to just a child widget) instead of returning mBounds.

So, explicitly getting the data needed seemed the easiest way to go; I'll ask stuart what mBounds is supposed to be for a toplevel window.

Comment 38

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

Attachment: updated patch

Updated patch with review comments.  I need to file a followup bug about the mBounds issues as well.

Comment 39

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

Fix checked in.