Need events for window activation / deactivation

VERIFIED FIXED in mozilla1.9.3a1

Status

()

defect
VERIFIED FIXED
10 years ago
a month ago

People

(Reporter: zpao, Assigned: zpao)

Tracking

({dev-doc-complete, verified1.9.2})

Trunk
mozilla1.9.3a1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta3-fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Looks like xul-window-visible might be good enough for getting focus and seems
to be sent on global window activations.

Comment 2

10 years ago
Have you already tried the "focus" and "blur" DOM events that are dispatched on the window's root element?
(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).
Posted patch Patch v0.1 (obsolete) — Splinter Review
First pass to get the events in there. Name & placement in code based on feedback from Enn & smaug.
Assignee: nobody → paul
Comment on attachment 398431 [details] [diff] [review]
Patch v0.1

You dispatch "activate" in both cases. And please use NS_LITERAL_STRING
Status: NEW → ASSIGNED
Posted patch Patch v0.2 (obsolete) — Splinter Review
Updated with NS_LITERAL_STRING and the right event names. What should testing consist of here?
Attachment #398431 - Attachment is obsolete: true
I just realized that this isn't firing when a new window is created... Neil, do you know why?
(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.
Blocks: 521129

Comment 9

10 years ago
(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.
Is this ready for review?
(In reply to comment #10)
> Is this ready for review?

Not quite. I need to write tests, which I'm getting to today/tomorrow.
Posted patch Patch v0.3 (obsolete) — Splinter Review
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.
Attachment #402116 - Attachment is obsolete: true
Attachment #408674 - Flags: review?(enndeakin)

Updated

10 years ago
Attachment #408674 - Flags: review?(enndeakin) → review-

Comment 13

10 years ago
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.
Posted patch Patch v0.4Splinter Review
I left ids with "-window" since there's a window named outer-document and other things look for outer-document.
Attachment #408674 - Attachment is obsolete: true
Attachment #408900 - Flags: review?(enndeakin)

Updated

10 years ago
Attachment #408900 - Flags: review?(enndeakin) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/c71894f31c31
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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?
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Target Milestone: --- → mozilla1.9.3a1
(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.
Summary: Need events for window focus / activation → Need events for window activation / deactivation
Comment on attachment 408900 [details] [diff] [review]
Patch v0.4

a192=beltzner
Attachment #408900 - Flags: approval1.9.2? → approval1.9.2+
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.
Whiteboard: [doc-waiting-info]
(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?
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.
Keywords: dev-doc-needed
Whiteboard: [doc-waiting-info]
Marking as verified fixed on 1.9.2 due to no test failures.
Keywords: verified1.9.2
Need to document this for XUL developers.
Keywords: dev-doc-needed
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.