511503 - Need events for window activation / deactivation

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

For https://wiki.mozilla.org/Firefox/Projects/Per_Tab_Network_Prioritization I was hoping to get some notification for when windows get focused (and minimized/restored)

I was hoping to keep implementation of my project in JS (and in one place) so having events would probably be the easiest way.

Filing in DOM because of nsGlobalWindow::ActivateOrDeactivate.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Looks like xul-window-visible might be good enough for getting focus and seems
to be sent on global window activations.

Comment 2

[Simon Bünzli]

Have you already tried the "focus" and "blur" DOM events that are dispatched on the window's root element?

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #2)
> Have you already tried the "focus" and "blur" DOM events that are dispatched on
> the window's root element?

I tried that and the results were less than optimal. focus & blur for the chrome window will fire only if the content document isn't focused. The content document has it's own separate focus & blur events. So I could probably do a combination of these. We'll see as I get closer to finishing.

That said, it's probably valuable to have these events regardless. It seems like a pretty basic thing people might want to know.

(In reply to comment #1)
> Looks like xul-window-visible might be good enough for getting focus and seems
> to be sent on global window activations.

Neil says that's a bad event to use, but I filed bug 513148 to make it more useful (as a potential alternative to this).

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

First pass to get the events in there. Name & placement in code based on feedback from Enn & smaug.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 398431 [details] [diff] [review]
Patch v0.1

You dispatch "activate" in both cases. And please use NS_LITERAL_STRING

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2

Updated with NS_LITERAL_STRING and the right event names. What should testing consist of here?

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I just realized that this isn't firing when a new window is created... Neil, do you know why?

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #7)
> I just realized that this isn't firing when a new window is created... Neil, do
> you know why?

Nevermind. I'm just not attaching the event listener until after delayedStartup, so I miss it when the window is created. Simple test showed it is firing when the window is created, just before I'm listening.

Comment 9

[Neil Deakin]

(In reply to comment #6)
> Updated with NS_LITERAL_STRING and the right event names. What should testing
> consist of here?

You should be able to test this by checking for the events in the function switchWindowTest in dom/tests/mochitest/chrome/window_focus.xul I'd also check in various other places during the test to ensure the events don't fire when they shouldn't, for example when switching child frames. Let me know if you need help with the test.

Comment 10

[Dão Gottwald [:dao]]

Is this ready for review?

Comment 11

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #10)
> Is this ready for review?

Not quite. I need to write tests, which I'm getting to today/tomorrow.

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.3

Now with tests. Thanks for the help debugging issues.

I had to put doc declaration in FocusManager in #ifndef DEBUG_FOCUS. I had turned it on and there were compilation errors. Alternatives are to ignore and use the same variable name or rename one of them.

Comment 13

[Neil Deakin]

Comment on attachment 408674 [details] [diff] [review]
Patch v0.3

>+  // send event

send activate event

>+#ifndef DEBUG_FOCUS
>+  nsCOMPtr<nsIDocument> doc = do_QueryInterface(window->GetExtantDocument());
>+#endif
>+  nsContentUtils::DispatchTrustedEvent(doc,
>+                                       window,
>+                                       NS_LITERAL_STRING("activate"),
>+                                       PR_TRUE, PR_TRUE, nsnull);

It would be clearer to just use a different variable name (such as document) than have ifdefs.


>+  // send event

send deactivate event

>+#ifndef DEBUG_FOCUS
>+  nsCOMPtr<nsIDocument> doc = do_QueryInterface(window->GetExtantDocument());
>+#endif
>+  nsContentUtils::DispatchTrustedEvent(doc,
>+                                       window,
>+                                       NS_LITERAL_STRING("deactivate"),
>+                                       PR_TRUE, PR_TRUE, nsnull);

similar here


"deactivate")
>+    id = "frame-" + gOldExpectedWindow.document.documentElement.id + "-window";
>+  else if (gNewExpectedWindow && event.type == "activate")
>+    id = "frame-" + gNewExpectedWindow.document.documentElement.id + "-window";

The checks here shouldn't be based on the window we're expecting as that's what we're going to compare against. Instead, I think just using the following should work:

id = event.target.document.documentElement.id + "-window";

(You don't probably need the -window part really)


>+    var id;
>+    if (getTopWindow(gLastFocusWindow) != getTopWindow(expectedWindow)) {
>+      if (gOldExpectedWindow)
>+        id = "frame-" + gOldExpectedWindow.document.documentElement.id;
>+      else
>+        id = "outer"
>+      expectedEvents += "deactivate: " + id + "-window";
>+    }

The check here can just get the id from gLastFocusWindow:

id = getTopWindow(gLastFocusWindow).document.documentElement.id;
 

>+    if (getTopWindow(gLastFocusWindow) != getTopWindow(expectedWindow)) {
>+      if (gOldExpectedWindow)
>+        id = "frame-" + gNewExpectedWindow.document.documentElement.id;
>+      else
>+        id = "outer"
>+      if (expectedEvents)
>+        expectedEvents += " ";
>+      expectedEvents += "activate: " + id + "-window";
>+    }

Similar here for expectedWindow instead of gLastFocusWindow.

Comment 14

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.4

I left ids with "-window" since there's a window named outer-document and other things look for outer-document.

Comment 15

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/c71894f31c31

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This patch has a test and nothing turned orange. Let's call it verified based on check-in.

Paul, is the summary of the bug still correct? Shouldn't it be activation / deactivation?

Comment 17

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #16)
> Paul, is the summary of the bug still correct? Shouldn't it be activation /
> deactivation?
A little more correct, yes.

Asked for a1.9.2? because this blocks bug 514490, which we want on branch.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 408900 [details] [diff] [review]
Patch v0.4

a192=beltzner

Comment 19

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/de90c90518c2

Comment 20

[Eric Shepherd [:sheppy]]

Trying to write some code to play with this in preparation for documenting, and I'm not apparently getting it right, since I never get the activate or deactivate events.  Here's my code:

function handleEvent(e) {
  alert(e.type);
}
window.addEventListener("activate", handleEvent, true);
window.addEventListener("deactivate", handleEvent, true);

My handleEvent function is never getting called. What am I doing wrong here? This is with build 20091111.

Comment 21

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #20)
> Trying to write some code to play with this in preparation for documenting, and
> I'm not apparently getting it right, since I never get the activate or
> deactivate events.  Here's my code:
> 
> function handleEvent(e) {
>   alert(e.type);
> }
> window.addEventListener("activate", handleEvent, true);
> window.addEventListener("deactivate", handleEvent, true);
> 
> My handleEvent function is never getting called. What am I doing wrong here?
> This is with build 20091111.

I think this only works for top level windows or maybe just from chrome? The only current consumer is using it with a window object directly from browser.js & the test with the patch above is browser-chrome. Maybe Neil can explain it better?

Comment 22

[Eric Shepherd [:sheppy]]

If this is internal only, I'm going to scrap plans to document it. If this turns into something more generally useful, please re-add the doc needed keyword.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Marking as verified fixed on 1.9.2 due to no test failures.

Comment 24

[Dão Gottwald [:dao]]

Need to document this for XUL developers.

Comment 25

[Eric Shepherd [:sheppy]]

Documented in a note here:

https://developer.mozilla.org/en/XUL/window

Comment 26

[Eric Shepherd [:sheppy]]

Also added here:

https://developer.mozilla.org/en/XUL/Events#Window_activation_events