Closed Bug 520535 Opened 11 years ago Closed 10 years ago

XUL windows with dependent=true and titlebar=no do not receive keyboard & mouse events

Categories

(Core :: Widget: Win32, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- final-fixed

People

(Reporter: saerdnA, Assigned: enndeakin)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091004 Minefield/3.7a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091004 Minefield/3.7a1pre

Using the latest 3.7 nightly build (04-Oct-2009) on WinXP Pro SP3, the chrome dialogs opened with "titlebar=no" do not receive focus:
- all keyboard events are handled by the opener
- mouse click events don't work (i.e. on buttons/checkboxes/etc.)
- mouseover/mouseout events seem to be handled ok

The problem is recent: I have tried with a build from middle of Aug-2009 and it is not present, though there the titlebar is still shown even with "titlebar=no" - so perhaps fixing that problem has caused this one?

The problem is not present in older Firefox versions (1.5, 2.0, 3.0, 3.5).

Reproducible: Always

Steps to Reproduce:
0. use 3.7a1pre build of 04-Oct-2009, on Windows, using default theme
1. install Versions addon v0.21 (https://addons.mozilla.org/addon/3830)
2. open the addon (from Statusbar or Tools menu) and try a [Scan] or [Edit] button (note: in case of first install, there will be a query dialog first => answer with yes)
3. a secondary dialog will open, containing button(s) and radio/checkboxes => these are not reachable via keyboard/mouse
Actual Results:  
@step 3. the secondary dialog is not reachable via keyboard/mouse

Expected Results:  
@step 3. the secondary dialog should respond to keyboard/mouse (button click, radio/checkbox selection/command, etc.)
This regressed on 15-16 Aug 2009; http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ef2e4699b319&tochange=32d7abba0d2b so maybe a regression from Bug 510686 -  Dialogs always open minimisable, maximisable and resizable by Neil Rashbrook.
 
Maybe Neil Rashbrook has some advice for you?
Blocks: 510686
Component: General → Extension Compatibility
Keywords: regression
QA Contact: general → extension.compatibility
Version: unspecified → Trunk
Well the good news is that this extension is compatible with SeaMonkey 2.0a1 - 2.1a1 so that I was able to debug this properly. (I did notice however that the Modern theme doesn't seem to style iconic submenus correctly though.)

As expected, all my patch does it to make the title bar go away as requested. Unfortunately the focus manager rewrite seems to be calling the wrong widget function when a window gets focus, and as a result attempting to focus a titlebar-less window actually focuses its owner.
Attached patch Possible patchSplinter Review
Assignee: nobody → neil
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #405045 - Flags: review?(jmathies)
Attachment #405045 - Flags: review?(enndeakin)
Can we put a test together for the focus problem w/no titlebar?
Note that this bug also requires the 'dependent' flag in addition to the 'titlebar' flag.

However, this patch causes issues with noautohide panels. Seems like DispatchFocusToTopLevelWindow should instead be going up the parent hierarchy and only stopping when reaching the top or a parent which is equal to the owner.
Attachment #405045 - Flags: review?(jmathies)
Attachment #405045 - Flags: review?(enndeakin)
Attachment #405045 - Flags: review-
Enn, is this related or a separate bug? I wanted to check something to do with the customise toolbar sheet so I used about:config to set toolbar.customization.usesheet to true and tried to customise the toolbar but clicking on any of the controls caused the window title to go blank, but on a build with my patch applied this phenomenon does not happen.

[Things were worse on my Linux build since I couldn't interact with any of the controls at all and I had to close the entire window to break the spell.]
The same problem happens in Firefox 3.6 beta 2.
Is this going to be a huge problem for Add-ons? Should this block?
blocking2.0: --- → ?
Flags: wanted-firefox3.6+
Flags: blocking-firefox3.6?
Summary: titlebar=no windows do not receive keyboard & mouse events → XUL windows with dependent=true and titlebar=no do not receive keyboard & mouse events
Duplicate of this bug: 526474
Grepping the add-ons on AMO in bug 529939 suggests around 120 extensions have the potential to be impacted by this. It's difficult to guess how difficult it would be for developers to change away from dependent windows with no titlebars, I guess a panel would be the alternative but I know that they have issues, particularly with iframes in them which is what bug 526474 was specifically about so I suspect we may need to fix this if we can.
Bug 485500 is another panel + iframe/browser bug. Bug 526603 might be a duplicate of this bug.

Dependent, titlebar-less windows are sometimes used to make "toolpalette" style windows, with miniature minimize and close chrome. It would be a little more effort to build such windows with panels. Also, dependent, titlebar-less windows might actually be used to workaround the iframe/browser problems panels currently suffer.

I hesitate to ask for blocking, but this bug does make more grief for an already bad panel+iframe/browser situation. A fix for this bug is important.
I think this blocks, yeah. CC'ing Vlad, Mano and Connor, as Enn's out and we'll need some other people to help patch and review.
Flags: blocking-firefox3.6? → blocking-firefox3.6+
Rashbrook: do you think you can get a new patch up here, or should I re-assign to someone? No worries if that's needed, just trying to get some ETAs down ...
Whiteboard: [needs patch]
Whiteboard: [needs patch] → [needs new patch, feedback]
Whiteboard: [needs new patch, feedback] → [needs new patch, feedback, ETA]
Neil R - does silence here mean you can't take this?

Smaug - any chance this is one you can pick up? Neil Deakin is out until tomorrow, and this is a blocker.
Assignee: neil → nobody
Component: Extension Compatibility → Widget: Win32
Flags: wanted-firefox3.6+
Flags: blocking-firefox3.6+
Product: Firefox → Core
QA Contact: extension.compatibility → win32
Flags: wanted1.9.2+
Flags: blocking1.9.2+
Assignee: nobody → neil
Assignee: neil → jmathies
(In reply to comment #14)
> Neil R - does silence here mean you can't take this?
No, it means that I overlooked the bugmail because I was still busy catching up after recovering from illness. Unfortunately I won't be able to look into this for two days anyway, and ideally I'd need a test case for comment #5 anyway.
AFAICT, Neil's patch is the right way to fix the problem in this extension, but as Enn points out it causes problems in other areas. GetTopLevelHWND(mWnd, false) returns the top level or owner window. The second parameter indicates whether or not we should stop at dialog frames. In this case, DispatchFocusToTopLevelWindow requests top level or owner, so when the 'popup' dialog receives the set focus event, DispatchFocusToTopLevelWindow sends it off to the larger parent dialog, not the popup. Widget is essentially doing what it's supposed to.

The design of this extension's UI may not be correct. They create a dialog window as a child and remove the title bar. It doesn't appear to be parented correctly - you can drag the underlying larger dialog out from underneath the child for instance. I'm not sure if that type of design is common, but it doesn't appear to be correct from a win32 windowing perspective).
(In reply to comment #11)
> Dependent, titlebar-less windows are sometimes used to make "toolpalette" style
> windows, with miniature minimize and close chrome. It would be a little more
> effort to build such windows with panels. Also, dependent, titlebar-less
> windows might actually be used to workaround the iframe/browser problems panels
> currently suffer.

This particular extension is treating the dialog as a child popup, which isn't correct use. However, a free floating 'toolpalette' dialog would likely have the same focus problems we see here. In which case we should consider taking Neil's patch, and address problems in panels separately.
(In reply to comment #16)
> The design of this extension's UI may not be correct.

(In reply to comment #17)
> This particular extension is treating the dialog as a child popup,
> which isn't correct use.

This "wrong child popup" was designed during Firefox 1.5 & 2, when <panel> element was not available.  Since that design worked also in 3 & 3.5, and early versions of Fx 3.7, I did not switch to using <panel>.

I now have a workaround in place which uses <panel> for Fx3+ (toolkit 1.9+), but still uses the "wrong child popup" for Fx2- (toolkit 1.8-).

What I'm saying is that for me this bug is no longer "blocking".
I use this design (https://bugzilla.mozilla.org/show_bug.cgi?id=526474) for "bubble" windows with arrow pointing to button, so for me it's "blocking".
(In reply to comment #16)
> The second parameter indicates whether or not we should stop at dialog frames.
What about popup frames, and why are they and dialog frames getting confused?
(In reply to comment #20)
> (In reply to comment #16)
> > The second parameter indicates whether or not we should stop at dialog frames.
> What about popup frames, and why are they and dialog frames getting confused?

GetTopLevelHWND looks kind of borked to me, but maybe my definition of a top level window and whoever wrote it are different. It doesn't return the top level window by default, it returns the top level owner. Hence this bug - it skips past the top level dialog frame and keeps going. The parameter "aStopOnDialogOrPopup" also looks wrong, because popups don't have the WS_CHILD style, they have WS_POPUP. So I think calling GetTopLevelHWND(child of a popup, true) would return the owner (??).

I'm guessing the upper level focus manager code expects this weirdness.
Looks like CE ran into this too, maybe we should remove that ifdef'ing to see if it addresses this:

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#6897
Well, there are three cases:
1. The widget is for a main window
2. The widget is for a dependent window or dialog
3. The widget is for a popup
It's unclear to me how many callers of GetTopLevelHWND could be from a popup as opposed to a dialog; most of them look like they may have a dialog or a window, and either want the ancestor window (e.g. BeginResizeDrag) or the root owner (e.g. GetAttention).
Attached file panel testcase
I've been trying to find the panel issues Enn mentioned. Attached is a panel test case which afaict works fine with neil's patch. We really need to know what the issue is with stopping on dialogs for dispatching focus events. We can make changes to gettoplevelhwnd to make this work, but we really don't have any idea if those changes will in turn break something else without some feedback from folks who know focus manager better.
The call to GetTopLevelHWND from DispatchFocusToTopLevelWindow wants the topmost parent. Most windows and dialogs don't have owners, so ::GetParent is a suitable call to make. The bug is that when a window does have an owner (such as a dependent window), the wrong window is retrieved.

Passing true to aStopOnDialogOrPopup instead will fix that particular case, but will break the case when a popup is used:

1. Open the following as a chrome window:

<button type="menu" label="Open Me">
  <panel noautohide="true">
    <textbox/>
  </panel>
</button>

2. Open the popup by pressing the button.
3. Switch to another window.
4. Click on the textbox in the panel.

The window and panel should become active. Instead, with this patch, nothing happens.

This is because the aStopOnDialogOrPopup argument will cause either dialogs or popups to be returned. What is actually desired is any toplevel window or dialog only.
Attached patch something like this (obsolete) — Splinter Review
This patch checks for toplevel windows and dialogs. We should probably audit the other callers of GetTopLevelHWND and check what behaviour they really want.
Assignee: jmathies → enndeakin
Attachment #416103 - Flags: review?(jmathies)
(In reply to comment #26)
> Created an attachment (id=416103) [details]
> something like this
> 
> This patch checks for toplevel windows and dialogs. We should probably audit
> the other callers of GetTopLevelHWND and check what behaviour they really want.

Did you intend win->GetWindowType(wintype)? 

FWIW, I was looking at changes to GetTopLevelHWND and didn't see any options. The 'dialog' in the extension is actually a popup. There's really no way to look at styles or extended styles to zero in on it. I agree GetTopLevelHWND needs a serious overhaul.

I did notice one other minor anomaly, with your test case loaded in the xul editor in the ede, clicking the button -> clicking the xul editor -> switch windows -> click the popup text edit -> results in focus in the text editor. Doing the opposite: clicking the button -> clicking the popup text edit -> switch windows -> click the text editor -> results in focus in the editor. But this seems unrelated to this bug.
Attached patch updated patchSplinter Review
> Did you intend win->GetWindowType(wintype)?

I did!
Attachment #416103 - Attachment is obsolete: true
Attachment #416434 - Flags: review?(jmathies)
Attachment #416103 - Flags: review?(jmathies)
Attachment #416434 - Flags: review?(jmathies) → review+
Keywords: checkin-needed
Whiteboard: [needs new patch, feedback, ETA]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Is this patch branch safe? Any reason we're not putting it on 1.9.2 yet?
Whiteboard: [needs 1.9.2 landing]
Jim, could you check this in to 1.9.2? My Windows machine isn't working so I can't test it.
(In reply to comment #31)
> Jim, could you check this in to 1.9.2? My Windows machine isn't working so I
> can't test it.

Sure.
blocking2.0: ? → ---
You need to log in before you can comment on or make changes to this bug.