1445519 - Window size keep shrinking when open the same window multiple times without width/height after bug 1439875

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase

This only seems to happen on macOS with bug 1439875 applied.

Steps to reproduce:
1. open the testcase
2. click the button to open the new window
3. click the button in the new window to open another new window
repeat 3.

Expected result:
All windows should have the same size.

Actual result:
New windows keep shrinking by a titlebar height.


This may be the same issue as bug 1444045, and it doesn't seem to be fixed by current patches in bug 1444525 so it's probably worth separate investigating.

And this is the reason of wpt failure when landing bug 1439875.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

From the log of XULStore, we can see that it keeps saving a smaller height each time a new window is opened:
XULStore: Saving height=777 for id=main-window, doc=chrome://browser/content/browser.xul
XULStore: Saving height=755 for id=main-window, doc=chrome://browser/content/browser.xul
XULStore: Saving height=733 for id=main-window, doc=chrome://browser/content/browser.xul
XULStore: Saving height=711 for id=main-window, doc=chrome://browser/content/browser.xul

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

So the reason is... again, the value returned from nsCocoaWindow::ClientToWindowSize is not reliable probably when window hasn't shown up. It doesn't extend the size as expected, and thus SizeShell uses the inner size to resize the window directly.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

I have a patch...

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1445519 - Use frameRectForContentRect:styleMask: for nsCocoaWindow::ClientToWindowSize.

Review commit: https://reviewboard.mozilla.org/r/227588/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227588/

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8c8ef0fb0ce423211d336813e0626a3104b7178

Comment 6

[Richard Marti (:Paenglab)]

Tried this patch on TB and it fixes the growing on every start. Tested with drawInTitlebar and without.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8958674 [details]
Bug 1445519 - Use frameRectForContentRect:styleMask: for nsCocoaWindow::ClientToWindowSize.

https://reviewboard.mozilla.org/r/227588/#review233500

Do you know how this API is used?

[mWindow frameRectForContentRect:rect] returns a non-inflated rect if mWindow is a window that draws into the titlebar. So, in other words, when ClientToWindowSize is called on such a window, it correctly gives you the inflation for a window of the same type, i.e. no inflation at all. If this value is then used to create a window that *does have a separate titlebar*, then the result window is going to be too small.

This seems like a problem inherent to this choice of API, and so I think this patch is a workaround that only helps for windows that are created with titlebars.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8958674 [details]
Bug 1445519 - Use frameRectForContentRect:styleMask: for nsCocoaWindow::ClientToWindowSize.

https://reviewboard.mozilla.org/r/227588/#review233504

r+ with the comment changed to:

// Our caller expects the inflated rect for windows *with separate titlebars*,
// i.e. for windows where [mWindow drawsContentsIntoWindowFrame] is NO.
// So we call frameRectForContentRect on NSWindow here, instead of mWindow, so
// that we don't run into our override if this window is a window that draws
// its content into the titlebar.

But I'd like some more explanations of why this helps and a bug filed to create a consistent API that does not need this workaround.

Comment 9

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #7)
> If this
> value is then used to create a window that *does have a separate titlebar*,
> then the result window is going to be too small.

I realize now that that's not really what's happening. I thought we were calling this method on one window in order to compute the size for a different window. But we're not doing that.

So now I'm thinking that what might be happening is that we create a window, remove its titlebar, call this method, size the window, and re-add its titlebar. Changing the chromemargin attribute keeps the outer size of the window fixed and resizes the inner size of the window.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Markus Stange [:mstange] from comment #9)
> (In reply to Markus Stange [:mstange] from comment #7)
> > If this
> > value is then used to create a window that *does have a separate titlebar*,
> > then the result window is going to be too small.
> 
> I realize now that that's not really what's happening. I thought we were
> calling this method on one window in order to compute the size for a
> different window. But we're not doing that.
> 
> So now I'm thinking that what might be happening is that we create a window,
> remove its titlebar, call this method, size the window, and re-add its
> titlebar. Changing the chromemargin attribute keeps the outer size of the
> window fixed and resizes the inner size of the window.

See https://mozilla.logbot.info/layout/20180314 for an hypothesis of how that may end up happening.

If that's true, then this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a77cf3479b2134ca49013ae7ef721fdb8879b33c should fix it, I'd expect.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58eb13b394f4
Use frameRectForContentRect:styleMask: for nsCocoaWindow::ClientToWindowSize. r=mstange

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/58eb13b394f4

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8958674 [details]
Bug 1445519 - Use frameRectForContentRect:styleMask: for nsCocoaWindow::ClientToWindowSize.

Approval Request Comment
[Feature/Bug causing the regression]: potential regression from bug 1442521 (given bug 1444045), and this is a prerequisite for bug 1439875 we may want to uplift
[User impact if declined]: may have issue on mac with window size, and also we would not be able to uplift bug 1439875 for stylo-chrome's performance fix
[Is this code covered by automated tests?]: kinda covered by a wpt, but probably worth a separate test
[Has the fix been verified in Nightly?]: just landed
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: it aligns our behavior on mac to what we have on windows.
[String changes made/needed]: n/a

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Also note that please uplift the patch landed rather than the patch in MozReview. There is some comment changes.

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 8958674 [details]
Bug 1445519 - Use frameRectForContentRect:styleMask: for nsCocoaWindow::ClientToWindowSize.

fix an issue with window sizes on mac, beta60+

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fb798a901a72