Closed Bug 1445519 Opened 2 years ago Closed 2 years ago

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

Categories

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

Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files)

Attached file 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.
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
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.
I have a patch...
Assignee: nobody → xidorn+moz
Component: XUL → Widget: Cocoa
Priority: -- → P1
Tried this patch on TB and it fixes the growing on every start. Tested with drawInTitlebar and without.
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 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.
Attachment #8958674 - Flags: review?(mstange) → review+
(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.
(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.
See Also: → 1445737
See Also: → 1445738
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58eb13b394f4
Use frameRectForContentRect:styleMask: for nsCocoaWindow::ClientToWindowSize. r=mstange
https://hg.mozilla.org/mozilla-central/rev/58eb13b394f4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Blocks: 1444045
See Also: 1444045
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
Attachment #8958674 - Flags: approval-mozilla-beta?
Also note that please uplift the patch landed rather than the patch in MozReview. There is some comment changes.
Comment on attachment 8958674 [details]
Bug 1445519 - Use frameRectForContentRect:styleMask: for nsCocoaWindow::ClientToWindowSize.

fix an issue with window sizes on mac, beta60+
Attachment #8958674 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.