autohiding glass panels don't display properly

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Widget: Win32
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: robarnold, Assigned: robarnold)

Tracking

({verified1.9.1})

Trunk
mozilla1.9.2a1
x86
Windows Vista
verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
In Bug 451300, I added support for XUL glass panels by changing the win32 window style to be WS_POPUP | WS_THICKFRAME instead of WS_POPUP | WS_OVERLAPPED. For some reason, WS_THICKFRAME or WS_CAPTION is needed for the glass effect. If I use WS_CAPTION, autohiding panels work just fine, but with WS_THICKFRAME (which looks much better), they appear briefly and then disappear. It's not clear why this is happening, but I think it may have regressed sometime in the last month.

Updated

9 years ago
Blocks: 451300
No longer depends on: 451300

Updated

9 years ago
Blocks: 458173
Any update? noautohide="true" needs to be removed.
(Assignee)

Comment 2

9 years ago
I haven't had time to work on it. I was hoping that someone who knows the popup/panel code would have some ideas.

Updated

9 years ago
Blocks: 460560

Updated

9 years ago
No longer blocks: 458173
(Assignee)

Comment 3

9 years ago
So I tried setting noautohide="false" and it works most of the time just fine. It seems that whenever a textbox has focus (like the location bar or search), then I get the old behavior. Also, when I focus a tab (the tab itself, not the contents), switch away from the window and then back it sometimes doesn't show up when I press Ctrl-Tab. It always seems to work when I press the button though, so that's a good sign at least.
The difference between the keyboard shortcut and the button is that we focus the search field in the latter case. There's a patch in bug 463799 that focuses the panel otherwise. Can you give this patch a try?
(Assignee)

Comment 5

9 years ago
Tried the focus patch and it doesn't seem to help. Focusing the panel on popupshown doesn't seem to help either. The popup only seems to disappear when the tab key is released. Furthermore, if I focus the address bar with the mouse, the panel disappears however if I focus the address bar with the keyboard (Ctrl L), then the panel displays properly.
(Assignee)

Comment 6

9 years ago
Next debugging fact: when the address bar would be covered by the ctrl-tab panel, it displays properly even if focused by the mouse.
(Assignee)

Comment 7

9 years ago
Created attachment 347098 [details] [diff] [review]
v1.0

On further debugging, it appears that the message causing problems for us is WM_GETMINMAXINFO (0x24). This message doesn't seem like it would have a position attached so I suspect the call to ::GetMessagePosition in nsWindow::EventIsInsideWindow is getting its position from a previous message. This patch simply removes the autohide property from the panel and ignores rollup in the case of a WM_GETMINMAXINFO message.
Assignee: jag → tellrob
Status: NEW → ASSIGNED
Attachment #347098 - Flags: review?(vladimir)
Attachment #347098 - Flags: review?(dao)
Attachment #347098 - Flags: review?(vladimir) → review+
Comment on attachment 347098 [details] [diff] [review]
v1.0

Looks ok, but needs a comment about why we're ignoring WM_GETMINMAXINFO there, and a pointer to this bug (more than just the bug number though -- explain what that event was causing, etc.)
(Assignee)

Comment 9

9 years ago
Created attachment 347131 [details] [diff] [review]
v1.1

I traced the history of the code and it turns out that the patch in bug 281948 incorrectly modified the message filter condition and removed the comment that cleanly explained why WM_GETMINMAXINFO was being ignored (and only in certain circumstances). I've restored the original comment and removed the incorrect (and overlapping) condition. Still seems to work and this no longer feels like a hack.
Attachment #347098 - Attachment is obsolete: true
Attachment #347131 - Flags: review?(vladimir)
Attachment #347098 - Flags: review?(dao)
Comment on attachment 347131 [details] [diff] [review]
v1.1

Yeah, that looks a lot better.
Attachment #347131 - Flags: review?(vladimir) → review+
Comment on attachment 347131 [details] [diff] [review]
v1.1

+    <panel id="ctrlTab-panel" class="KUI-panel" hidden="true" norestorefocus="true" ignorekeys="true" >

nit: there's a trailing space after the last attribute
(Assignee)

Comment 12

9 years ago
Created attachment 347171 [details] [diff] [review]
v1.2 (checked in)

Nits addressed
Attachment #347131 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed

Updated

9 years ago
Flags: blocking1.9.1?

Updated

9 years ago
Attachment #347171 - Flags: approval1.9.1b2?

Updated

9 years ago
Blocks: 461950, 463693, 463822

Updated

9 years ago
Target Milestone: --- → mozilla1.9.1b2

Updated

