Closed
Bug 457997
Opened 16 years ago
Closed 16 years ago
autohiding glass panels don't display properly
Categories
(Core :: Widget: Win32, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: robarnold, Assigned: robarnold)
References
Details
(Keywords: verified1.9.1)
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
beltzner
:
approval1.9.1b2+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
vlad
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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•16 years ago
|
Comment 1•16 years ago
|
||
Any update? noautohide="true" needs to be removed.
Assignee | ||
Comment 2•16 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.
Assignee | ||
Comment 3•16 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.
Comment 4•16 years ago
|
||
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•16 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•16 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•16 years ago
|
||
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•16 years ago
|
||
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 11•16 years ago
|
||
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 | ||
Updated•16 years ago
|
Keywords: checkin-needed
Updated•16 years ago
|
Flags: blocking1.9.1?
Updated•16 years ago
|
Attachment #347171 -
Flags: approval1.9.1b2?
Updated•16 years ago
|
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9.1b2
Updated•16 years ago
|
Component: XUL → Widget: Win32
QA Contact: xptoolkit.widgets → win32
Comment 13•16 years ago
|
||
Comment on attachment 347171 [details] [diff] [review]
v1.2 (checked in)
a=beltzner
Attachment #347171 -
Flags: approval1.9.1b2? → approval1.9.1b2+
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Comment 14•16 years ago
|
||
Comment 15•16 years ago
|
||
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?
Comment 16•16 years ago
|
||
(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•16 years ago
|
Assignee | ||
Comment 17•16 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.
Comment 18•16 years ago
|
||
(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.
Comment 19•16 years ago
|
||
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•16 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?
Comment 21•16 years ago
|
||
> Does the alltabs panel have noautohide="true"?
no
> Is there a way to activate it with a keyboard shortcut?
Ctrl+Shift+Tab
Updated•16 years ago
|
Attachment #347171 -
Attachment description: v1.2 → v1.2 (checked in)
Assignee | ||
Comment 22•16 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•16 years ago
|
||
This just ignores WM_GETMINMAXINFO in DealWithPopups
Attachment #348898 -
Flags: review?(vladimir)
Comment 24•16 years ago
|
||
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+
Comment 25•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Updated•16 years ago
|
Attachment #348898 -
Flags: approval1.9.1?
Comment 26•16 years ago
|
||
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 27•16 years ago
|
||
Comment on attachment 348898 [details] [diff] [review]
level="top" popups v1.0
a191=beltzner
Attachment #348898 -
Flags: approval1.9.1? → approval1.9.1+
Comment 28•16 years ago
|
||
Keywords: fixed1.9.1
Updated•16 years ago
|
Keywords: verified1.9.1
Updated•16 years ago
|
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•