1165303 - [tablet mode] Switching tabs when Windows is in tablet mode causes Firefox to disappear

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

In Windows 10 (build 10074), switching tabs causes the Firefox (Nightly) window to minimize and throws the user back to the start screen. It is therefore almost impossible to work with Firefox in tablet mode.

Comment 2

[:Gijs (he/him)]

This is triggered by tabbrowser.xml calling updateTitlebar. Investigating further from there.

Comment 3

[:Gijs (he/him)]

Doesn't happen if you disable tabs in the titlebar (ie enable the titlebar).

Comment 4

[:Gijs (he/him)]

So this is specifically triggered by this call:

  ::SendMessageW(mWnd, WM_SETTEXT, (WPARAM)0, (LPARAM)(LPCWSTR)strTitle.get());

in 

NS_METHOD nsWindow::SetTitle(const nsAString& aTitle)

at: http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#2995

interestingly, the code at:


http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#4684

suggests that we should ignore the message if we're painting our own titlebar area. This lines up with comment #3 in that if we disable draw-in-titlebar and just show the Windows titlebar, this bug doesn't happen.

Interestingly, in my testing, we don't break; there, and instead enter the following block. I'll try to figure out why this happens.

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #4)
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.
> cpp#4684

This is actually the thing that's hiding the window by setting the window style to exclude WS_VISIBLE. The other window style call does not reshow the window.

> suggests that we should ignore the message if we're painting our own
> titlebar area. This lines up with comment #3 in that if we disable
> draw-in-titlebar and just show the Windows titlebar, this bug doesn't happen.
> 
> Interestingly, in my testing, we don't break; there, and instead enter the
> following block. I'll try to figure out why this happens.

I think I was misreading the code; it's meant to bail out if we don't paint in the titlebar (so if the "normal" windows titlebar is enabled).

So... in this case we're the ones firing WM_SETTEXT. Is it necessary to do what we're doing here and hide the window, do stuff, unhide it? Or do we not really need to do anything (at least in Firefox's case) when using a custom set of client margins, because we don't draw the title at all anyway? The code says that this message "paints the titlebar area", but it's not clear to me how this works and if this is meant to just be about the window title update, or also about other stuff.

Jim and Jeff, you've recently touched this code, can you help figure out what the correct path forward here is?

Comment 6

[Jim Mathies [:jimm]]

> > http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.
> > cpp#4684
> 
> This is actually the thing that's hiding the window by setting the window
> style to exclude WS_VISIBLE. The other window style call does not reshow the
> window.
> 
> So... in this case we're the ones firing WM_SETTEXT. Is it necessary to do
> what we're doing here and hide the window, do stuff, unhide it? Or do we not
> really need to do anything (at least in Firefox's case) when using a custom
> set of client margins, because we don't draw the title at all anyway? The
> code says that this message "paints the titlebar area", but it's not clear
> to me how this works and if this is meant to just be about the window title
> update, or also about other stuff.

You can try skipping setting the text on the window but you might see regressions in places where the window title is displayed like the taskbar, task manager, etc.. Worth a try though if you need to get rid of this. Note windows will paint the entire titlebar, including icon, test and frame so we definitely don't want windows handling this.

You might check chrome to see what they do.

Comment 7

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1165303 - avoid hiding and reshowing the window in response to our own settext messages because it makes Windows 10 unhappy, r?jimm

Bug 1165303 - avoid hiding and reshowing the window in response to our own settext messages because it makes Windows 10 unhappy, r?jimm

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to :Gijs Kruitbosch from comment #7)
> Created attachment 8617513 [details]
> MozReview Request: Bug 1165303 - avoid hiding and reshowing the window in
> response to our own settext messages because it makes Windows 10 unhappy,
> r?jimm
> 
> Bug 1165303 - avoid hiding and reshowing the window in response to our own
> settext messages because it makes Windows 10 unhappy, r?jimm

I believe this will make window text flicker on top of our Chrome when running without the dwm.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8617513 [details]
MozReview Request: Bug 1165303 - avoid hiding and reshowing the window in response to our own settext messages because it makes Windows 10 unhappy, r?jimm

Bug 1165303 - avoid hiding and reshowing the window in response to our own settext messages because it makes Windows 10 unhappy, r?jimm

Comment 10

[:Gijs (he/him)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > Created attachment 8617513 [details]
> > MozReview Request: Bug 1165303 - avoid hiding and reshowing the window in
> > response to our own settext messages because it makes Windows 10 unhappy,
> > r?jimm
> > 
> > Bug 1165303 - avoid hiding and reshowing the window in response to our own
> > settext messages because it makes Windows 10 unhappy, r?jimm
> 
> I believe this will make window text flicker on top of our Chrome when
> running without the dwm.

OK, so I need to test this patch on classic, is that what you're saying?

Comment 11

[:Gijs (he/him)]

(or winxp, really, either way?)

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 8617513 [details]
MozReview Request: Bug 1165303 - avoid hiding and reshowing the window in response to our own settext messages because it makes Windows 10 unhappy, r?jimm

https://reviewboard.mozilla.org/r/10647/#review9329

::: widget/windows/nsWindow.cpp:3001
(Diff revision 2)
> +  mSendingSetText = false;

Lets use AutoRestore here.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8617513 [details]
MozReview Request: Bug 1165303 - avoid hiding and reshowing the window in response to our own settext messages because it makes Windows 10 unhappy, r?jimm

I can reproduce the issue suspected by Jeff on my Win7 VM with the classic theme. I'll do a new version of the patch tomorrow that checks for the compositor before bailing.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

(In reply to :Gijs Kruitbosch from comment #13)
> Comment on attachment 8617513 [details]
> MozReview Request: Bug 1165303 - avoid hiding and reshowing the window in
> response to our own settext messages because it makes Windows 10 unhappy,
> r?jimm
> 
> I can reproduce the issue suspected by Jeff on my Win7 VM with the classic
> theme. I'll do a new version of the patch tomorrow that checks for the
> compositor before bailing.

Can you figure out what's happening on Windows 10? The hide and reshow idiom is pretty common. Can you confirm that all programs that use it are broken?

Comment 15

[:Gijs (he/him)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > Comment on attachment 8617513 [details]
> > MozReview Request: Bug 1165303 - avoid hiding and reshowing the window in
> > response to our own settext messages because it makes Windows 10 unhappy,
> > r?jimm
> > 
> > I can reproduce the issue suspected by Jeff on my Win7 VM with the classic
> > theme. I'll do a new version of the patch tomorrow that checks for the
> > compositor before bailing.
> 
> Can you figure out what's happening on Windows 10? The hide and reshow idiom
> is pretty common. Can you confirm that all programs that use it are broken?

I'm not sure how I would. I'm not a win32 programmer by trade; I just know (a little) about using MSVC, and I can see that as soon as the "hide" line is executed, the window goes away - and doesn't come back when the "show" line is executed. Note that this *only* happens in tablet mode, ie not in "normal" mode. Note also that this doesn't just affect tabswitching but also page loading and various other things that would change the page title.

Comment 16

[:Gijs (he/him)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > Comment on attachment 8617513 [details]
> > MozReview Request: Bug 1165303 - avoid hiding and reshowing the window in
> > response to our own settext messages because it makes Windows 10 unhappy,
> > r?jimm
> > 
> > I can reproduce the issue suspected by Jeff on my Win7 VM with the classic
> > theme. I'll do a new version of the patch tomorrow that checks for the
> > compositor before bailing.
> 
> Can you figure out what's happening on Windows 10? The hide and reshow idiom
> is pretty common. Can you confirm that all programs that use it are broken?

So I don't know how common it is, but the earlier example of chromium seems to be avoiding it on aero+ ( see comment above: https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/win/hwnd_message_handler.cc&q=WS_VISIBLE&sq=package:chromium&type=cs&l=1209 - I'm assuming this source is up-to-date; I couldn't find search functionality for their git repo without downloading the entire thing etc.)

Comment 17

[:Gijs (he/him)]

Comment on attachment 8617513 [details]
MozReview Request: Bug 1165303 - avoid hiding and reshowing the window in response to our own settext messages because it makes Windows 10 unhappy, r?jimm

Bug 1165303 - avoid hiding and reshowing the window in response to our own settext messages because it makes Windows 10 unhappy, r?jimm

Comment 18

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #17)
> Comment on attachment 8617513 [details]
> MozReview Request: Bug 1165303 - avoid hiding and reshowing the window in
> response to our own settext messages because it makes Windows 10 unhappy,
> r?jimm
> 
> Bug 1165303 - avoid hiding and reshowing the window in response to our own
> settext messages because it makes Windows 10 unhappy, r?jimm

Jim, I realize you've r+'d essentially this already, but considering Jeff's comments I wanted to make sure you still thought this made sense.

(Also, I didn't know about AutoRestore - neat! Hope I've done it right...)

Comment 19

[Jim Mathies [:jimm]]

(In reply to :Gijs Kruitbosch from comment #18)
> (In reply to :Gijs Kruitbosch from comment #17)
> > Comment on attachment 8617513 [details]
> > MozReview Request: Bug 1165303 - avoid hiding and reshowing the window in
> > response to our own settext messages because it makes Windows 10 unhappy,
> > r?jimm
> > 
> > Bug 1165303 - avoid hiding and reshowing the window in response to our own
> > settext messages because it makes Windows 10 unhappy, r?jimm
> 
> Jim, I realize you've r+'d essentially this already, but considering Jeff's
> comments I wanted to make sure you still thought this made sense.
> 
> (Also, I didn't know about AutoRestore - neat! Hope I've done it right...)

This seems fine if it fixes the win10 problem. I can't test on win10 but I tested on win7 with both aero basic and glass. I didn't see any drawing anomalies.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/76590118f442

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/76590118f442

Comment 23

[:Gijs (he/him)]

Comment on attachment 8617513 [details]
MozReview Request: Bug 1165303 - avoid hiding and reshowing the window in response to our own settext messages because it makes Windows 10 unhappy, r?jimm

Approval Request Comment
[Feature/regressing bug #]: windows 10
[User impact if declined]: Firefox is almost entirely unusable in win10 tablet mode in its default configuration.
[Describe test coverage new/current, TreeHerder]: no :(
[Risks and why]: medium low. The change is specific to win32 with the DWM on, but it does change how/when we update the window, so it's not entirely without risk. I'd like to have it on aurora to get a bit more testing and so it makes the 40 release if all goes well so that win10 tablet mode is usable in that. I don't think uplifting to late beta makes sense.
[String/UUID change made/needed]: no

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8617513 [details]
MozReview Request: Bug 1165303 - avoid hiding and reshowing the window in response to our own settext messages because it makes Windows 10 unhappy, r?jimm

Windows 10 is going to be one of the "new feature", taking it.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7333a8d7b494

Comment 26

[Cornel Ionce [:noni] [Hubs QA]]

The issue is fixed on a Microsoft Surface Pro 2 device running Windows 10 64-bit (10166) using:
- Latest 41.0a2, build ID: 20150714004006;
- Firefox 40 beta 4, build ID: 20150713153304.