9 years ago
Component: XUL → Widget: Win32
QA Contact: xptoolkit.widgets → win32
Comment on attachment 347171 [details] [diff] [review]
v1.2 (checked in)

a=beltzner
Attachment #347171 - Flags: approval1.9.1b2? → approval1.9.1b2+
Flags: blocking1.9.1? → blocking1.9.1+
http://hg.mozilla.org/mozilla-central/rev/8e51153faaa8
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081111 Minefield/3.1b2pre ID:20081111032821

Dao, is there a way to get it into test-suite? Or do we have to handle it in Litmus only?
Status: RESOLVED → VERIFIED
Flags: in-litmus?
(In reply to comment #0)
> but with
> WS_THICKFRAME (which looks much better), they appear briefly and then
> disappear.

Apparently the patch in bug 465076 (attachment 348548 [details] [diff] [review] currently) triggers exactly these symptoms :( -- e.g. see bug 465076 comment 16.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.9.1b2 → ---

Updated

9 years ago
Blocks: 465076
No longer blocks: 463822
(Assignee)

Comment 17

9 years ago
I need more information from Henrik to debug this. I'll need to know the size and position of the firefox window and the ctrl tab panel. I'll also need to know exactly what he does from the moment the browser starts up to the ctrl-tab panel hiding itself including keyboard presses, mouse clicks and mouse moves (alt-tabbing included).

I'm curious if the effect happens when the panel is not glass as well. If so, then the bug lies in the general or win32 popup code. Try it that first before going through the pain of writing down a reduced testcase.
(In reply to comment #17)
> I'm curious if the effect happens when the panel is not glass as well. If so,
> then the bug lies in the general or win32 popup code.

FWIW, I haven't seen anything like that on XP.
Ok, it's just easy. This only happens if the mouse cursor is outside of the ctrl-tab panels area when you hit Ctrl+Tab. So following steps are enough:

1. Open at least 3 tabs
2. Move the cursor into the middle of the screen
3. Hit Ctrl+Tab
4. Release the keys and move the mouse to a corner of the screen
5. Hit Ctrl+Tab

With step 3 the panel is correctly shown and sticky as long as you hold down the ctrl+tab keys. With step 5 the panel is closed immediately. And yes, this is Aero only. Classic works like expected.

Rob, I hope that helps you to figure out the issue.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 20

9 years ago
Ok, I can reproduce it with that tryserver build. Thanks Henrik. Not sure when I'll get a chance to debug this but I have a feeling that it's similar to the previous bug since the mouse pointer position makes the difference. Does the alltabs panel have noautohide="true"? Is there a way to activate it with a keyboard shortcut?
> Does the alltabs panel have noautohide="true"?

no

> Is there a way to activate it with a keyboard shortcut?

Ctrl+Shift+Tab

Updated

9 years ago
Attachment #347171 - Attachment description: v1.2 → v1.2 (checked in)
(Assignee)

Comment 22

9 years ago
Ok, the key difference is that the ctrl-tab panel has level="top" which means the native window has no parent (or owner apparently). This means that the call to ::GetParent that happens after the check for WM_GETMINMAXINFO returns NULL and so the old behavior of calling ::GetMessagePos happens all over again. I traced the introduction of the WM_GETMINMAXINFO check and it appears to have been introduced by the patch in bug 81416 nearly 7 years ago. From the comment in bug 81416 comment 6, it's not clear why WM_GETMINMAXINFO was included (it was included without the ::GetParent check initially). I'm also still not sure why this doesn't trigger for non-glass popups.
(Assignee)

Comment 23

9 years ago
Created attachment 348898 [details] [diff] [review]
level="top" popups v1.0

This just ignores WM_GETMINMAXINFO in DealWithPopups
Attachment #348898 - Flags: review?(vladimir)
Can we mark this FIXED and get a followup bug, please? Or the have the original patch backed out?
Attachment #348898 - Flags: review?(vladimir) → review+
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Flags: blocking1.9.1+
http://hg.mozilla.org/mozilla-central/rev/f0c06d36f6fd
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1

Updated

9 years ago
Attachment #348898 - Flags: approval1.9.1?
Verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20081206 Minefield/3.2a1pre ID:20081206051147 by doing the STR from comment 19 and the latest tryserver build on bug 465076. The Ctrl-Tab panel closes now when releasing the keys.
Status: RESOLVED → VERIFIED
Comment on attachment 348898 [details] [diff] [review]
level="top" popups v1.0

a191=beltzner
Attachment #348898 - Flags: approval1.9.1? → approval1.9.1+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1d4d0cf9a903
Keywords: fixed1.9.1

Updated

9 years ago
Keywords: verified1.9.1
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.