Closed Bug 1165303 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox :: General, defect, P1)

Unspecified
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 41
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: phlsa, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [windows10][workaround in comment 3])

Attachments

(1 file)

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.
Priority: -- → P1
Summary: Switching tabs when Windows is in tablet mode causes Firefox to disappear → [tablet mode] Switching tabs when Windows is in tablet mode causes Firefox to disappear
No longer blocks: windows-10
This is triggered by tabbrowser.xml calling updateTitlebar. Investigating further from there.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Doesn't happen if you disable tabs in the titlebar (ie enable the titlebar).
Whiteboard: [windows10] → [windows10][workaround in comment 3]
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.
(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?
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(jmathies)
> > 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.
Flags: needinfo?(jmathies)
Bug 1165303 - avoid hiding and reshowing the window in response to our own settext messages because it makes Windows 10 unhappy, r?jimm
Attachment #8617513 - Flags: review?(jmathies)
(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.
Flags: needinfo?(jmuizelaar)
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
(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?
Flags: needinfo?(jmuizelaar)
(or winxp, really, either way?)
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.
Attachment #8617513 - Flags: review?(jmathies)
Attachment #8617513 - Flags: review+
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.
Attachment #8617513 - Flags: review-
Flags: needinfo?(jmuizelaar)
(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?
(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.
(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 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
Attachment #8617513 - Flags: review?(jmathies)
Attachment #8617513 - Flags: review-
Attachment #8617513 - Flags: review+
(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...)
Attachment #8617513 - Flags: review?(jmathies) → review+
(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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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
Attachment #8617513 - Flags: approval-mozilla-aurora?
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.
Attachment #8617513 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
QA Contact: cornel.ionce
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: