1442521 - Meaning of width/height and {min,max}{width,height} on <window> are unclear

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

See bug 1439875 comment 45.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1442521 - Treat args of nsXULWindow::SetSpecifiedSize as client area size and convert them to outer window size for SetSize.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Funny... although it fixes some failures from bug 1439875, it seems to break more...

Need to see why's that...

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8955404 [details]
Bug 1442521 - Treat args of nsXULWindow::SetSpecifiedSize as client area size and convert them to outer window size for SetSize.

Cancel review request for now...

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Err... It seems the width and height in nsXULWindow is quite a mess... It sometimes means outer size sometimes inner size...

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1442521 - Make width/height attr on <window> mean the inner size in nsXULWindow.

It seems that the layout system assumes those attributes are for size of
the <window> element, i.e. inner window size, not outer window size.

See for example nsContainerFrame::SyncWindowProperties. It reads
{min,max}{width,height} attributes from the element via
nsIFrame::GetXUL{Min,Max}Size, and passes them into SetSizeConstraints.
The latter inflates the sizes with window decoration size before calling
into widget code. It can also be seen that various XUL size related
methods on nsBox and nsIFrame put the same assumption.

The test test_windowminmaxsize.xul apparently puts the same assumption
as the layout system on the meaning of those properties.

(Another test test_resize_move_windows.xul, which tests effectiveness of
features of window.open, also fails if we size the window earlier than
current in bug 1439875, and doesn't fail with this patch on top. It may
indicate that it makes use of the same assumption, but I can't really
figure out how it does so.)

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

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

This patch as itself looks fine now... except the macOS a11y crash which seems weird...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=257786ee7eb464ae79fa9cd3ade22b9fda2f1fcf

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

bz, could you review this patch? smaug is probably in Tokyo for Web Components F2F so may not be able to get to this soonish.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

FWIW, I'll have time tomorrow.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8955451 [details]
Bug 1442521 - Make width/height attr on <window> mean the inner size in nsXULWindow.

https://reviewboard.mozilla.org/r/224612/#review231166

So this is going to cause an issue with restoration of persisted window sizes, right?  That is, right now we're persisting outer window sizes but the first update to a build with this patch will treat them as inner ones?  I guess after that the new (slightly larger) window sizes will be stable...  And switching back will just go back to the slightly smaller sizes?

r=me, but I really hope that this won't confuse things on update.  Please test how updating across this change works when restoring maximized windows, etc...

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> Comment on attachment 8955451 [details]
> Bug 1442521 - Make width/height attr on <window> mean the inner size in
> nsXULWindow.
> 
> https://reviewboard.mozilla.org/r/224612/#review231166
> 
> So this is going to cause an issue with restoration of persisted window
> sizes, right?  That is, right now we're persisting outer window sizes but
> the first update to a build with this patch will treat them as inner ones? 
> I guess after that the new (slightly larger) window sizes will be stable... 
> And switching back will just go back to the slightly smaller sizes?

Yes, exactly. I didn't take this into consider, but indeed it behaves as you described here. I guess it's not a big deal?

> r=me, but I really hope that this won't confuse things on update.  Please
> test how updating across this change works when restoring maximized windows,
> etc...

I tested it for restoring maximized windows, and it seems everything works just the same as what is described above. I think that means it works as expected.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d40106bafeb
Make width/height attr on <window> mean the inner size in nsXULWindow. r=bz

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7d40106bafeb

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Julian, given the regression bug 1446343 and the trickiness of fixing widget code, I suggest we backout this bug from 60beta and continue fixing stuff in 61. Could you help with that?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out from Beta by request.
https://hg.mozilla.org/releases/mozilla-beta/rev/270b317fe5c3