Cannot change window size mode and window controls disappear after restarting in full screen mode

VERIFIED FIXED in Firefox 34

Status

()

Core
Widget: Win32
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Alice0775 White, Assigned: Gijs)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {regression, ux-mode-error})

29 Branch
mozilla36
x86_64
Windows 7
regression, ux-mode-error
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox32 wontfix, firefox33+ wontfix, firefox34+ verified, firefox35+ verified, firefox36 verified, firefox-esr31+ affected)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
[Tracking Requested - why for this release]: regression

Steps To Reproduce:
1. Key press F11 into fullscreen
2. Ctrl+w to quit browser
3. Restart Browser
   --- Restored previous window size mode(fullscreen) as expected
4. Key press F11 to exit fullscreen

Actual Results:
 Cannot change window size mode.
 No window control buttons

Expected Results:
 The window size mode should change to normal mode.


Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/beddd6d4bcdf
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131118025935
Bad:
http://hg.mozilla.org/mozilla-central/rev/f2adb62d07eb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131118075715
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=beddd6d4bcdf&tochange=f2adb62d07eb
(Reporter)

Comment 1

3 years ago
Add step

Steps To Reproduce:
0. Enabled Title bar
(Assignee)

Comment 2

3 years ago
Alice, is there any chance you'd be able to look at when this regressed on the UX branch? I believe you can use "-r ux" with mozregression to use it against the ux branch. The branch was reset in April 2013 so presumably this happened sometime between then and November. I expect this is a widget issue, and so I think even a one-day regression window would really help.

(I would check myself but I'm not back to my regular Windows machine until Monday)
Flags: needinfo?(alice0775)
(Assignee)

Comment 3

3 years ago
CC'ing the world for ideas/help... :-)
Flags: firefox-backlog+

Updated

3 years ago
Flags: qe-verify?
(Reporter)

Comment 4

3 years ago
It may be necessary to repeat str to reproduce the problem.

Regression window(ux)
Good:
http://hg.mozilla.org/projects/ux/rev/bfb5c27c566b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130910040201
Bad:
http://hg.mozilla.org/projects/ux/rev/339479a60c1c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130911040201
Pushlog:
http://hg.mozilla.org/projects/ux/pushloghtml?fromchange=bfb5c27c566b&tochange=339479a60c1c

Suspect: 
7f26161e7dd6	Dão Gottwald — Bug 882009 - Make the tabs toolbar margin respect the tabsintitlebar attribute. r=Gijs
ed0e0347b778	Dão Gottwald — Bug 882009 - Invert updateTitlebarDisplay / TabsInTitlebar dependency. f=MattN r=Gijs
Flags: needinfo?(alice0775)
(Reporter)

Updated

3 years ago
Blocks: 882009
I'm tracking for all active releases but given that this hasn't blown up, depending on the fix and when it is available, we may or may not choose to take this fix in 33 or on ESR31.
status-firefox32: affected → wontfix
tracking-firefox33: ? → +
tracking-firefox34: ? → +
tracking-firefox35: ? → +
tracking-firefox-esr31: ? → +
Flags: qe-verify? → qe-verify+
Dao, can you help here? Thanks
Flags: needinfo?(dao)
Gijs, I hope you had good holidays. Sorry for the n-i on the day you come back but could you help here? thanks
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 8

3 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> Gijs, I hope you had good holidays. Sorry for the n-i on the day you come
> back but could you help here? thanks

I'm busy with other work until at least Tuesday/Wednesday, I'm afraid. I'll try to look then if Dão doesn't get a chance before then, so leaving needinfo.

Out of curiosity, Alice, does this only reproduce with the titlebar on/off, or in both cases?
(Assignee)

Comment 9

3 years ago
(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> > Gijs, I hope you had good holidays. Sorry for the n-i on the day you come
> > back but could you help here? thanks
> 
> I'm busy with other work until at least Tuesday/Wednesday, I'm afraid. I'll
> try to look then if Dão doesn't get a chance before then, so leaving
> needinfo.
> 
> Out of curiosity, Alice, does this only reproduce with the titlebar on/off,
> or in both cases?

w/ needinfo...
Flags: needinfo?(alice0775)
(Reporter)

Comment 10

3 years ago
It does not happen with titlebar off.
Flags: needinfo?(alice0775)
(Assignee)

Comment 11

3 years ago
Interestingly, I can't reproduce this very well on Windows 8, in the sense that the window control buttons work fine - but the titlebar disappears. :-\

I'll look if I can reproduce more completely on Windows 7.
(Assignee)

Comment 12

3 years ago
I also can't reproduce this fully on Windows 7 - I see the titlebar disappearing, but the window controls remain, and are functional (though the titlebar fog overlaps them).
(Reporter)

Comment 13

3 years ago
(In reply to :Gijs Kruitbosch from comment #12)
> I also can't reproduce this fully on Windows 7 - I see the titlebar
> disappearing, but the window controls remain, and are functional (though the
> titlebar fog overlaps them).

Regarding window control buttons, It is reproduced on Basic and Classic windows 7 visual style, not on Aero.

Updated

3 years ago
Summary: Cannot change window size mode and disappear window control button after restart when browser was exited in full screen mode → Cannot change window size mode and window controls disappear after restarting in full screen mode

Updated

3 years ago
Points: --- → 8
(Assignee)

Updated

3 years ago
See Also: → bug 1009458
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Comment 16

3 years ago
At least on win8, mOriginalBounds is also non-null in the started-in-fullscreen case, so there goes that theory.
Flags: needinfo?(jmathies)
(Assignee)

Comment 17

3 years ago
The universe decided this isn't fun enough yet: if I set a breakpoint in nsWindow::SetSizeMode, the stepping / timing differences make the bug go away.
(Assignee)

Comment 18

3 years ago
OK, so I'm getting closer, finally. First off, comment #14 was wrong - initially, the chromemargin attribute is set on the window. It doesn't get changed until you exit fullscreen mode. This is because there's a check in TabsInTitlebar._update for window.fullScreen, and it doesn't run at all in that case.

1) re-adding http://hg.mozilla.org/projects/ux/rev/ed0e0347b778#l1.12 fixes the chromemargin situation and the bug.
2) removing the fullscreen check at the top of TabsInTitlebar._update fixes the chromemargin situation and *doesn't* fix the bug

(2) not working is what I find surprising. (1) strikes me as a bit of a wallpaper, although we could certainly use it to un-fubar things in the short term if people think that's a good idea, potentially only for this case (window opens with sizemode=fullscreen and drawintitlebar=false).

On the widget side, it seems nsWindow::mCustomNonClient doesn't ever get unset from its initial value of "true" in the (2) and the existing case, whereas it does in the (1) case. That seems like a bug to me, but in fact, it seems it never gets unset unless you set the attribute to -1,-1,-1,-1, which the code says means "request a reset" ( http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#2293 ). I also randomly ran into this snippet: http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#6117 which I see triggering a no-op call to SetSizeModeChanged in the broken case when switching from fullscreen to normal mode, but not in the "correct" case. I don't know why.

Seems to me like the ideal solution involves making the Windows code deal with removing the attribute at any time - but then, I'm fairly sure that we don't do anything special for this when hitting the titlebar button in customize mode, and that seems to work fine...

Matt, Mike, I recall that you dealt with this stuff extensively, any thoughts considering the above?
Flags: needinfo?(mconley)
Flags: needinfo?(MattN+bmo)
(In reply to :Gijs Kruitbosch from comment #18)
> Matt, Mike, I recall that you dealt with this stuff extensively, any
> thoughts considering the above?

I would check if this creates new reflows upon window with browser_windowopen_reflows.js and also check the impact on tpaint/ts_paint.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 20

3 years ago
(In reply to Matthew N. [:MattN] from comment #19)
> (In reply to :Gijs Kruitbosch from comment #18)
> > Matt, Mike, I recall that you dealt with this stuff extensively, any
> > thoughts considering the above?
> 
> I would check if this creates new reflows upon window with
> browser_windowopen_reflows.js and also check the impact on tpaint/ts_paint.

It shouldn't if we limit this to fullscreen + tabsintitlebar disabled, I would have thought?

But really, I'm mostly curious about whether you think this points to something in Windows-widget-land that needs fixing. I really don't understand why removing chromemargin when we do isn't "good enough" so to speak. Thoughts on that particular bit? :-)
Flags: needinfo?(MattN+bmo)
Too late in the 33 cycle!
status-firefox33: affected → wontfix
(Assignee)

Updated

3 years ago
See Also: → bug 1005098
Gijs - You look to be the most active in the bug. Can you take it and continue to drive it? Note that 34 is now on beta. If we want a fix in 34 (I'm not sure that this is necessary), this bug will need a fix in the next couple of weeks.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 23

3 years ago
(In reply to Lawrence Mandel [:lmandel] from comment #22)
> Gijs - You look to be the most active in the bug. Can you take it and
> continue to drive it? Note that 34 is now on beta. If we want a fix in 34
> (I'm not sure that this is necessary), this bug will need a fix in the next
> couple of weeks.

For the latter question: Necessary, no, but desirable, yes. We've been shipping this since 29, so if we ship another release with this unfixed, that won't kill us, but ideally we should figure this out and fix it.

Matt brought this up while I was in MV last week and mentioned he didn't know the answer to comment #20 without having to dig/step through the code himself. I'll leave the needinfo for me and try to make time to do more digging later today. I suspect there *is* an issue in widget/win32 here, but I'll need to look at it in more detail to be sure.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 24

3 years ago
Here's some alternative STR that mess up:

0) ensure titlebar is disabled
1) go into fullscreen
2) toggle titlebar on
3) go out of fullscreen

Not sure if the cause is the same yet. :-\
(Assignee)

Comment 25

3 years ago
Ah, finally, some progress. It seems that removing chromemargin calls ResetChromeMargin just fine, so that part works correctly.

Unfortunately, the first thing ResetChromeMargin does is check if mHideChrome is set to true:

http://hg.mozilla.org/mozilla-central/annotate/7e3c85754d32/widget/windows/nsWindow.cpp#l2293

If so, it bails out. mHideChrome is updated when going in and out of fullscreen (through widget's MakeFullscreen methods):

http://hg.mozilla.org/mozilla-central/annotate/7e3c85754d32/widget/windows/nsWindow.cpp#l2738

whereas chromemargin is updated through TabsInTitlebar, from a resize event caught by tabbrowser.xml. These race, which explains comment 17.

The reason the "early" removal of the attribute worked is because at that point the window isn't fullscreen yet, and so mHideChrome is still false.

Just fixing the widget code to update the margins when switching mHideChrome back to false should be enough to fix this, without any browser frontend fixes, so I'm moving this bug accordingly. I'm also making this reflect reality by taking.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 36.2
Component: General → Widget: Win32
Flags: needinfo?(mconley)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dao)
Product: Firefox → Core
(Assignee)

Comment 26

3 years ago
Created attachment 8513495 [details] [diff] [review]
dropping out of fullscreen mode without titlebar breaks titlebar/tabs layout,

 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=727a51283b9d
Attachment #8513495 - Flags: review?(jmathies)
(Assignee)

Comment 27

3 years ago
(In reply to :Gijs Kruitbosch from comment #24)
> Here's some alternative STR that mess up:
> 
> 0) ensure titlebar is disabled
> 1) go into fullscreen
> 2) toggle titlebar on
> 3) go out of fullscreen
> 
> Not sure if the cause is the same yet. :-\

