Closed Bug 457997 Opened 16 years ago Closed 16 years ago

autohiding glass panels don't display properly

Categories

(Core :: Widget: Win32, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: robarnold, Assigned: robarnold)

References

Details

(Keywords: verified1.9.1)

Attachments

(2 files, 2 obsolete files)

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.
Blocks: 451300
No longer depends on: 451300
Blocks: 458173
Any update? noautohide="true" needs to be removed.
I haven't had time to work on it. I was hoping that someone who knows the popup/panel code would have some ideas.
Blocks: 460560
No longer blocks: 458173
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?
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.
Next debugging fact: when the address bar would be covered by the ctrl-tab panel, it displays properly even if focused by the mouse.
Attached patch v1.0 (obsolete) (deleted) — Splinter Review
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)
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.)
Attached patch v1.1 (obsolete) (deleted) — Splinter Review
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
Attached patch v1.2 (checked in) (deleted) — Splinter Review
Nits addressed
Attachment #347131 - Attachment is obsolete: true
Keywords: checkin-needed
Flags: blocking1.9.1?
Attachment #347171 - Flags: approval1.9.1b2?
Blocks: 461950, 463693, 463822
Target Milestone: --- → mozilla1.9.1b2
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+
Status: ASSIGNED → RESOLVED
Closed: 16 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 → ---
Blocks: 465076
No longer blocks: 463822
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
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
Attachment #347171 - Attachment description: v1.2 → v1.2 (checked in)
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.
Attached patch level="top" popups v1.0 (deleted) — Splinter Review
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?
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Flags: blocking1.9.1+
Status: ASSIGNED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
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+
Keywords: verified1.9.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: