Closed Bug 428071 Opened 16 years ago Closed 16 years ago

NS_(DE)ACTIVATE events are fired when they shouldn't

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: mstange, Assigned: smichaud)

References

Details

(Whiteboard: [ETA 4/30][steven taking])

Attachments

(2 files, 1 obsolete file)

Fix reported problems
18.20 KB, patch
jaas
: review+
mstange
: review+
roc
: superreview+
mconnor
: approval1.9+
Details | Diff | Splinter Review
Screenshot showing an inactive window with sheet, using an adapted GrApple theme
126.99 KB, image/png
Details
Attached patch Fix v1.0: Key => Main (obsolete) — — Splinter Review 
Cocoa windows can have "key" state and "main" state. Most of the time a window that's main is also key. However, that's not the case when opening the Dock menu of the active application.
When you open the Dock menu of the active application, the application's window loses key state but keeps main state. The same thing happens when dragging the app's Dock icon.

In bug 406730 I'm listening for NS_ACTIVATE / NS_DEACTIVATE events. These events are currently sent in nsChildView's viewWindowDidBecomeKey / DidResignKey. However, that makes the window look inactive during Dock operation which should not be the case.

The attached patch changes the behavior to check for main state instead of key state.
Attachment #314662 - Flags: review?(smichaud)
In principle this patch (attachment 314662 [details] [diff] [review]) should be fine ... as long
as a window that can take keyboard input never becomes key without
also becoming main.

But I not sure that's true, so I won't be comfortable r+ing this patch
until I've had a chance to test it.

I probably won't be able to test until next Monday (2008-04-14).

> I probably won't be able to test until next Monday (2008-04-14).

Because I'm taking Thursday and Friday off, and am busy today with another bug.

I've changed my mind on this bug. It doesn't really need fixing.
There are other cases where the NS_(DE)ACTIVATE events don't reflect changes in a window's main state; e.g. when a sheet opens, the window receives a NS_DEACTIVATE event but doesn't lose main state.

I changed the patch for bug 406730 to ask Cocoa for the window's main state instead of relying on the NS_(DE)ACTIVATE events.
It looks like this bug needs to be fixed for bug 406730, after all.

When switching between windows of the active application, the key/main state changes like this:

     << ResignedKey    window A
     >> BecameKey      window B
 <<     ResignedMain   window A
 >>     BecameMain     window B

That means that at the time window B becomes key (and the NS_ACTIVATE event is sent), it's not main yet. However, the appearance of the window depends on its main state and not on its key state, so I can't just ask for isKeyWindow. For example, an opening sheet takes key state but the parent window does not get the inactive look.

(In reply to comment #1)
> ... as long
> as a window that can take keyboard input never becomes key without
> also becoming main.

In theory, such windows exist. See http://developer.apple.com/documentation/UserExperience/Conceptual/OSXHIGuidelines/XHIGWindows/chapter_18_section_5.html#//apple_ref/doc/uid/20000961-TPXREF54

However, these "panel style" windows are not used by Mozilla.
Blocks: 406730
There is another type of window that can become key without becoming main:
Sheets. (Surprise!)

Fortunately there is already code in Gecko that sends the necessary NS_(DE)ACTIVATE events for sheets. Currently we're sending the events twice.
This is what happens when (1) focusing the window and (2) opening a sheet:

(a) Before applying the patch:

    (1) Window is activated.
         >>     BecameMain
             >> BecameKey
              ! NS_ACTIVATE
     (2) A sheet is opened.
              ! NS_DEACTIVATE
             << ResignedKey
              ! NS_DEACTIVATE
              ! NS_DEACTIVATE
            >> BecameKey
              ! NS_ACTIVATE
              ! NS_ACTIVATE

(b) After applying the patch:

    (1) Window is activated.
         >>     BecameMain
              ! NS_ACTIVATE
             >> BecameKey
     (2) A sheet is opened.
              ! NS_DEACTIVATE
             << ResignedKey
             >> BecameKey
              ! NS_ACTIVATE
Markus, I've started to test the current code and your patch wrt the
sending of NS_ACTIVATE and NS_DEACTIVATE events, but I'm not yet
finished.

I'm inclined to accept your patch (with changes) ... but I want to be
very careful about changing focus code at this late date, because it's
such a horrible mess (both on the Mac and cross-platform).

And now I'm being pulled off this bug to work on another :-(

Here's what I've found so far, and some ideas for changes:

With your patch, calls on a sheet to NS_ACTIVATE and NS_DEACTIVATE
aren't accompanied by calls to its focused (first responder)
ChildView/nsChildView.

As you note in comment #5, there's a lot of sending of duplicate
events when sheets are involved.  It's worse without your patch, but
there's still some even with your patch.

My idea is to take your patch, but alter it so that the behavior with
sheet windows becomes the same as with non-sheet windows, and there's
no duplicate sending of events.  This might involve adding code to
prevent two NS_ACTIVATE events (or two NS_DEACTIVATE events) being
sent in a row to the same object.

I hope to get back to this bug soon ... but realistically it probably
won't be before next week.

(In reply to comment #6)
> With your patch, calls on a sheet to NS_ACTIVATE and NS_DEACTIVATE
> aren't accompanied by calls to its focused (first responder)
> ChildView/nsChildView.

Are you referring to viewsWindowDidBecomeKey and viewsWindowDidResignKey?
Even without my patch, viewsWindowDidBecomeKey doesn't seem to be called - there's a weird "else {...}" in windowBecameKey in nsWindowMap.mm:

if (delegate && [delegate respondsToSelector:@selector(sendFocusEvent:)]) {
  /* ... */
} else {
  id firstResponder = [window firstResponder];
  if ([firstResponder isKindOfClass:[ChildView class]])
   [firstResponder viewsWindowDidBecomeKey];
}

So viewsWindowDidBecomeKey is only called when there's no sendFocusEvent, which never the case, is it?
Interestingly, there is no such "else" in windowResignedKey.

But nevertheless, with my patch, the call to viewsWindowDidResignKey is missing when opening the sheet.
The NS_DEACTIVATE event is sent in nsCocoaWindow::Show:

    if (mWindowType == eWindowType_sheet) {
      /* ... */
          [[mSheetWindowParent delegate] sendFocusEvent:NS_LOSTFOCUS];
          [[mSheetWindowParent delegate] sendFocusEvent:NS_DEACTIVATE];
          /* ... */
          [[mWindow delegate] sendFocusEvent:NS_GOTFOCUS];
          [[mWindow delegate] sendFocusEvent:NS_ACTIVATE];

Should I add
[[mSheetWindowParent delegate] viewsWindowDidResignKey];
and
[[mWindow delegate] viewsWindowDidBecomeKey];
here?

> As you note in comment #5, there's a lot of sending of duplicate
> events when sheets are involved.

Let's have a look at windowResignedKey in nsWindowMap.mm again:

if (delegate && [delegate respondsToSelector:@selector(sendFocusEvent:)]) {
  [delegate sendFocusEvent:NS_DEACTIVATE];
  [delegate sendFocusEvent:NS_LOSTFOCUS];
}
id firstResponder = [window firstResponder];
if ([firstResponder isKindOfClass:[ChildView class]])
  [firstResponder viewsWindowDidResignKey];

Here the events are sent twice: First using sendFocusEvent directly, and again in viewsWindowDidResignKey.
Would it make sense to remove these direct calls to sendFocusEvent and only call them in viewsWindowDidResignKey?
In what cases is delegate != firstResponder?

> My idea is to take your patch, but alter it so that the behavior with
> sheet windows becomes the same as with non-sheet windows, and there's
> no duplicate sending of events.  This might involve adding code to
> prevent two NS_ACTIVATE events (or two NS_DEACTIVATE events) being
> sent in a row to the same object.

Like this?

id firstResponder = [window firstResponder];
if (delegate && [delegate respondsToSelector:@selector(sendFocusEvent:)] &&
    (delegate != firstResponder ||
     ![firstResponder isKindOfClass:[ChildView class]])) {
  [delegate sendFocusEvent:NS_DEACTIVATE];
  [delegate sendFocusEvent:NS_LOSTFOCUS];
}
if ([firstResponder isKindOfClass:[ChildView class]])
  [firstResponder viewsWindowDidResignKey];

And similar stuff in windowBecameKey and nsCocoaWindow::Show, of course.

We really need to get this sorted out soon, bug 406730 depends on it.
Markus, I'm going to write my own patch.

I'll be as conservative as possible -- I'll make as few changes as possible from the current code and from your current patch (fix v 1.0).

I'll be working on this the rest of today and into tomorrow.

As for the wierdness wrt to viewsWindowDidBecomeKey and viewsWindowDidResignKey at the bottom of WindowMap.mm:  Yes, it's wierd, but it's been very throughly tested.  See the comments above windowBecameKey and windowResignedKey for more information.

Thank you, Steven.

I've discovered another problem of my patch: When a sheet is open and the user switches to a different application, the sheet isn't deactivated. And it's not activated when focus returns.

And one more note: In bug 54488 we have to repaint a window whenever its *key* state changes. You might want to consider that as well.
This blocks a Firefox 3 blocker, and is thus also blocking.
Flags: blocking1.9+
Assignee: mstange → smichaud
Status: ASSIGNED → NEW
Here's a patch that seens to cover all the bases.

I've kept the behavior in embedders (like Camino) the same as it was
previously -- let me know if this is a problem.

I tested switching between top-level windows (with and without sheets
above them) within the browser and between apps.  As far as I can
tell, the right events are always sent to their correct targets,
without duplication.

I also tested to see if this patch re-introduces the focus problems I
talk about in the comments that are now above [TopLevelWindowData
activateInWindow:] and [TopLevelWindowData deactivateInWindow:] (in
nsWindowMap.mm).  As far as I can tell it doesn't.

Normally I'd make a tryserver build, but this time I haven't -- the
changes I've made here aren't really testable by themselves.  However
if somebody wants one, let me know and I'll make one.

Markus, please try out this patch with the ones that you've been
working on (at least the ones for bug 406730 and bug 54488).  Let us
know your results.
Attachment #314662 - Attachment is obsolete: true
Attachment #317714 - Flags: review?(joshmoz)
Attachment #314662 - Flags: review?(smichaud)
> And one more note: In bug 54488 we have to repaint a window whenever
> its *key* state changes. You might want to consider that as well.

I really wasn't able to take this into consideration.  Markus, let me
know if you think it will require changes to this bug ... keeping in
mind (or course) that these would have to be changes that don't break
what we've already got :-)

Comment on attachment 317714 [details] [diff] [review]
Fix reported problems

Please rename method "toplevelActiveState" to "toplevelStateActive".
Attachment #317714 - Flags: review?(joshmoz) → review+
> Please rename method "toplevelActiveState" to "toplevelStateActive".

I'll do this when I land the patch (or make other revisions).

Markus, we need to hear from you before we can do anything more here.
Attachment #317714 - Flags: review?(mstange)
Comment on attachment 317714 [details] [diff] [review]
Fix reported problems

Thanks Steven, it works really well. With one exception: When switching to another window while a sheet is open, the sheet-containing window doesn't get the inactive look, since the NS_DEACTIVATE event is only fired on the sheet. I know that's by design and I think we might get away with that.
Attachment #317714 - Flags: review?(mstange) → review+
In the patch for bug 406730, I set the "active" attribute on the window returned by querying GetMainWidget:

+      // Get the window widget
+      nsCOMPtr<nsIBaseWindow> baseWindow(do_QueryInterface(GetDocShell()));
+      NS_ENSURE_TRUE(baseWindow, nsnull);
+      nsCOMPtr<nsIWidget> mainWidget;
+      baseWindow->GetMainWidget(getter_AddRefs(mainWidget));

So when an NS_(DE)ACTIVATE event is received by a sheet, the mainWidget is the sheet. We could work around the issue I mentioned in comment 16 by looking for the real top level window, e.g. in case of a sheet look for its parent window. Is that possible? Does the cross-platform Gecko code know about window hierarchies?

Regarding bug 54488:
The patch for bug 54488 works flawlessly with this patch. Adding mGeckoChild->Invalidate(PR_FALSE); in sendFocusEvent does all the magic that's necessary.
> Does the cross-platform Gecko code know about window hierarchies?

I believe it does (at least to a certain extent) ... though I'm no
authority.  All I can suggest is using trial and error to find out if
nsIWidget::GetParent() can be made to do what you want.  I'd expect
that if GetParent() returns NULL, this means you have a top-level
widget.

OK, I'll try that tomorrow. However, I have little confidence in GetParent; I use it in bug 54488 (in frameIsInActiveWindow) and it doesn't seem to cross window boundaries.
I'll wait for your results before asking for an sr for my patch.

(Following up comment #19)

Actually, ordinary windows don't have parents.  But I believe sheet
windows do (windows of type eWindowType_sheet).

I've got access to the parent window using GetParentWidget(), but now I'm stuck. I need the nsIDOMXULDocument for that window but I don't know how to do that...

There's something in nsBaseFilePicker.cpp that uses nsIDocShellTreeItems to get parent DocShells using docShellAsItem->GetSameTypeParent(getter_AddRefs(parent)):
http://mxr.mozilla.org/firefox/source/widget/src/xpwidgets/nsBaseFilePicker.cpp#72

But even when (if) I've got the DocShell of that parent window, I still don't know how to get to the DOMDocument.
You can QI an nsIDOMDocument to get an nsIDOMXULDocument ... but maybe you already knew that.

Now that I think about it, the real problem is getting from an
nsIWidget to an nsIDOMDocument.  It's truly a shame that Gecko makes
this kind of thing so difficult!

Anyway, something like the following might do:

1) You can get from an nsView object from an nsIWidget object by
   calling nsView::GetViewFor(nsIWidget*).

2) You can get an nsIFrame object by calling GetClientData() on the
   nsView object and casting it appropriately.

3) You can get an nsIContent object by calling GetContent() on the
   nsIFrame object.

4) You can get an nsIDocument object by calling GetDocument() on the
   nsIContent object.  This in turn can be QIed to an nsIDOMDocument
   (or an nsIDOMXULDocument).

> 1) You can get from an nsView object from an nsIWidget object by
>   calling nsView::GetViewFor(nsIWidget*).

This gets you _to_ an nsView object from an nsIWidget object.

Sounds great, thanks! I'll try it later today.
If at any point you get a NULL result, things get more complicated.
Let's hope that doesn't happen.

Calling nsView::GetViewFor() (a static method) may cause linker
problems.  If so, there's a (hackish but reliable) workaround in
[ChildView getScrollableView] that doesn't require linking in another
module.

Yeah, it works! Not exactly the way you outlined (nsView::GetViewFor returns null because the widget's clientData doesn't contain a view, but an nsXULWindow), but your advice got me on the right track.
I'll upload a patch to bug 406730 soon.

Unfortunately, I've now found another problem with your patch.
In comment 4 I mentioned that when switching windows of the active application, the key state changes before the main state and thus the NS_DEACTIVATE event is sent when the window is still main.
Your patch solves this problem, but only as long as no sheet is open.

Steps to reproduce:
1. Open a browser window and the Addons manager.
2. Click the "Uninstall" button of an extension entry in the Addons manager.
   A confirmation sheet opens.
3. Focus the browser window.

Actual results:
    << ResignedKey   (Addons sheet)
     ! NS_DEACTIVATE (Addons sheet) with [Addons isMainWindow] == YES
       ...

Desired results:
    << ResignedKey   (Addons sheet)
    >> BecameKey     (Browser)
 <<    ResignedMain  (Addons)
     ! NS_DEACTIVATE (Addons sheet) so that [Addons isMainWindow] == NO
       ...
That's funny. When I switch to a different *application* while having a sheet open, the NS_DEACTIVATE event fires when the sheet loses key state (i.e. before resignedMain), but [window isMainWindow] returns NO.
I'll get to this tomorrow ... and see what revisions (if any) I can make to my patch.

(In reply to comment #29 and comment #30)

> Unfortunately, I've now found another problem with your patch.  In
> comment 4 I mentioned that when switching windows of the active
> application, the key state changes before the main state and thus the
> NS_DEACTIVATE event is sent when the window is still main.
>
> Your patch solves this problem, but only as long as no sheet is
> open.

I don't think I can (or should) change my patch to fix this
(especially in light of comment #30).

But how about if you change nsCocoaWindow::IsActive() as I suggest in
bug 406730 comment #91?  With additional changes to your patch for bug
406730, this might do the trick.

(In reply to comment #32)
> But how about if you change nsCocoaWindow::IsActive() as I suggest in
> bug 406730 comment #91?  With additional changes to your patch for bug
> 406730, this might do the trick.

That's a great idea!
Whiteboard: [ETA 4/30][steven taking]
do we need an approval here?  What's going on?
Yes.  Now that I've got fixes for this bug (428071), bug 406730 and bug 431429 all working together, it's time to ask for an sr and then an approval.

(See bug 406730 for more info.)
Attachment #317714 - Flags: superreview?(roc)
> Yes.  Now that I've got fixes for this bug (428071), bug 406730 and
> bug 431429 all working together, it's time to ask for an sr and then
> an approval.

I've made a combo tryserver build (with a combo patch) available at
bug 406730 comment #97.

There was vociferous agreement among drivers that this is too risky for 1.9. We can't take any more patches like this until we have infrastructure and a decent test suite for Mac window focus issues.
Flags: blocking1.9+ → blocking1.9-
Flags: wanted1.9.0.x?
Well, some drivers disagree so this may be back on the table. So I'll do the review as best I can.
Comment on attachment 317714 [details] [diff] [review]
Fix reported problems

Well, I didn't see anything wrong, FWIW. But I still think it's scary.
Attachment #317714 - Flags: superreview?(roc) → superreview+
Attachment #317714 - Flags: approval1.9?
Attachment #317714 - Flags: approval1.9? → approval1.9+
Checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Hardware: Macintosh → All
Flags: wanted1.9.0.x?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: