Initial size of unmaximized window for new profile is too small

RESOLVED FIXED in Firefox 61

Status

()

defect
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: euthanasia_waltz, Assigned: emilio)

Tracking

(Depends on 1 bug, {regression})

61 Branch
mozilla61
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox59 unaffected, firefox60 unaffected, firefox61 fixed)

Details

(Whiteboard: [stockwell fixed:product])

Attachments

(3 attachments)

(Reporter)

Description

a year ago
STR:
1. Create a new profile
2. Launch profile in new browser
3. Unmaximize the new browser window

ER:
Ordinally size.

AR:
Too small.
OS        ScreenSize WindowSize
LinuxMint 1280x1024  300x200
LinuxMint 1024x768   300x200
Manjaro   1024x768   300x99
(No problem on Windows10)
Windows10 1280x1024  1160x916

Former(or 59.0.1) size on LinuxMint(1280x1024) is 1152x899.

mozregression:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a75367f49378821ebe82c84e2dc714beb37bdfc2&tochange=6dbe66440bd2b872776e22d1a2cbd32755c81a2c
(Reporter)

Updated

a year ago
Blocks: 1446264
(Assignee)

Comment 1

a year ago
I can repro this, it needs a really small screen resolution.

Thanks for reporting this!
Assignee: nobody → emilio
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

a year ago
I think the bug 1439875 hid the existing bug 1446629, and bug 1446264 re-exposed it.
(Assignee)

Comment 3

a year ago
That's the resize that changes the window size to small, from a normal size.

Unsure where does that come from...
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
Comment on attachment 8962913 [details]
Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

Hi Karl, Martin. Could you take a look at this patch and see if it makes sense?

I spent a fair amount of time debugging it so the description in the commit message should be accurate, but I'm not sure if this is the preferred way to fix it, or I should just call gtk_window_move_resize(GTK_WINDOW(mShell)) after the gtk_window_* calls instead, or what not.
Attachment #8962913 - Flags: feedback?(stransky)
Attachment #8962913 - Flags: feedback?(karlt)
(Assignee)

Comment 7

a year ago
Now, about why did this work before bug 1439875, and that bug regressed this:

Before that patch, the order of calls is:

  OnChromeLoaded()
    Resize(..) -> enqueues a resize.
    SetSizeMode(Maximized) -> enqueues an event that triggers the window state change..
    Show(..) -> runs _before_ any of those events have run, making the window visible.

Show does a check_resize, and hits https://github.com/GNOME/gtk/blob/39851fbdbf08b3479192ceafaf788fa3bfbe8fd9/gtk/gtkwindow.c#L7781, effectively calling gtk_window_move_resize, since now it's visible, which picks up the first Resize(..) call.

But after that patch, the order of calls is:

  BeforeStartLayout()
    Resize(..) -> enqueues a resize.
    SetSizeMode(Maximized) -> enqueues an event that triggers the window state change.
    // nothing...

So the check_resize that happens after the Resize call bails out, since it's not visible, so there's no gtk_window_move_resize. The SizeMode change event doesn't bail out, effectively dropping the resize event on the floor.
Comment on attachment 8962913 [details]
Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

Thank you for the accurate and helpful analysis.  There have been a number of
bugs in this GTK code, and so it is difficult to predict exactly what effects
even small changes may have.

gdk_window_resize() behaves a little differently from gtk_window_resize().
The difference that I am most afraid of is that the gdk_ version does not add
the necessary extra when CSD is in use.
https://github.com/GNOME/gtk/blob/39851fbdbf08b3479192ceafaf788fa3bfbe8fd9/gtk/gtkwindow.c#L8355

(There is also a difference when called while the window is already maximized.
gtk_window_resize() has no effect, but gdk_window_resize() may have an effect,
perhaps depending on window manager.  I haven't checked the GTK_WINDOW_POPUP
situation.)

For the sake of CSD at least, I would recommend a different workaround.

The minimal change I can imagine would be to call
gtk_container_check_resize(GTK_CONTAINER(mShell)) before
gtk_window_maximize/fullscreen calls.
That would apply resize_width/height before maximized can be set.
Attachment #8962913 - Flags: feedback?(karlt)
Comment on attachment 8962913 [details]
Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

Karl is the best reviewer here.
Attachment #8962913 - Flags: feedback?(stransky)
It may be also related to Bug 1433603.
See Also: → 1433603
(Assignee)

Comment 11

a year ago
(In reply to Karl Tomlinson (:karlt) from comment #8)
> Thank you for the accurate and helpful analysis.  There have been a number of
> bugs in this GTK code, and so it is difficult to predict exactly what effects
> even small changes may have.
> 
> gdk_window_resize() behaves a little differently from gtk_window_resize().
> The difference that I am most afraid of is that the gdk_ version does not add
> the necessary extra when CSD is in use.
> https://github.com/GNOME/gtk/blob/39851fbdbf08b3479192ceafaf788fa3bfbe8fd9/
> gtk/gtkwindow.c#L8355
> 
> (There is also a difference when called while the window is already
> maximized.
> gtk_window_resize() has no effect, but gdk_window_resize() may have an
> effect,
> perhaps depending on window manager.  I haven't checked the GTK_WINDOW_POPUP
> situation.)
> 
> For the sake of CSD at least, I would recommend a different workaround.

I see, that makes sense.

> The minimal change I can imagine would be to call
> gtk_container_check_resize(GTK_CONTAINER(mShell)) before
> gtk_window_maximize/fullscreen calls.
> That would apply resize_width/height before maximized can be set.

Unfortunately that won't work, because by the time this happens, the window is not yet visible, and thus check_resize bails out without doing any work.

I also thought of something like:

    if (mIsShown) {
        gtk_window_resize(GTK_WINDOW(mShell), size.width, size.height);
    } else {
        // Size will be picked up when shown.
        gtk_window_set_default_size(GTK_WINDOW(mShell), size.width, size.height);
    }

By the time Show is called, the window is already maximized, and thus we don't even check the default size anymore... So I'm kinda out of ideas.

I'll try to dig a bit more, maybe workaround it by not calling maximize, etc. before Show(..), but if you know a good way to tell GTK+ "Please update the size even though you're not a visible widget" that'd be neat.
Flags: needinfo?(karlt)
(Assignee)

Comment 12

a year ago
Oh, also, this may explain one of the other weird-ish glitches I'm seeing with GTK, which is the window not being correctly invalidated on first paint.

Even before my patch, the size in gtk_window_map happens to be 1x1 until the resize is checked, and thus the invalidate call invalidates basically nothing.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> I'll try to dig a bit more, maybe workaround it by not calling maximize,
> etc. before Show(..),

Yes, maximize could be delayed until after gtk_widget_show(), I assume

This approach deals with the loss of resize_width when maximizing before show,
which I guess is enough to make the reported case work.
It doesn't address the async resize before maximize after show, but I guess
that is not so common.

> but if you know a good way to tell GTK+ "Please update
> the size even though you're not a visible widget" that'd be neat.

I don't see a way to do that.

An alternative, which is fairly simple but crafty, might be to remove
GDK_WINDOW_STATE_MAXIMIZED from aEvent->changed_mask in
OnWindowStateEvent() if the window is not shown.  That would prevent
gtk_window_state_event() from setting priv->maximized.  The show will trigger
a subsequent window-state-event because GDK_WINDOW_STATE_WITHDRAWN will
change.

This alternative would also address an async resize before maximize after
show.
Flags: needinfo?(karlt)
FWIW, if you are making the window maximized after showing, you would likely cause the same regression as bug 1448199 which happens on Windows due to delayed maximizing.
Any updates here? since this blocks bug 1447859 and in the past 7 days there are 39 failures on the following platforms: Linux debug, Linux x64 asan and linux64-ccov opt.

Recent log failure: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=174442928&lineNumber=17586

Relevant part of the log: 
[task 2018-04-19T00:48:04.962Z] 00:48:04     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_maximized_persist.xul | width should match after restore window - got 200, expected 300
[task 2018-04-19T00:48:04.962Z] 00:48:04     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
[task 2018-04-19T00:48:04.963Z] 00:48:04     INFO - checkWindow@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_maximized_persist.xul:38:7
[task 2018-04-19T00:48:04.963Z] 00:48:04     INFO - runTest@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_maximized_persist.xul:88:5
[task 2018-04-19T00:48:04.965Z] 00:48:04     INFO - async*onload@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_maximized_persist.xul:1:1
[task 2018-04-19T00:48:04.965Z] 00:48:04     INFO - rval@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:146:17
[task 2018-04-19T00:48:04.966Z] 00:48:04     INFO - EventHandlerNonNull*this.addLoadEvent@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:171:13
[task 2018-04-19T00:48:04.967Z] 00:48:04     INFO - @chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1444:5
[task 2018-04-19T00:48:04.968Z] 00:48:04     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-04-19T00:48:04.969Z] 00:48:04     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_maximized_persist.xul | height should match after restore window - got 200, expected 300
Flags: needinfo?(enndeakin)
Whiteboard: [stockwell needswork]
(Assignee)

Comment 16

a year ago
I haven't had the time to look into this recently, but it's on my radar.
Flags: needinfo?(enndeakin)
(Assignee)

Updated

a year ago
Flags: needinfo?(emilio)
(Assignee)

Updated

a year ago
Component: XUL → Widget: Gtk
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Flags: needinfo?(emilio)

Comment 18

a year ago
mozreview-review
Comment on attachment 8962913 [details]
Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

https://reviewboard.mozilla.org/r/231714/#review244790

::: commit-message-0d428:1
(Diff revision 2)
> +Bug 1449166: Prevent maximization of unshown window. r?karlt

This is not actually preventing maximization, but it
does prevent GtkWindow's maximized flag from being set.

Perhaps the checkin comment could be something like
"trick GTK into using values from gtk_window_resize when showing maximized windows

so as to provide a sensible size for the window when the user exits maximized
state."

::: widget/gtk/nsWindow.cpp:3329
(Diff revision 2)
> +    // If we're not shown yet prevent maximization, since it will eat pending
> +    // resizes. See bug 1449166.

I'd like more details in the explanation comment inline here, as the
mechanism is not easy to understand.

Something like:

"When a window is resized before it is shown, gtk_window_resize() delays
 resizes until the window is shown.  If gtk_window_state_event() sees a
 GDK_WINDOW_STATE_MAXIMIZED change [1] before the window is shown, then
 gtk_window_compute_configure_request_size() ignores the values from the
 resize [2].
 [1] https://gitlab.gnome.org/GNOME/gtk/blob/3.22.30/gtk/gtkwindow.c#L7967
 [2] https://gitlab.gnome.org/GNOME/gtk/blob/3.22.30/gtk/gtkwindow.c#L9377

 In order to provide a sensible size for the window when the user exits
 maximized state, we hide the GDK_WINDOW_STATE_MAXIMIZED change from
 gtk_window_state_event() so as to trick GTK into using the values from
 gtk_window_resize() in its configure request.

 We instead notify gtk_window_state_event() of the maximized state change once
 the window is shown."

::: widget/gtk/nsWindow.cpp:3332
(Diff revision 2)
> +        aEvent->changed_mask =
> +            static_cast<GdkWindowState>(aEvent->changed_mask & ~GDK_WINDOW_STATE_MAXIMIZED);

Please wrap so as to keep within 80 columns.

I would wrap after '>' so that the statement requires only two lines, but wrapping after '='
and '&' is fine too.

::: widget/gtk/nsWindow.cpp:3334
(Diff revision 2)
> +    // If we're not shown yet prevent maximization, since it will eat pending
> +    // resizes. See bug 1449166.
> +    if (!mIsShown) {
> +        aEvent->changed_mask =
> +            static_cast<GdkWindowState>(aEvent->changed_mask & ~GDK_WINDOW_STATE_MAXIMIZED);
> +    }

We need to tell gtk_window_state_event() about the maximized state eventually so that it can appropriately draw and handle input on the window buttons it adds when in client-side-decorated (CSD) mode.

CSD is only used on some systems.  You probably did not notice any problems because the window manager added these buttons on your system.

I expect this should work:

    } else if (aEvent->changed_mask & GDK_WINDOW_STATE_WITHDRAWN &&
               aEvent->new_window_state & GDK_WINDOW_STATE_MAXIMIZED) {
        aEvent->changed_mask = static_cast<GdkWindowState>
            (aEvent->changed_mask & GDK_WINDOW_STATE_MAXIMIZED);
    }
Attachment #8962913 - Flags: review?(karlt)

Comment 19

a year ago
mozreview-review-reply
Comment on attachment 8962913 [details]
Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

https://reviewboard.mozilla.org/r/231714/#review244790

> We need to tell gtk_window_state_event() about the maximized state eventually so that it can appropriately draw and handle input on the window buttons it adds when in client-side-decorated (CSD) mode.
> 
> CSD is only used on some systems.  You probably did not notice any problems because the window manager added these buttons on your system.
> 
> I expect this should work:
> 
>     } else if (aEvent->changed_mask & GDK_WINDOW_STATE_WITHDRAWN &&
>                aEvent->new_window_state & GDK_WINDOW_STATE_MAXIMIZED) {
>         aEvent->changed_mask = static_cast<GdkWindowState>
>             (aEvent->changed_mask & GDK_WINDOW_STATE_MAXIMIZED);
>     }

aEvent->changed_mask | GDK_WINDOW_STATE_MAXIMIZED
Comment hidden (mozreview-request)

Comment 21

a year ago
mozreview-review
Comment on attachment 8962913 [details]
Bug 1449166: Trick GTK into using values from gtk_window_resize when showing maximized windows.

https://reviewboard.mozilla.org/r/231714/#review245548

::: widget/gtk/nsWindow.cpp:3330
(Diff revision 3)
>          }
>          return;
>      }
>      // else the widget is a shell widget.
>  
> +    // The block below is a bit evil.

Aye, it is.
Attachment #8962913 - Flags: review?(karlt) → review+
Keywords: regression
Priority: -- → P1
emilio: can you give us an eta for landing this patch?
Flags: needinfo?(emilio)
(Assignee)

Comment 23

a year ago
I triggered autoland, but it's closed... So whenever it reopens.
Flags: needinfo?(emilio)

Comment 24

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c2f10e92f864
Trick GTK into using values from gtk_window_resize when showing maximized windows. r=karlt
https://hg.mozilla.org/mozilla-central/rev/c2f10e92f864
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Whiteboard: [stockwell needswork] → [stockwell fixed:product]
Duplicate of this bug: 1453554
Depends on: 1458860
(Assignee)

Comment 27

a year ago
Filed https://gitlab.gnome.org/GNOME/gtk/issues/1044 on the underlying GTK issue / limitation.
You need to log in before you can comment on or make changes to this bug.