This doesn't seem to be fixed, so presumably that's a different bug... :-(

Comment 28

3 years ago
Comment on attachment 8513495 [details] [diff] [review]
dropping out of fullscreen mode without titlebar breaks titlebar/tabs layout,

Review of attachment 8513495 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +2299,5 @@
>  
> +  if (mHideChrome) {
> +    mFutureMarginsOnceChromeShows = margins;
> +    mFutureMarginsToUse = true;
> +    return NS_ERROR_INVALID_ARG;

Seems like this should be NS_OK since the call succeeds, but delays actually setting the margins?
Attachment #8513495 - Flags: review?(jmathies) → review+
(Assignee)

Comment 29

3 years ago
(In reply to Jim Mathies [:jimm] from comment #28)
> Comment on attachment 8513495 [details] [diff] [review]
> dropping out of fullscreen mode without titlebar breaks titlebar/tabs layout,
> 
> Review of attachment 8513495 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsWindow.cpp
> @@ +2299,5 @@
> >  
> > +  if (mHideChrome) {
> > +    mFutureMarginsOnceChromeShows = margins;
> > +    mFutureMarginsToUse = true;
> > +    return NS_ERROR_INVALID_ARG;
> 
> Seems like this should be NS_OK since the call succeeds, but delays actually
> setting the margins?

Yes.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8689c2f2f7cb
https://hg.mozilla.org/mozilla-central/rev/8689c2f2f7cb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(Assignee)

Updated

3 years ago
See Also: bug 1005098
status-firefox36: --- → fixed
Gijs - This fix looks like it has stuck on m-c. 34 and 35 are marked as affected. Do you want to propose for uplift?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 32

3 years ago
Comment on attachment 8513495 [details] [diff] [review]
dropping out of fullscreen mode without titlebar breaks titlebar/tabs layout,

Approval Request Comment
[Feature/regressing bug #]: 
[User impact if declined]: odd behaviour of titlebar when coming out of fullscreen if browser started in fullscreen, if the titlebar is enabled
[Describe test coverage new/current, TBPL]: nope :-(
[Risks and why]: low to medium. The risk is different regressions relating to tracking window modes and/or what part of the window needs to be non-client-margin'd (ie where we think the titlebar is/isn't). Considering the patch, I would say that risk is quite low, but if there is something we've not thought about, the impact might be non-trivial. OTOH, we still have beta baking time left, and this is an easy, small change to uplift and therefore also to back out.
[String/UUID change made/needed]: nope
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8513495 - Flags: approval-mozilla-beta?
Attachment #8513495 - Flags: approval-mozilla-aurora?
Comment on attachment 8513495 [details] [diff] [review]
dropping out of fullscreen mode without titlebar breaks titlebar/tabs layout,

Beta+
Aurora+
Attachment #8513495 - Flags: approval-mozilla-beta?
Attachment #8513495 - Flags: approval-mozilla-beta+
Attachment #8513495 - Flags: approval-mozilla-aurora?
Attachment #8513495 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/f51cca1cda64
https://hg.mozilla.org/releases/mozilla-beta/rev/f6b893ef9186
status-firefox34: affected → fixed
status-firefox35: affected → fixed
I was able to reproduce the bug on Windows 7 64-bit (Basic theme) using Nightly 35.0a1 (2014-09-04).

This is now verified fixed on:
 * Firefox 34 Beta 8 (build1 / 20141110195804),
 * Aurora 35.0a2 (2014-11-11),
 * Nightly 36.0a1 (2014-11-11),
using Windows 7 64-bit and Windows 8.1 64-bit.

I noticed a few inconsistencies/potential issues on *latest nightly with e10s* though, not sure if they're related to this or have separate bugs on file (couldn't find any):
1. [Win7] With aero, the tab strip displays a black gradient as background after resizing the window. 
    * screenshot: http://i.imgur.com/C6oIlfd.png
    * note: perhaps this is similar to Bug 951646
2. [Win7, Win8.1] The title bar cannot be enabled while the browser is in full screen.
    * str: enter full screen by pressing F11 → click menu → Customize → click Title Bar from the bottom of the palette → click Exit Customize
3. [Win8.1] The window is rendered black after switching a few times between full screen and windowed
    * screenshot: http://i.imgur.com/pa2CZ5c.png 
    * note:  I was unable to determine exact STR for this bug, nor reproduce it more than once

Gijs, what's your take on these?
Status: RESOLVED → VERIFIED
status-firefox34: fixed → verified
status-firefox35: fixed → verified
status-firefox36: fixed → verified
Flags: needinfo?(gijskruitbosch+bugs)
QA Contact: andrei.vaida
(Assignee)

Comment 36

3 years ago
(In reply to Andrei Vaida, QA [:avaida] from comment #35)
> I was able to reproduce the bug on Windows 7 64-bit (Basic theme) using
> Nightly 35.0a1 (2014-09-04).
> 
> This is now verified fixed on:
>  * Firefox 34 Beta 8 (build1 / 20141110195804),
>  * Aurora 35.0a2 (2014-11-11),
>  * Nightly 36.0a1 (2014-11-11),
> using Windows 7 64-bit and Windows 8.1 64-bit.
> 
> I noticed a few inconsistencies/potential issues on *latest nightly with
> e10s* though, not sure if they're related to this or have separate bugs on
> file (couldn't find any):
> 1. [Win7] With aero, the tab strip displays a black gradient as background
> after resizing the window. 
>     * screenshot: http://i.imgur.com/C6oIlfd.png
>     * note: perhaps this is similar to Bug 951646

This deserves its own bug, yes. Indeed potentially related to bug 951646. Please file a bug with detailed STR, including the about:support details of the machine(s) on which you can repro. Probably belongs in Core::Graphics.

> 2. [Win7, Win8.1] The title bar cannot be enabled while the browser is in
> full screen.
>     * str: enter full screen by pressing F11 → click menu → Customize →
> click Title Bar from the bottom of the palette → click Exit Customize

There's not meant to be a titlebar in fullscreen, if that's what you mean. Never. As noted in comment #24, this does break once you go back to normal mode... :-\

This definitely deserves its own bug. It'd be great if you could file that.

> 3. [Win8.1] The window is rendered black after switching a few times between
> full screen and windowed
>     * screenshot: http://i.imgur.com/pa2CZ5c.png 
>     * note:  I was unable to determine exact STR for this bug, nor reproduce
> it more than once

Wat. That seems like a separate graphics bug... :-(

I would be very surprised if any of the graphics issues were related to the one fixed here, though - so they don't need to block this one. (2) is probably related in some way, but was the same before this fix, so we didn't regress that.
Flags: needinfo?(gijskruitbosch+bugs)
Thanks for the feedback, Gijs. Just to wrap things up here, I've filed Bug 1098204 for Issue#1, Bug 1098282 for Issue#2 and Bug 1098294 for Issue#3.
Comment on attachment 8513495 [details] [diff] [review]
dropping out of fullscreen mode without titlebar breaks titlebar/tabs layout,

>+  // Indicates we need to apply margins once toggling chrome into showing:
>+  bool                  mFutureMarginsToUse;
This variable is not properly initialised. Things seem to work OK if you're drawing in the title bar, because you set it to true when you enter full screen, but if you're not, then you're out of luck.

Updated

3 years ago
Depends on: 1102855
(In reply to comment #38)
> (From update of attachment 8513495 [details] [diff] [review])
> >+  // Indicates we need to apply margins once toggling chrome into showing:
> >+  bool                  mFutureMarginsToUse;
> This variable is not properly initialised. Things seem to work OK if you're
> drawing in the title bar, because you set it to true when you enter full
> screen, but if you're not, then you're out of luck.

Filed bug 1102855.

Updated

2 years ago
Depends on: 1148236
You need to log in before you can comment on or make changes to this bug.