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)
Tracking
()
VERIFIED
FIXED
Firefox 41
People
(Reporter: phlsa, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
(Whiteboard: [windows10][workaround in comment 3])
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
jimm
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details |
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.
Reporter | ||
Updated•10 years ago
|
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
Updated•9 years ago
|
Blocks: tabletmode
Updated•9 years ago
|
No longer blocks: windows-10
Assignee | ||
Comment 2•9 years ago
|
||
This is triggered by tabbrowser.xml calling updateTitlebar. Investigating further from there.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 years ago
|
||
Doesn't happen if you disable tabs in the titlebar (ie enable the titlebar).
Whiteboard: [windows10] → [windows10][workaround in comment 3]
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
(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)
Comment 6•9 years ago
|
||
> > 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)
Assignee | ||
Comment 7•9 years ago
|
||
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)
Comment 8•9 years ago
|
||
(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)
Assignee | ||
Comment 9•9 years ago
|
||
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
Assignee | ||
Comment 10•9 years ago
|
||
(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)
Assignee | ||
Comment 11•9 years ago
|
||
(or winxp, really, either way?)
Comment 12•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8617513 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
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-
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(jmuizelaar)
Comment 14•9 years ago
|
||
(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?
Assignee | ||
Comment 15•9 years ago
|
||
(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.
Assignee | ||
Comment 16•9 years ago
|
||
(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.)
Assignee | ||
Comment 17•9 years ago
|
||
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+
Assignee | ||
Comment 18•9 years ago
|
||
(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...)
Updated•9 years ago
|
Attachment #8617513 -
Flags: review?(jmathies) → review+
Comment 19•9 years ago
|
||
(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
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Assignee | ||
Comment 23•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox40:
--- → affected
Comment 24•9 years ago
|
||
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+
Comment 25•9 years ago
|
||
Updated•9 years ago
|
Flags: qe-verify+
Comment 26•9 years ago
|
||
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.
Description
•