Closed Bug 1446264 Opened 7 years ago Closed 7 years ago

Browser is opened in normal sizemode instead of previous maximized sizemode when restart browser with the patches for bug 1439875

Categories

(Core :: DOM: Core & HTML, defect, P1)

61 Branch
All
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 + verified

People

(Reporter: alice0775, Assigned: xidorn)

References

(Depends on 1 open bug)

Details

(Keywords: nightly-community, regression)

Attachments

(5 files, 2 obsolete files)

[Tracking Requested - why for this release]: Reproducible: always Steps To Re@roduce: 1. Maximize Browser 2. Exit Browser 3. Re-launch Browser Actual Results: Browser is opened with normal sizemode instead of maximized sizemode Expected Results: Browser should start in maximized sizemode. Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cad403cafed9b17d1b801837abcd039a0bce82d5&tochange=990a8eb972cd2a734da04ed8d787564752e8c9c7 Regressed by: Bug 1439875 I was surprised that there was no automated test. @:emilio Could you look into this?
Flags: needinfo?(emilio)
Component: XUL → DOM
I'm surprised about this too... Probably we don't really have much tests around sessionrestore.
Another thing I noticed on Linux (previous to Bug 1439875) is that the window shows in the normal mode, then maximizes. This happens only for the first window, but still looks and feels janky. Is it supposed to work like that?
Priority: -- → P1
Summary: Browser is opened in normal sizemode instead of previous maximized sizemode when restart browser → stylo: Browser is opened in normal sizemode instead of previous maximized sizemode when restart browser
Summary: stylo: Browser is opened in normal sizemode instead of previous maximized sizemode when restart browser → Browser is opened in normal sizemode instead of previous maximized sizemode when restart browser
Further info. Open the browser - maximize the browse Using the 'Hamburger' click 'Open New Window' - it opens in small window, not mazimized even though the main window is already. New Private window same as above, small window.
What's your OS? STR in comment 3 don't work for me on Linux (new window starts off maximized).
Flags: needinfo?(emilio)
This is fixed by backout in any case, but I need to look into this before relanding of course...
Summary: Browser is opened in normal sizemode instead of previous maximized sizemode when restart browser → Browser is opened in normal sizemode instead of previous maximized sizemode when restart browser with the patches for bug 1439875
I reproduced it on Windows I think.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #4) > What's your OS? STR in comment 3 don't work for me on Linux (new window > starts off maximized). Sorry, I'm on Windows 10 x64
Latest Nightly is completely unusable for me (on Linux). It starts up non-maximized as noted above, but it also doesn't receive click events at all, so I can't use it for anything. It behaves as if its transparent to mouse events - when I click on it the terminal window behind it is raised to the top. Kbd commands appears to work though.
Severity: normal → critical
Yes, that's bug 1446242. Unfortunately "on Linux" happens to be quite broad and I can't repro on Fedora 27, so I'm trying to get a distro version that repros the problem.
(Not critical since the offending patches were backed out.) My distro is Kubuntu 17.10.
Severity: critical → normal
Patches from bug #1439875 relanded and this bug is back.
Severity: normal → major
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(emilio)
Hardware: Unspecified → All
(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #11) > Patches from bug #1439875 relanded and this bug is back. Yes, I know, sorry about that. This is a win-only issue, so putting those patches back in the tree seemed worth it to avoid people breaking them in the meantime. I cannot investigate on my normal laptop, but will investigate on Monday if Xidorn hasn't got around to it. I suspect this is running earlier than expected on windows now (before mChromeLoaded), and thus we end up not persisting the size mode: https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/xpfe/appshell/nsWebShellWindow.cpp#489 Or perhaps we just need to set mPersistentAttributeMask in BeforeStartLayout: https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/xpfe/appshell/nsXULWindow.cpp#1095 The other thing we can do is landing a partial backout of bug 1439875, by commenting / ifdef'ing out these two lines: https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/xpfe/appshell/nsXULWindow.cpp#2244-2245 But I'm not sure this is severe enough to warrant it.
I'll have a look Monday.
This is because of code in nsXULWindow::SetSize[1] and nsXULWindow::SetPositionAndSize[2] whose comment states that: > If we're called before the chrome is loaded someone obviously wants this > window at this size & in the normal size mode (since it is the only mode > in which setting dimensions makes sense). We don't persist this one-time > size. and they set mIgnoreXULSizeMode to true. And later when we try to restore the size mode in nsXULWindow::LoadMiscPersistentAttributesFromXUL we skip it because of that[3]. The comment apparently no longer makes sense because we are always setting size before chrome is loaded since bug 1439875. I don't have idea immediately how should this be fixed. A quick and dirty way might be adding another flag and set it to true during nsXULWindow::SizeShell, and avoid setting those ignore flags when we are calling into them from SizeShell. I'm not sure whether there is more sensible way to fix this. [1] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/xpfe/appshell/nsXULWindow.cpp#619-627 [2] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/xpfe/appshell/nsXULWindow.cpp#652-659 [3] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/xpfe/appshell/nsXULWindow.cpp#1330-1343
Flags: needinfo?(xidorn+moz)
So at one point I had a patch that basically replaced those mIgnore* flags for a scoped boolean mSizingShell that was set during SizeShell. It may be worth doing that and using it? That'd catch both sizes, and I think makes sense.
I don't think they should be replaced, though. If someone invokes those methods after BeforeStartLayout but before OnChromeLoaded, we may not want to override that in OnChromeLoaded which calls the various sizing bits again.
Well, I guess that's pretty much the same idea from comment 15. I don't think there's a more sensible way to do this either.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #17) > I don't think they should be replaced, though. If someone invokes those > methods after BeforeStartLayout but before OnChromeLoaded, we may not want > to override that in OnChromeLoaded which calls the various sizing bits again. Oh, yeah, I meant converting those bits to something like if (!mSizingShell && !mChromeLoaded)
OK, then let's try this...
Assignee: nobody → xidorn+moz
Hmmm... the new test times out on Linux... Why GTK widget doesn't trigger resize event when restoring :/ And it causes extra reflows in browser_windowopen_reflows.js on Windows? And unexpected pass in browser_startup_images.js on Linux?
Actually... the failures in browser_windowopen_reflows.js sound very familiar... I think last time when I saw them in try push, they disappeared when it gets landed... Also I cannot reproduce that failure locally...
When changing this relatively fragile initialization of the sizemode, could you please verify that you are not breaking the case of startup where browser.startup.blankWindow is set to true? It took some hacking to get it to work https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/browser/components/nsBrowserGlue.js#52-65 and bug 1434986.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #24) > And unexpected pass in browser_startup_images.js on Linux? Despite being labelled "unexpected pass" by the test, this sounds like regression. This test is reporting when we waste time by loading an image that we are not painting. The unexpected pass here means that we are now actually painting this chevron.svg image. This suggests we are now painting the window at a tiny size at which the navigator toolbar overflows, before resizing it to the correct size.
(In reply to Florian Quèze [:florian] from comment #26) > When changing this relatively fragile initialization of the sizemode, could > you please verify that you are not breaking the case of startup where > browser.startup.blankWindow is set to true? It took some hacking to get it > to work > https://searchfox.org/mozilla-central/rev/ > 3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/browser/components/nsBrowserGlue. > js#52-65 and bug 1434986. It doesn't break blankWindow in the sense that the window is opened maximized, but I cannot really tell whether everything works as expected, because it reliably crashes for me locally with or without this patch for some assertion in timestamp... (In reply to Florian Quèze [:florian] from comment #27) > Despite being labelled "unexpected pass" by the test, this sounds like > regression. I guess we can just mark that as intermittentShown like what we've done in bug 1439875 for Windows.
The Windows failure is pretty weird. Its stack is > rect@chrome://browser/content/browser-tabsintitlebar.js:106:23 > _update@chrome://browser/content/browser-tabsintitlebar.js:168:28 > updateAppearance@chrome://browser/content/browser-tabsintitlebar.js:61:5 > handleEvent@chrome://browser/content/tabbrowser.xml:761:15 > EventListener.handleEvent*tabbrowser-tabs_XBL_Constructor@chrome://browser/content/tabbrowser.xml:127:11 > toolbar_XBL_Constructor@chrome://browser/content/customizableui/toolbar.xml:42:26 > toolbar_XBL_Constructor@chrome://browser/content/customizableui/toolbar.xml:31:13 > init@chrome://browser/content/browser-tabsintitlebar.js:29:16 > onBeforeInitialXULLayout@chrome://browser/content/browser.js:1233:5 > @chrome://browser/content/browser.js:1194:5 > EventListener.handleEvent*@chrome://browser/content/browser.js:1193:3 > @chrome://browser/content/browser.xul:90:3 The event handler calling updateAppearance is for resize event[1], and the resize event seems to be triggered in some random code getting element[2]. That's probably because we flush style for wrapping object for js access, which is needed for XUL elements due to bindings. The code that triggers the toolbar ctor is also just getting element[3]. The most weird thing is that, the whole stack is invoked from onBeforeInitialXULLayout which is actually dispatched before the code path touched here[4], so there should be no chance that it causes issue... [1] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/tabbrowser.xml#760 [2] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/browser/components/customizableui/content/toolbar.xml#42 [3] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/browser-tabsintitlebar.js#29 [4] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/dom/xul/XULDocument.cpp#2747-2752
Another interesting thing is that, in the log, onBeforeInitialXULLayout appears later than the expected reflow from onDOMContentLoaded. Are we calling into onBeforeInitialXULLayout unnecessarily after onDOMContentLoaded? Codewise, it seems we are definitely dispatching DOMContentLoaded after BeforeInitialXULLayout[1]... [1] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/dom/xul/XULDocument.cpp#2779-2781
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #30) > Another interesting thing is that, in the log, onBeforeInitialXULLayout > appears later than the expected reflow from onDOMContentLoaded. Are we > calling into onBeforeInitialXULLayout unnecessarily after onDOMContentLoaded? Oh that's unrelated. The test itself is printing known reflows first and then unexpected. So we would always see unexpected coming in a later place.
Actually... logically this shouldn't happen if onBeforeInitialXULLayout is triggered before onDOMContentLoaded because in that case, TabsInTitlebar._domLoaded should be false, and any call into TabsInTitlebar._update() other than the one from TabsInTitlebar.init() should've been guarded[1]. This makes things more mysterious :/ [1] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/browser-tabsintitlebar.js#116-118
I put some test code in involving js code, and see the following result: > onBeforeInitialXULLayout > TabsInTitlebar._update: domLoaded = false, fromInit = true > onDOMContentLoaded > TabsInTitlebar._update: domLoaded = false, fromInit = false > TabsInTitlebar._update: domLoaded = false, fromInit = false > TabsInTitlebar.onDOMContentLoaded > TabsInTitlebar._update: domLoaded = true, fromInit = false > TabsInTitlebar._update: domLoaded = true, fromInit = false So TabsInTitlebar._update is invoked after onDOMContentLoaded twice. In my local machine, I can only see one line of that after TabsInTitlebar.onDOMContentLoaded, so it seems the extra TabsInTitlebar._update is where the problem is.
So... I can reproduce the additional TabsInTitlebar._update call if the window is opened maximized, and that is triggered by a resize event. The resize event is triggered by two things: First, nsWindow::SetNonClientMargins invoked from nsXULWindow::SyncAttributesToWidget as part of nsXULWindow::OnChromeLoaded. It calls into UpdateNonClientMargins then ResetLayout. ResetLayout invokes SetWindowPos which triggers OnWindowPosChanged for frame change, for which we call into OnResize to trigger resize event[1]. (Actually this path was added by myself in bug 1443392. That code path is invoked when the window is not maximized as well, but because the sizemode is normal, the non-client margins aren't actually changed, and consequently resize path is terminated in nsViewManager::DoSetWindowDimensions when it finds the size isn't changed. Second, nsWindow::Show invoked from nsXULWindow::SetVisibility as part of nsXULWindow::OnChromeLoaded. It calls into ShowWindow which also triggers OnWindowPosChanged but for all of frame change, size and position, because at that moment, the window is still sized as normal, so we actually need a resize. This is probably unavoidable because we never size the window as maximized before, so when we show it, it has to trigger a resize. And oh, I just find out that this is a problem indeed because the test on infra opens the window maximized[2] (which I don't think I saw before because not all tasks have screenshot attached), so this is indeed the problem (although I would probably expect it not to be maximized, but that may be a different topic). So then the question is how can we avoid the extra resize here... We need to make the window maximized before the initial reflow... [1] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/widget/windows/nsWindow.cpp#6813 [2] https://taskcluster-artifacts.net/HlGFUbGARKybltnEBh4diQ/0/public/test_info/mozilla-test-fail-screenshot_ajmc_r.png
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #34) > So then the question is how can we avoid the extra resize here... We need to > make the window maximized before the initial reflow... Shouldn't that be already the case? I mean... I'm suspicious about OnChromeLoaded causing a resize here. The chromemargin attribute should be set much earlier (or at least that's the point of bug 1445737).
If we can have the window maximized when we call SetSizeMode, both resize triggers would disappear (that's probably because OnWindowPosChanged would be triggered which invokes UpdateNonClientMargins to update the client size, and that resize ends up being terminated in nsViewManager::DoSetWindowDimensions). I tried to use > ::ShowWindow(mWnd, SW_HIDE | SW_MAXIMIZE); in nsWindow::SetSizeMode, and it works in the sense that no extra resize shows up, however the window is not correctly positioned and sized. Probably windows cannot correctly maximize a hidden window?
Actually... the parameter of ShowWindow doesn't seem to be bit flags, and SW_HIDE is just zero, so that call ends up showing the window directly...
I tried some hacky way to have everything works, but it doesn't seem to work well. What I tried is to add the following code into nsWindow::SetSizeMode: > if (!mIsVisible) { > if (aMode == nsSizeMode_Maximized) { > DWORD style = GetWindowLong(mWnd, GWL_STYLE); > SetWindowLong(mWnd, GWL_STYLE, style | WS_MAXIMIZE); > HMONITOR monitor = MonitorFromWindow(mWnd, MONITOR_DEFAULTTONEAREST); > MONITORINFO monitorInfo; > monitorInfo.cbSize = sizeof(monitorInfo); > GetMonitorInfo(monitor, &monitorInfo); > RECT rect = monitorInfo.rcWork; > SetWindowPos(mWnd, 0, rect.left, rect.top, > rect.right - rect.left, > rect.bottom - rect.top, > SWP_FRAMECHANGED|SWP_NOACTIVATE| > SWP_NOOWNERZORDER|SWP_NOZORDER); > SetWindowLong(mWnd, GWL_STYLE, style); > } > } Setting the window style to WS_MAXIMIZE so that the system is aware of that the window is in the maximized state, and then size the window explicitly with the screen working area rectangle to trigger OnWindowPosChanged so that view manager gets the size we want at the very beginning. And then set the style back, so that the window is able to restore. The end result of this code is that the window cannot restore to the size we persist. If the last SetWindowLong is removed, then if user restore the window, it becomes some unusable small size. I guess there is some super hacky way to make everything just work... but I don't really want to try that route. Jim, do you have any advice on how can we have the window maximized before showing the window (so that we can run layout with the correct size) while still make it possible to restore to the size we want?
Flags: needinfo?(jmathies)
Another route to fixing the regression is to make the window not maximized... It doesn't seem to me that browser_windowopen_reflows.js is designed to open in maximized mode, so if we make it just open in normal mode, this regression wouldn't show up... But that also indicates that we would have worse performance depending on the mode of the window, then we may want to add test for that case as well, since always having maximized window seems to be a common practice.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #28) > (In reply to Florian Quèze [:florian] from comment #26) > > When changing this relatively fragile initialization of the sizemode, could > > you please verify that you are not breaking the case of startup where > > browser.startup.blankWindow is set to true? It took some hacking to get it > > to work > > https://searchfox.org/mozilla-central/rev/ > > 3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/browser/components/nsBrowserGlue. > > js#52-65 and bug 1434986. > > It doesn't break blankWindow in the sense that the window is opened > maximized, but I cannot really tell whether everything works as expected, > because it reliably crashes for me locally with or without this patch for > some assertion in timestamp... I assume this is on a debug build? Is it on Windows or Linux? What's the assertion? > (In reply to Florian Quèze [:florian] from comment #27) > > Despite being labelled "unexpected pass" by the test, this sounds like > > regression. > > I guess we can just mark that as intermittentShown like what we've done in > bug 1439875 for Windows. It doesn't seem intermittent on your try results so if you want to silence the failure you can just remove 'linux' from the list of platforms at https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/test/performance/browser_startup_images.js#63 But I'm assuming you want to figure out why this regresses, and not just silence the message.
Could you backout bug 1439875 until this is fixed? Fixing this bug seems to be more complex than expected first.
Comment on attachment 8960057 [details] Bug 1446264 part 4 - Add an approach to remove all persisted values of a document. https://reviewboard.mozilla.org/r/228828/#review235078 The code seems fine but I'm unclear where you want to use it. Is this just for a test?
(In reply to Neil Deakin from comment #42) > The code seems fine but I'm unclear where you want to use it. Is this just > for a test? Yes, it's just for a test.
(In reply to Masatoshi Kimura [:emk] from comment #41) > Could you backout bug 1439875 until this is fixed? Fixing this bug seems to > be more complex than expected first. Rather than backing out the patch completely, I'd rather just revert the behavior change (commenting out https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/xpfe/appshell/nsXULWindow.cpp#2264-2265). But I'd really prefer not having to unless completely necessary.
(In reply to Florian Quèze [:florian] from comment #40) > (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #28) > > I guess we can just mark that as intermittentShown like what we've done in > > bug 1439875 for Windows. > > It doesn't seem intermittent on your try results so if you want to silence > the failure you can just remove 'linux' from the list of platforms at > https://searchfox.org/mozilla-central/rev/ > 877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/test/ > performance/browser_startup_images.js#63 But I'm assuming you want to figure > out why this regresses, and not just silence the message. I have another try push with that removed from the list, but there are failures in the other direction. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=22d7a4fe25ae8f8f3909b2008f12ebca30eb4838 Actually it's weird... It seems to fail consistently either way, and that happens in different tasks... We are running this test in two tasks with different settings?
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #45) > Actually it's weird... It seems to fail consistently either way, and that > happens in different tasks... We are running this test in two tasks with > different settings? Oh, I see, we do run it twice, one for lodpi, one for hidpi. It seems the hidpi one is regressed somehow.
Comment on attachment 8960057 [details] Bug 1446264 part 4 - Add an approach to remove all persisted values of a document. https://reviewboard.mozilla.org/r/228828/#review235156
Attachment #8960057 - Flags: review?(enndeakin) → review+
If there is no easy way to have window maximized before showing up, I'd suggest we accept the regression on the mochitests, because otherwise we need to backout bug 1439875 which goes a larger step back on performance. florian, what do you think?
Flags: needinfo?(florian)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #49) > If there is no easy way to have window maximized before showing up, Why is it not easy? Isn't it just a matter of setting the sizemode attribute earlier? It seemed to work well enough for https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/browser/components/nsBrowserGlue.js#52-56 > I'd suggest we accept the regression on the mochitests, Are you saying this about the chevron.svg issue on Linux, or the reflows on Windows? Or both? I think the linux issue is acceptable to whitelist, the Windows one I'm less comfortable.
Flags: needinfo?(florian)
(In reply to Florian Quèze [:florian] from comment #50) > (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #49) > > If there is no easy way to have window maximized before showing up, > > Why is it not easy? Isn't it just a matter of setting the sizemode attribute > earlier? It seemed to work well enough for > https://searchfox.org/mozilla-central/rev/ > b29daa46443b30612415c35be0a3c9c13b9dc5f6/browser/components/nsBrowserGlue. > js#52-56 It's... not an issue inside the shared code. It is an issue inside widget code, or more specifically, this is probably limitation from the system. The problem is that, on Windows, when we invoke nsWindow::SetSizeMode for syncing the sizemode to the window, if the window hasn't been visible, we just record the new sizemode without telling the system anything, and thus the system doesn't trigger the native resize (OnWindowPosChanged) to resize stuff. The native resize for maximizing is only triggered when we eventually show the window in maximized state in nsWindow::Show via Windows API ShowWindow, but that is too late since we don't want to display window before we finish layout. A hacky workaround I tried in comment 38 is that we manually set window size to the screen to trigger the native resize forcefully, but that way the system would lose the track of the restored size. > > I'd suggest we accept the regression on the mochitests, > > Are you saying this about the chevron.svg issue on Linux, or the reflows on > Windows? Or both? > > I think the linux issue is acceptable to whitelist, the Windows one I'm less > comfortable. I mean both.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #51) > The problem is that, on Windows, when we invoke nsWindow::SetSizeMode for > syncing the sizemode to the window, if the window hasn't been visible, we > just record the new sizemode without telling the system anything, and thus > the system doesn't trigger the native resize (OnWindowPosChanged) to resize > stuff. To be clear, I don't see anything via which we can ask the system to maximize an invisible window. There is a flag for making window open in maximized state, but setting that flag doesn't trigger the native resize message, so it's not helpful in this case at all.
Flags: needinfo?(florian)
Alternatively, we can try to have the test not maximize the window so that it wouldn't see the extra resize... I suppose it wasn't designed to catch that at all. It just happens to be opened maximized.
(In reply to Florian Quèze [:florian] from comment #40) > I assume this is on a debug build? Is it on Windows or Linux? What's the > assertion? Filed bug 1447549 for that, since it isn't quite related to this bug.
Hmm, let's just display the window earlier. It doesn't seem to break other things, and we'll see if that breaks other stuff...
Flags: needinfo?(jmathies)
Flags: needinfo?(florian)
I mentioned this in one of the related issues, but it might have been overlooked. What's the story on Linux? I'm running Arch Linux with GNOME Shell/Wayland/CSD, and on two computers the Firefox window starts in the restored (normal) state, then gets maximized. This happened even before bug #1439875. Is that the expected behaviour? Actually, I just tested, and it seems a bit worse: a maximized window flashes as black, then Firefox gets painted in the restored state, and finally it gets maximized again. On another computer, the restored window has its margins flash to black, but I'm not sure about the initial maximized one.
I actually have no idea why Linux can continue to work as expected after bug 1439875... with my patchset (not published in this bug yet because I want to check the try result first), there is a chance that the problem you described on Linux can be resolved as well.
Thanks. I'll wait until this gets fixed and things stabilize for a bit, then test again and file a new issue.
Hmmm, a little bit better than previous but still have extra reflow: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dee72ae566a78374d5d7856a417379f5e2643ebc This time it's select@chrome://global/content/bindings/textbox.xml
So the new extra reflow happens because my windows patch makes the window show eagerly (to avoid the resize reflow for maximizing), and consequently it steals the focus from the old window. Before that change, when we want to focus the url bar, the focus event is not dispatched because the window hasn't been displayed at that moment, and thus has no focus. But after that change, the window now has focus, so some more event is dispatched and more code run to trigger an extra reflow.
Comment on attachment 8960933 [details] Bug 1446264 part 1 - Mark chevron.svg intermittent shown in startup_images test. https://reviewboard.mozilla.org/r/229668/#review235444
Attachment #8960933 - Flags: review?(florian) → review+
Comment on attachment 8960934 [details] Bug 1446264 part 2 - Ignore XUL position when sizemode is maximized. https://reviewboard.mozilla.org/r/229670/#review235448 ::: xpfe/appshell/nsXULWindow.cpp:1371 (Diff revision 1) > } else { > - // For maximized windows, ignore the XUL size attributes, as setting the > - // size would set the window back to the normal sizemode. > + // For maximized windows, ignore the XUL size and position attributes, > + // as setting them would set the window back to normal sizemode. > if (sizeMode == nsSizeMode_Maximized) { > mIgnoreXULSize = true; > + mIgnoreXULPosition = true; Why is this correct? In virtual-desktop setups, can't you have a maximized window with nonzero position?
Comment on attachment 8960056 [details] Bug 1446264 part 3 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell(). https://reviewboard.mozilla.org/r/228826/#review235454 r=me ::: commit-message-be113:3 (Diff revision 2) > +Bug 1446264 part 5 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell(). r?bz > + > +When SetSize etc. are called while OnChromeLoaded hasn't been invoked, "are called before OnChromeLoaded has been invoked" ::: commit-message-be113:10 (Diff revision 2) > +the window specifically, and thus ignore the values from the <window> > +element. > + > +However, bug 1439875 changes the behavior so that SizeShell is also > +invoked before OnChromeLoaded, which confuses the functions above, and > +causes some of the attributes not loaded properly. "to not be loaded" ::: commit-message-be113:12 (Diff revision 2) > + > +However, bug 1439875 changes the behavior so that SizeShell is also > +invoked before OnChromeLoaded, which confuses the functions above, and > +causes some of the attributes not loaded properly. > + > +This patch add a separate flag to avoid ignoring those attributes when "adds" ::: xpfe/appshell/nsXULWindow.cpp:582 (Diff revision 2) > > NS_IMETHODIMP nsXULWindow::SetPositionDesktopPix(int32_t aX, int32_t aY) > { > mWindow->Move(aX, aY); > + if (mSizingShellFromXUL) { > + // If we're invoked for sizing from XUL, we want neither ignore anything "want to neither" ::: xpfe/appshell/nsXULWindow.cpp:626 (Diff revision 2) > > DesktopToLayoutDeviceScale scale = mWindow->GetDesktopToDeviceScale(); > DesktopSize size = LayoutDeviceIntSize(aCX, aCY) / scale; > mWindow->Resize(size.width, size.height, aRepaint); > + if (mSizingShellFromXUL) { > + // If we're invoked for sizing from XUL, we want neither ignore anything "want to neither" ::: xpfe/appshell/nsXULWindow.cpp:664 (Diff revision 2) > DesktopToLayoutDeviceScale scale = mWindow->GetDesktopToDeviceScale(); > DesktopRect rect = LayoutDeviceIntRect(aX, aY, aCX, aCY) / scale; > mWindow->Resize(rect.X(), rect.Y(), rect.Width(), rect.Height(), > !!(aFlags & nsIBaseWindow::eRepaint)); > + if (mSizingShellFromXUL) { > + // If we're invoked for sizing from XUL, we want neither ignore anything "want to neither"
Comment on attachment 8960056 [details] Bug 1446264 part 3 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell(). https://reviewboard.mozilla.org/r/228826/#review235456
Attachment #8960056 - Flags: review?(bzbarsky) → review+
Comment on attachment 8960934 [details] Bug 1446264 part 2 - Ignore XUL position when sizemode is maximized. https://reviewboard.mozilla.org/r/229670/#review235448 > Why is this correct? In virtual-desktop setups, can't you have a maximized window with nonzero position? At least Windows and GTK widget don't agree with you: * https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/widget/windows/nsWindow.cpp#1793-1796 * https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/widget/gtk/nsWindow.cpp#1190-1193
Comment on attachment 8960058 [details] Bug 1446264 part 5 - Add test for this bug. https://reviewboard.mozilla.org/r/228824/#review235462 ::: toolkit/content/tests/chrome/test_maximized_persist.xul:15 (Diff revision 2) > + src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"/> > +<script class="testbody" type="application/javascript"><![CDATA[ > + SimpleTest.waitForExplicitFinish(); > + const WIDTH = 300; > + const HEIGHT = 300; > + const STATE_MAXIMIZED = 1; I assume you need to hardcode this because this is not a ChromeWindow? If so, please document that. ::: toolkit/content/tests/chrome/test_maximized_persist.xul:39 (Diff revision 2) > + null, "chrome,dialog=no,all," + features); > + } > + > + function checkWindow(msg, win, sizemode, width, height) { > + is(win.windowState, sizemode, "sizemode should match " + msg); > + if (sizemode == STATE_NORMAL) { Couls you use win.STATE_NORMAL instead of the hardcoded thing? ::: toolkit/content/tests/chrome/test_maximized_persist.xul:74 (Diff revision 2) > + let win = openWindow(); > + await SimpleTest.promiseFocus(win); > + > + // Check the default state. > + const chrome_url = win.location.href; > + checkWindow("when open initially", win, STATE_NORMAL, WIDTH, HEIGHT); And here, can you get STATE_NORMAL from "win"? ::: toolkit/content/tests/chrome/test_maximized_persist.xul:79 (Diff revision 2) > + checkWindow("when open initially", win, STATE_NORMAL, WIDTH, HEIGHT); > + const widthDiff = win.outerWidth - win.innerWidth; > + const heightDiff = win.outerHeight - win.innerHeight; > + // Maximize the window. > + await changeSizeMode(() => win.maximize()); > + checkWindow("after maximize window", win, STATE_MAXIMIZED); And here. And through the rest of the function.
Attachment #8960058 - Flags: review?(bzbarsky) → review+
Comment on attachment 8960934 [details] Bug 1446264 part 2 - Ignore XUL position when sizemode is maximized. https://reviewboard.mozilla.org/r/229670/#review235470
Attachment #8960934 - Flags: review?(bzbarsky) → review+
Comment on attachment 8960058 [details] Bug 1446264 part 5 - Add test for this bug. https://reviewboard.mozilla.org/r/228824/#review235462 > Couls you use win.STATE_NORMAL instead of the hardcoded thing? Oh, that works. Thanks!
Comment on attachment 8960935 [details] Bug 1446264 part 3 - Allow window to show earlier when sizemode is changed before window turned visible. https://reviewboard.mozilla.org/r/229672/#review235608 lgtm
Attachment #8960935 - Flags: review?(jmathies) → review+
Comment on attachment 8960936 [details] Bug 1446264 part 4 - Delay handling of WM_SETFOCUS after we explicit show the window. https://reviewboard.mozilla.org/r/229674/#review235610 This is risky as it may have unexpected side effects. For example, once we set focus to a window it's assumed the dom state will be in the correct focus state. You're delaying sending the focus event to gecko which could cause odd race issues revolving around focus state. Find with landing htis to see but lets keep an eye out for regression bugs.
Attachment #8960936 - Flags: review?(jmathies) → review+
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/38995f6de9df part 1 - Mark chevron.svg intermittent shown in startup_images test. r=florian https://hg.mozilla.org/integration/autoland/rev/f9f4a91b3edb part 2 - Ignore XUL position when sizemode is maximized. r=bz https://hg.mozilla.org/integration/autoland/rev/8e3dd0825df9 part 3 - Allow window to show earlier when sizemode is changed before window turned visible. r=jimm https://hg.mozilla.org/integration/autoland/rev/2280d7116066 part 4 - Delay handling of WM_SETFOCUS after we explicit show the window. r=jimm https://hg.mozilla.org/integration/autoland/rev/27efbd3b9218 part 5 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell(). r=bz https://hg.mozilla.org/integration/autoland/rev/af69031d49a0 part 6 - Add an approach to remove all persisted values of a document. r=enndeakin+6102 https://hg.mozilla.org/integration/autoland/rev/7b9a6f304c95 part 7 - Add test for this bug. r=bz
Backed out 7 changesets (bug 1446264) for wpt failures at html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-height.html Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7b9a6f304c95ba963b76e15bf6e05d26a390e23f Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=169562742&repo=autoland&lineNumber=1841 Backout: https://hg.mozilla.org/integration/autoland/rev/22e482785671fb41d2e97ca29d328c2a01c5c715
Flags: needinfo?(xidorn+moz)
So... part 3 does cause problem. We cannot show the window earlier, because we may restore its sizemode to normal before we start painting due to e.g. window.open with top/left/width/height, and that could lead to some flicking, which should be considered a regression itself. Whether it should affect the wpt is different question, though. The new test added should have also caught that, but probably an empty window is rendered too fast so it doesn't look that bad, but browser.xul is much heavier. I guess it is probably impossible to workaround the reflow for maximizing on Windows, then...
On the other hand, the fact that the resizing for window feature happens after the initial layout also indicates we are wasting a resize reflow for that case as well. It's probably something we should improve as well.
Depends on: 1447864
The patches were backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla61 → ---
FYI, after landing Bug 1447719, first window is opened in maximized mode as expected, but second new window is still opened in normal mode. If setting browser.startup.blankWindow=fasle, the first window is still opened in normal mode.
So currently we are in a state that even backing out the XUL sizing patch is non-trivial... Nothing is trivial forward... The window open path is so complicated :(
So, per discussion with emilio and florian, I think the best way forward currently is to land this patch with the extra reflows whitelisted. We can probably fix bug 1447864 then land the two windows widget patches to remove those extra reflows.
Flags: needinfo?(xidorn+moz)
Depends on: 1448199
No longer depends on: 1447864
Attachment #8960935 - Attachment is obsolete: true
Attachment #8960936 - Attachment is obsolete: true
Comment on attachment 8960056 [details] Bug 1446264 part 3 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell(). https://reviewboard.mozilla.org/r/228826/#review236098 Bug 1443578 renamed all the browser_*_reflows.js tests as they now also cover flicker. ::: browser/base/content/test/performance/browser_windowopen_reflows.js:33 (Diff revision 4) > "onDOMContentLoaded@chrome://browser/content/browser.js", > ], > maxCount: 2, // This number should only ever go down - never up. > }, > + > + // The reflows below are triggered by maximizing the window after How much effort would it take to push these 2 whitelist entries to the list of exceptions only when the test window is in the maximized state? I think it should be straightforward. Of course a better solution would be to duplicate the test to have one instance of the test always opening maximized windows, and one instance of the test always opening non-maximized windows. But I don't want to block this bug on that test enhancement. ::: browser/base/content/test/performance/browser_windowopen_reflows.js:41 (Diff revision 4) > + stack: [ > + "rect@chrome://browser/content/browser-tabsintitlebar.js", > + "_update@chrome://browser/content/browser-tabsintitlebar.js", > + "updateAppearance@chrome://browser/content/browser-tabsintitlebar.js", > + "handleEvent@chrome://browser/content/tabbrowser.xml", > + "EventListener.handleEvent*tabbrowser-tabs_XBL_Constructor@chrome://browser/content/tabbrowser.xml", This line containing the '*' character won't exist in the stack on beta and later, so your patch here will make this test fail when 61 will merge to beta. Please remove this line and stop the stack after the "handleEvent@chrome://browser/content/tabbrowser.xml" line. ::: browser/base/content/test/performance/browser_windowopen_reflows.js:51 (Diff revision 4) > + stack: [ > + "verticalMargins@chrome://browser/content/browser-tabsintitlebar.js", > + "_update@chrome://browser/content/browser-tabsintitlebar.js", > + "updateAppearance@chrome://browser/content/browser-tabsintitlebar.js", > + "handleEvent@chrome://browser/content/tabbrowser.xml", > + "EventListener.handleEvent*tabbrowser-tabs_XBL_Constructor@chrome://browser/content/tabbrowser.xml", Same here.
Attachment #8960056 - Flags: review?(florian) → review-
Comment on attachment 8960056 [details] Bug 1446264 part 3 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell(). https://reviewboard.mozilla.org/r/228826/#review236098 > How much effort would it take to push these 2 whitelist entries to the list of exceptions only when the test window is in the maximized state? I think it should be straightforward. > > Of course a better solution would be to duplicate the test to have one instance of the test always opening maximized windows, and one instance of the test always opening non-maximized windows. But I don't want to block this bug on that test enhancement. I doubt how straightforward that could be, because it really depends on when exactly do we maximize the window. It could happen inside openDialog, or it could happen after. Since I don't have a screen with such small resolution to force the window enter maximized state, I have no idea when that happens. If it happens after openDialog, there is no way in the current setup that we can push it into the list, because the `with*Observer` function wants us to pass the list in before the window opens. Even if we know, I suspect the timing could actually be very sensitive to code change in the open window path, and depending on that may make the test even more fragile. Given that we may be able to remove them soonish, I don't want to waste much time on investigating this.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #101) > I doubt how straightforward that could be I was thinking you would just check for the sizemode attribute near the end of the test. > the `with*Observer` function wants us to > pass the list in before the window opens. It's only a reference to the list, you can still change it until the end of the test (since bug 1438233 where I changed reflow tests to collect all stacks during the test and analyze them at the end of the test).
OK, I cannot make it today, then. I don't think I want to wait for another hour for the build to finish and test whether that setting works near 1am.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #103) > OK, I cannot make it today, then. I don't think I want to wait for another > hour for the build to finish and test whether that setting works near 1am. Even if we add the whitelist entries unconditionally with the assumption that you'll be removing them soon anyway, the tests have changed enough recently that it would be safer to do another try run before landing.
Sure. I just wanted to test it locally before pushing to try run in case there is any silly small mistake (which I usually make especially in late night). (And at this point, I don't want to touch my dev laptop this weekend, so let's wait for Monday.)
Blocks: 1448760
Comment on attachment 8960056 [details] Bug 1446264 part 3 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell(). https://reviewboard.mozilla.org/r/228826/#review236538 Thanks for restricting this to maximized windows only :-). ::: browser/base/content/test/performance/browser_windowopen.js:118 (Diff revision 5) > subject => subject == win); > > await BrowserTestUtils.firstBrowserLoaded(win, false); > await BrowserTestUtils.browserStopped(win.gBrowser.selectedBrowser, "about:home"); > > + if (Services.appinfo.OS == "WINNT" && win.windowState == win.STATE_MAXIMIZED) { Nit: we usually use AppConstants.platform == "win" rather than Services.appinfo.OS.
Attachment #8960056 - Flags: review?(florian) → review+
(In reply to Florian Quèze [:florian] from comment #112) > > + if (Services.appinfo.OS == "WINNT" && win.windowState == win.STATE_MAXIMIZED) { > > Nit: we usually use AppConstants.platform == "win" rather than > Services.appinfo.OS. Sorry, I just noticed this file is already full of Services.appinfo.OS uses. Feel free to ignore this comment, I'll cleanup in a follow-up.
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/58de007dbd7d part 1 - Mark chevron.svg intermittent shown in startup_images test. r=florian https://hg.mozilla.org/integration/autoland/rev/78fa8ca081e0 part 2 - Ignore XUL position when sizemode is maximized. r=bz https://hg.mozilla.org/integration/autoland/rev/6e74eff6b09d part 3 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell(). r=bz,florian https://hg.mozilla.org/integration/autoland/rev/6acd877b6f28 part 4 - Add an approach to remove all persisted values of a document. r=enndeakin+6102 https://hg.mozilla.org/integration/autoland/rev/6dbe66440bd2 part 5 - Add test for this bug. r=bz
Thanks a lot for fixing this Xidorn :)
Flags: needinfo?(emilio)
Not fixed yet on 61.0a1 (2018-03-26) (64-bit)
I tested locally on 61.0a1 (2018-03-26) (64-bit) on macOS and it seems to me this has been fixed. Note that you need to maximize the window once to have it work, otherwise the normal state from last close may have been persisted. If the issue is still there, mind telling what OS are you using?
61.0a1 (2018-03-26) (64-bit) Win Ctrl + shift + J to open Browser console it is small Maximise Close Ctrl + shift + J to open Browser console it is still small F12 to open Browser console it is small Maximise Close F12 to open Browser console it is still small History opens up maximised
> 61.0a1 (2018-03-26) (64-bit) Win That build presumably predates this fix being merged (which happened on the afternoon of the 26th, Pacific time). To make sure, please say what the revision id in about:buildconfig is.
Yep, I can reproduce the problem with browser and web console on Windows (but not macOS), while the browser itself can be opened in maximized correctly. That's probably worth a separate bug as this bug is mainly for browser window.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #124) > > Built from https://hg.mozilla.org/mozilla-central/rev/de32269720d056972b85f4eec5f0a8286de6e3af > > OK, that does have the changes from this bug. alreay filed bug 1448760
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #119) > I tested locally on 61.0a1 (2018-03-26) (64-bit) on macOS and it seems to me > this has been fixed. > > Note that you need to maximize the window once to have it work, otherwise > the normal state from last close may have been persisted. > > If the issue is still there, mind telling what OS are you using? (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #123) > Yep, I can reproduce the problem with browser and web console on Windows > (but not macOS), while the browser itself can be opened in maximized > correctly. > > That's probably worth a separate bug as this bug is mainly for browser > window. On OS X, still reproducing with browser windows, but only private navigation windows. Normal windows correctly open up maximized. And before this fix, private windows were opening in a very smallsize, now it's at maximum height, but close to maximum width: there's still some margin/space on the left.
(In reply to Clément Lefèvre from comment #127) > On OS X, still reproducing with browser windows, but only private navigation > windows. Normal windows correctly open up maximized. > And before this fix, private windows were opening in a very smallsize, now > it's at maximum height, but close to maximum width: there's still some > margin/space on the left. Sounds like it's worth a separate bug as well.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #128) > (In reply to Clément Lefèvre from comment #127) > > On OS X, still reproducing with browser windows, but only private navigation > > windows. Normal windows correctly open up maximized. > > And before this fix, private windows were opening in a very smallsize, now > > it's at maximum height, but close to maximum width: there's still some > > margin/space on the left. > > Sounds like it's worth a separate bug as well. I was currently adding it as a comment on bug 1448760, why doesn't this fit? Btw, people restricted bug 1448760 to Windows while it still looks like it affect other OS.
(In reply to Clément Lefèvre from comment #129) > (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #128) > > (In reply to Clément Lefèvre from comment #127) > > > On OS X, still reproducing with browser windows, but only private navigation > > > windows. Normal windows correctly open up maximized. > > > And before this fix, private windows were opening in a very smallsize, now > > > it's at maximum height, but close to maximum width: there's still some > > > margin/space on the left. > > > > Sounds like it's worth a separate bug as well. > > I was currently adding it as a comment on bug 1448760, why doesn't this fit? > Btw, people restricted bug 1448760 to Windows while it still looks like it > affect other OS. That's about the browser console and web console, not about the browser window itself. And I cannot reproduce that issue on browser console or web console on macOS, so I believe that's an issue specific to Windows. What you described sounds like something related to the browser window itself, and it happens on macOS, so I'd suggest they are different issues unless you can prove they are the same. We can always duplicate them if we find they are actually the same.
Blocks: 1449111
(In reply to Clément Lefèvre from comment #129) > (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #128) > > (In reply to Clément Lefèvre from comment #127) > > > On OS X, still reproducing with browser windows, but only private navigation > > > windows. Normal windows correctly open up maximized. > > > And before this fix, private windows were opening in a very smallsize, now > > > it's at maximum height, but close to maximum width: there's still some > > > margin/space on the left. > > > > Sounds like it's worth a separate bug as well. > > I was currently adding it as a comment on bug 1448760, why doesn't this fit? > Btw, people restricted bug 1448760 to Windows while it still looks like it > affect other OS. Filed follow-up bug as bug 1449111. I used same component as this one, but I'm kinda unsure about this one.
Depends on: 1449166
Depends on: 1447859
Depends on: 1453554
Bug 1458860 can be a result of this one. I'm not sure and adding as dependency. Please remove it if you think that's completely unrelated.
Depends on: 1458860
No longer depends on: 1458860
Hello, in Nighlty this problem back again one or two days ago, have you any test to catch that bug?
Flags: needinfo?(xidorn+moz)
That's bug 1460639 regressed by bug 1453788.
Flags: needinfo?(xidorn+moz)
Flags: qe-verify+
I managed to reproduce the initial issues on 61.0a1 (2018-03-24). I can confirm that 61.0b6 build1 (20180517141400) is verified fixed across platforms (Windows 10 x64, Ubuntu 16.04 x64, macOS 10.13.4): the maximized sizemode is preserved between different windows and different sessions.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: