Closed Bug 355177 Opened 14 years ago Closed 12 years ago

window.resizeTo behavior changed in cocoa widget

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: moco, Assigned: vlad)

References

Details

Attachments

(4 files, 6 obsolete files)

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.
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.
Assignee: nobody → joshmoz
Depends on: 347452
Component: Preferences → Widget: Cocoa
Product: Firefox → Core
QA Contact: preferences → cocoa
*** Bug 362417 has been marked as a duplicate of this bug. ***
*** Bug 363009 has been marked as a duplicate of this bug. ***
*** Bug 363269 has been marked as a duplicate of this bug. ***
*** Bug 363684 has been marked as a duplicate of this bug. ***
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.
the animation is smooth, but we resize to a "too small" height.  let me attach a screen shot.
Confirmed that this bug is in Gran Paradiso alpha 3. This should be a release blocker. This is a serious regression of UI behavior. 
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Attached patch fix v1.0 (obsolete) — Splinter Review
Prefs fix, sizes correctly. This depends on the API in bug 347452.
Attachment #249954 - Attachment is obsolete: true
Attachment #260549 - Flags: review?(sspitzer)
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?
Attachment #260549 - Flags: review?(sspitzer) → review+
Attached patch fix v1.0.1 (obsolete) — Splinter Review
Attachment #260549 - Attachment is obsolete: true
Attachment #260782 - Flags: review?(enndeakin)
Note that this patch requires the patch on bug 347452, they would land at the same time.
The patch in bug 347452 landed, fix v1.0.1 should work with current trunk and no other patches.
Seems Fixed in the latest nightly (Version 2007040505 (1.2+)).
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.
It is necessary - this doesn't work without the QI.
Works fine here. I suspect something unrelated is causing an issue.
Attached patch fix v1.0.2 (obsolete) — Splinter Review
I made a mistake testing without QI, works for me now. Thanks!
Attachment #260782 - Attachment is obsolete: true
Attachment #261064 - Flags: review?(enndeakin)
Attachment #260782 - Flags: review?(enndeakin)
Attachment #261064 - Attachment is patch: true
Attachment #261064 - Attachment mime type: application/octet-stream → text/plain
Attachment #261064 - Flags: review?(enndeakin) → review+
fixed on trunk
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 377060
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: the resize animation when switching panels in preference dialog on Mac OS X trunk (with cocoa widgets) is not smooth → window.moveTo behavior regressed in cocoa widget (the resize animation when switching panels in preference dialog on Mac OS X trunk (with cocoa widgets) is not smooth)
Target Milestone: --- → mozilla1.9 M9
Target Milestone: mozilla1.9 M9 → ---
Target Milestone: --- → mozilla1.9 M10
Summary: window.moveTo behavior regressed in cocoa widget (the resize animation when switching panels in preference dialog on Mac OS X trunk (with cocoa widgets) is not smooth) → window.resizeTo behavior regressed in cocoa widget (the resize animation when switching panels in preference dialog on Mac OS X trunk (with cocoa widgets) is not smooth)
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.
Summary: window.resizeTo behavior regressed in cocoa widget (the resize animation when switching panels in preference dialog on Mac OS X trunk (with cocoa widgets) is not smooth) → window.resizeTo behavior changed in cocoa widget
(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?
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.
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.
Target Milestone: mozilla1.9 M10 → mozilla1.9 M11
Attachment #261064 - Attachment is obsolete: true
Priority: -- → P4
Target Milestone: mozilla1.9 M11 → ---
Attached file useful test case
run test case with -chrome; useful for debugging.
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #287781 - Flags: review?
Attachment #287781 - Flags: review? → review?(joshmoz)
Comment on attachment 287781 [details] [diff] [review]
fix

wrong patch
Attachment #287781 - Attachment is obsolete: true
Attachment #287781 - Flags: review?(joshmoz)
Attached patch real patch (obsolete) — Splinter Review
The real patch this time!
Attachment #287782 - Flags: review?(joshmoz)
(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?
If that's true, yes.  Someone should verify it first of course.
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.
Attachment #287782 - Flags: review?(joshmoz) → review-
Assignee: joshmoz → vladimir
Status: REOPENED → NEW
Attached patch patch v2Splinter Review
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.)
Attachment #287782 - Attachment is obsolete: true
Attachment #288780 - Flags: review?(joshmoz)
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
Attachment #288780 - Flags: review?(joshmoz) → review-
(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.
Attachment #288780 - Flags: review- → review+
Attached patch updated patchSplinter Review
Updated patch with review comments.  I need to file a followup bug about the mBounds issues as well.
Attachment #289055 - Flags: review?(joshmoz)
Attachment #289055 - Flags: superreview?(pavlov)
Attachment #289055 - Flags: review?(joshmoz)
Attachment #289055 - Flags: review+
Attachment #289055 - Flags: superreview?(pavlov) → superreview+
Fix checked in.
Status: NEW → RESOLVED
Closed: 13 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.