Last Comment Bug 336359 - Fire WHATWG DOM events when going online or offline
: Fire WHATWG DOM events when going online or offline
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
: Hixie (not reading bugmail)
Mentors:
Depends on: 336605 338255
Blocks: 336682
  Show dependency treegraph
 
Reported: 2006-05-02 20:46 PDT by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2008-03-31 00:50 PDT (History)
12 users (show)
asqueella: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (18.50 KB, patch)
2006-05-02 21:44 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
darin.moz: review+
darin.moz: superreview+
Details | Diff | Splinter Review
testcase (1.01 KB, text/html)
2006-05-02 21:52 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
First testcase: onload (1.02 KB, text/html)
2006-05-04 11:15 PDT, Boris Zbarsky [:bz] (TPAC)
no flags Details
Testcase for onclick (1.05 KB, text/html)
2006-05-04 12:16 PDT, Boris Zbarsky [:bz] (TPAC)
no flags Details
Testcase for onclick for IE6 (1008 bytes, text/html)
2006-05-04 15:10 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase IE6 for onload (436 bytes, text/html)
2006-05-04 16:27 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-02 20:46:49 PDT
See http://www.whatwg.org/specs/web-apps/current-work/#scs-offline

We already support the property, but we don't yet fire the events. It's slightly tricky because we want the events to fire on documents as they get thawed out of bfcache if the offline status changed while they were frozen.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-02 21:44:12 PDT
Created attachment 220608 [details] [diff] [review]
fix

darin, feel free to suggest another reviewer if this isn't up your alley
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-02 21:52:01 PDT
Created attachment 220609 [details]
testcase

testcase
Comment 3 Darin Fisher 2006-05-03 12:17:54 PDT
Comment on attachment 220608 [details] [diff] [review]
fix

>Index: dom/public/base/nsPIDOMWindow.h

>+  // Fire any DOM notiication events related to things that happened while

"notification"


>Index: dom/src/base/nsGlobalWindow.cpp

>+static PRBool GetOffline() {
>+  nsCOMPtr<nsIIOService> ios(do_GetService(NS_IOSERVICE_CONTRACTID));
>+  if (!ios) {
>+    // No ioservice would mean this is the case
>+    return PR_TRUE;
>+  }
>+  PRBool offline = PR_TRUE;
>+  ios->GetOffline(&offline);
>+  return offline;
>+}

Use NS_IsOffline from nsNetUtil.h


>+nsresult
>+nsGlobalWindow::FireDelayedDOMEvents()
>+{
>+  FORWARD_TO_INNER(FireDelayedDOMEvents, (), NS_ERROR_UNEXPECTED);
>+
>+  if (mFireOfflineStatusChangeEventOnThaw) {
>+    FireOfflineStatusEvent();
>+    mFireOfflineStatusChangeEventOnThaw = PR_FALSE;

Maybe you should reset that flag before calling out into other code.
What if FireDelayedDOMEvents is recursed somehow?  Or what if
Observe is called from within FireOfflineStatusEvent somehow?


r+sr=darin
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-03 14:53:24 PDT
checked in.
Comment 5 Boris Zbarsky [:bz] (TPAC) 2006-05-03 15:21:15 PDT
Er.. could we have some discussion, in general, before adding new events we fire?  Events are part of our API; once we add them we can't really remove them...

Also, why are we firing these on the body instead of the window?  I realize that this is what the WHATWG spec says, but I don't see _why_ it says that and it's inconsistent with what we do for, say, load events (where we fire them on the window, but use the body's onload attr to set a listener on the _window_).  I'd rather prefer the spec were changed here, unless there are good reasons for what it specifies.

Some review from a DOM peer in general would have been nice.  :(
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-03 16:08:29 PDT
OK, sorry. I'll make any changes people want me to make.

I don't know why the spec wants events targeted at the body, but clearly Ian went out of his way to make them to do so, so I assumed there was a good reason.
Comment 7 Boris Zbarsky [:bz] (TPAC) 2006-05-03 19:31:49 PDT
I assume so that on* handlers on the <body> would get the events; as I said we handle that differently in Gecko at the moment.  Ian, do you really want on* handlers on <html> to get the events in question?
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-05-04 03:10:28 PDT
I think firing them on the body doesn't make any sense at all since there might not be a concept of a body in the document being displayed. I would prefer that they were fired on the document.
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-05-04 03:12:12 PDT
Alternativly the root element might make sense since you can declarativly attach handlers there.
Comment 10 Boris Zbarsky [:bz] (TPAC) 2006-05-04 07:23:01 PDT
Sicking, what WHATWG says is:

   ... They must be fired on the body element, or, if there isn't a "the body
   element", on the DocumentWindow object. (As the events bubble, they will
   reach the Window  object.)

Here the phrase 'the body element' is a link to nowhere at the moment; it looks like Ian wants to define that in terms of the |body| property of the document (HTMLDocument, I guess).  So if the document has no concept of a body the event will fire on the document itself.

Ian's not reading bugmail, so I sent him mail asking why the spec says what it does.
Comment 11 Hixie (not reading bugmail) 2006-05-04 09:00:00 PDT
Yeah the target of these events is a bit of a mess right now. (I recently discovered that my understanding of how events work w.r.t. propagation to Window objects, as well as how <body onload> and window.onload, etc, work, is flawed.)

My intention was to have <body ononline=""> and <body onoffline=""> work. I'll make the spec say whatever you want to make that work. :-)
Comment 12 Boris Zbarsky [:bz] (TPAC) 2006-05-04 10:52:38 PDT
> My intention was to have <body ononline=""> and <body onoffline=""> work.

Right.  Note that with the patch in this bug they don't, btw, since it has no changes to hook up event listeners for those attributes.  It's also not said in the spec anywhere that these attributes should be hooked up as event listeners, is it?

Also, should Window (and HTMLBody, etc) have an ononline/onoffline property that can be set to a function?  That doesn't work with this patch either, and is not mentioned in the spec one way or another.  Note also that adding properties like this to Window is a bit of a dangerous business, since it tends to break scripts using variables with those names.

I'll attach some testcases that I've written up to test behavior of the "load" event (which does not bubble) and the "click" event (which does) in some situations and put in the results I see with Opera 8.5, Konqueror 3.5, and current trunk Gecko.  I'd love to hear what IE does, though its lack of support for addEventListener makes this a little wacky.
Comment 13 Boris Zbarsky [:bz] (TPAC) 2006-05-04 11:15:58 PDT
Created attachment 220804 [details]
First testcase: onload

This testcase adds at most 8 event listeners for the load event in the following ways:

1)  onload attribute on <html>
2)  setting window.onload
3)  addEventListener("load") on Window
4)  setting document.onload
5)  addEventListener("load") on Document
6)  onload attribute on <body>
7)  setting document.body.onload
8)  addEventListener("load") on document.body

In Gecko I see the following listeners fire, in order:

#6: target is Document, currentTarget is Window, this is Window
#3: target is Document, currentTarget is Window, this is Window

Knowing what I know about our arch, we're simply dispatching the event to the Window, with the Document set as event.target, so only listeners #2, #3, and #6 are relevant (since we set #6 on the window), and #6 overrides #2.

In Opera I see the following listeners fire, in order:

#7: target is body, currentTarget is body, this is body
#8: target is body, currentTarget is body, this is body
#3: target is Document, currentTarget is Document, this is Document
#4: target is Document, currentTarget is Document, this is Document
#5: target is Document, currentTarget is Document, this is Document

I'm not sure what happened to listener #2.  Even if I don't add listeners #6 and #7 I don't see listener #2 fire.  Note that in opera listener #7 overrides listener #6.  It looks like Opera dispatches two separate events -- one to the body and one to the Document.  I have no idea how they manage to trigger listener #3 with a currentTarget of Document....

In Konqueror I see the following listeners fire, in order:

#7: target is null, currentTarget is Document, this is Document
#5: target is null, currentTarget is Document, this is Document
#6: target is null, currentTarget is null, this is Window
#3: target is null, currentTarget is null, this is Window

I really have no idea how to make sense of this, and don't propose to try.
Comment 14 Boris Zbarsky [:bz] (TPAC) 2006-05-04 12:16:46 PDT
Created attachment 220811 [details]
Testcase for onclick

This testcase adds at most 8 event listeners for the click event in the
following ways:

1)  onclick attribute on <html>
2)  setting window.onclick
3)  addEventListener("click") on Window
4)  setting document.onclick
5)  addEventListener("click") on Document
6)  onclick attribute on <body>
7)  setting document.body.onclick
8)  addEventListener("click") on document.body

In Gecko I see the following listeners fire, in order:

#7: target is body, currentTarget is body, this is body
#8: target is body, currentTarget is body, this is body
#1: target is body, currentTarget is html, this is html
#4: target is body, currentTarget is Document, this is Document
#5: target is body, currentTarget is Document, this is Document
#6: target is body, currentTarget is Window, this is Window
#3: target is body, currentTarget is Window, this is Window

Again, note that listener #6 overrode listener #2.

In Opera I see the following listeners fire, in order:

#7: target is body, currentTarget is body, this is body
#8: target is body, currentTarget is body, this is body
#1: target is body, currentTarget is html, this is html
#3: target is body, currentTarget is Document, this is Document
#4: target is body, currentTarget is Document, this is Document
#5: target is body, currentTarget is Document, this is Document

Here listener #7 overrides listener #6 (tested by commenting it out, in which case replace #7 with #6 with no other changes).  Not sure what happened to listener #2.  Listener #3 ends up attached to the Document, not the Window?

In Konqueror I see the following listeners fire, in order:

#7: target is body, currentTarget is body, this is body
#8: target is body, currentTarget is body, this is body
#1: target is body, currentTarget is html, this is html
#4: target is body, currentTarget is Document, this is Document
#5: target is body, currentTarget is Document, this is Document
#3: target is body, currentTarget is null, this is Window

Again commenting out #7 just makes #6 fire with the same target, etc, as #7.  Note sure where listener #2 went.  Again, I'd love to know what IE6 or IE7 does.
Comment 15 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-04 15:10:13 PDT
Created attachment 220843 [details]
Testcase for onclick for IE6

I had to mutilate the testcase to make it work for IE6.
Afaik, there is no currentTarget equivalent for IE6 so I omitted that one.
I get these results with this testcase:
body.onclick: BODY this: BODY
html: BODY this: HTML
document.onclick: BODY this: #document
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-04 15:38:10 PDT
How about I just change it to fire at the document, bubbling to the window, which is something that seems pretty clear we'll want to support one way or another. Then however the spec changes to support "<body ononline='...'>", we can add that later.
Comment 17 Hixie (not reading bugmail) 2006-05-04 15:43:15 PDT
The spec doesn't mention <body ononline=""> because I didn't know how to do it. It's mentioned as an XXX comment in the source.

I don't suggest we add window.ononline; that, as you point out, seems like asking for trouble.

I'd like this to all work in the way most consistent with other features, like onload="", that are in a similar vain; I don't currently know how to do that.
Comment 18 Boris Zbarsky [:bz] (TPAC) 2006-05-04 15:44:54 PDT
Well...  It's not clear to me, frankly, why this event bubbles to start with...  It's really not the sort of event that would generally bubble.  Why not just fire a non-bubbling event at the Window?

I'd really like to know what IE does for onload, since that does seem to be the closest analogue to what we have here
Comment 19 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-04 16:27:38 PDT
Created attachment 220857 [details]
testcase IE6 for onload

Sorry for the previous testcase, which is incorrect (I'm just not that used anymore in coding for IE6).
This tests onload.
Result: I get an alert for body and an alert for window.

This reminds me a bit of the discussion in bug 305160, btw.
Comment 20 Boris Zbarsky [:bz] (TPAC) 2006-05-04 19:33:41 PDT
Yes, but is window.onload the same as <body onload="..."> in IE?  What's the |this| object in those onload event handlers?
Comment 21 Boris Zbarsky [:bz] (TPAC) 2006-05-04 22:30:02 PDT
On a separate note:

../../../../mozilla/dom/src/base/nsGlobalWindow.cpp: In member function `
   virtual nsresult nsGlobalWindow::FireDelayedDOMEvents()':
../../../../mozilla/dom/src/base/nsGlobalWindow.cpp:5522: warning: control 
   reaches end of non-void function

I assume we just want to return NS_OK?
Comment 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-05-05 00:34:12 PDT
I still like the idea of firing against the root element. That way it is possible (now or in the future) to add declarative listeners like ononload="" or similar xmlevents syntax.

I don't see any harm in bubbling, and bubbling events are in general easier to use since you don't have to mess with capturing.
Comment 23 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-05 01:46:47 PDT
(In reply to comment #20)
> Yes, but is window.onload the same as <body onload="..."> in IE?  What's the
> |this| object in those onload event handlers?
Yes, window.onload and <body onload="..."> are the same in IE6.
Doing this:
<body onload="alert(this.nodeName)">
<script>
window.nodeName = 'window';
function doe() {
alert(this.nodeName);
}
window.onload=doe;
</script>
This only fires one alert from the window.onload event handler which overwrites the <body onload> event handler.

Comment 24 Boris Zbarsky [:bz] (TPAC) 2006-05-05 07:58:41 PDT
Martijn: Good to know.  That explains why we do what we do!

Jonas: Given what other UAs do, just firing against the root element will not trigger <body ononline=""> listeners.  As far as bubbling goes, what's the benefit of bubbling here, really?  And the harm with bubbling is that it makes the part we care about hard to specify, imo....

Ian, sicking, how does these proposed texts sound:

Bubbling event:  These events are in the http://www.w3.org/2001/xml-events  namespace, do bubble, are not cancelable, have no default action, and use the normal Event interface.  They must be dispatched in such a way that as long as stopPropagation() is not called at least the following listeners will trigger:

  1) Listeners set via <body ononline=""> and <body onoffline="">
  2) Listeners added for the online and offline events on the Window
 [3) Listeners declared on the root element?  Are we talking <html ononline="">
     here, or XML events, or something?  sicking, could you clarify what you
     want?]

  or

Non-bubbling event:  These events are in the http://www.w3.org/2001/xml-events  namespace, do not bubble, are not cancelable, have no default action, and use the normal Event interface.  They must be dispatched on the Window object and UAs must also ensure that event listeners registered via the "ononline" and "onoffline" attributes are triggered.  This specification does not constrain how the latter is done.  [Again, the root element caveat applies here...]
Comment 25 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-05-05 14:04:53 PDT
Won't doing <body ononline="..."> register a listener on the window? So we would in fact fire the listener no matter what.
Comment 26 Boris Zbarsky [:bz] (TPAC) 2006-05-05 15:06:05 PDT
> Won't doing <body ononline="..."> register a listener on the window?

Apparently not in all UAs, see the tests here.
Comment 27 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-05-05 15:59:40 PDT
Ok, so here's what I think we should do. We should fire the events on the <body> element for (x)html and on the root element otherwise. The events should bubble, be not cancelable, have no default action, and use the Event interface. The event should be named "online" and "offline" and be in the null namespace [1].

This will make handlers registered on the <body>, the root element, the document and the window fire since the window takes part in the event flow according to the Window spec.

[1] In the latest draft of DOM 3 Events all 'standard' events are in the null namespace, and listeners added through addEventListener only listen to events in the null namespace.
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-05 22:32:28 PDT
Apart from the namespace, that's exactly what the spec currently says.
Comment 29 jag (Peter Annema) 2006-05-15 04:29:31 PDT
I was about to report the same thing that was reported in comment 21.

So, is return NS_OK fine?
Comment 30 Boris Zbarsky [:bz] (TPAC) 2006-05-15 09:23:52 PDT
> Apart from the namespace, that's exactly what the spec currently says.

No, it's not....  It doesn't talk about firing at the root element, for example.

If we want to ship this as working in alpha 1, could we please decide what behavior we actually want and implement it?  What we have right now is pretty broken (eg <body ononline="..."> doesn't work).  If we don't care about this working in alpha 1, I don't have very strong views about whether we fire these events in alpha 1 or not, I guess...
Comment 31 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-15 11:08:07 PDT
(In reply to comment #30)
> > Apart from the namespace, that's exactly what the spec currently says.
> 
> No, it's not....  It doesn't talk about firing at the root element, for
> example.

True. But that is the only other difference.

> If we want to ship this as working in alpha 1, could we please decide what
> behavior we actually want and implement it?  What we have right now is pretty
> broken (eg <body ononline="..."> doesn't work). 

What we currently have is upwardly compatible with sicking's suggestion in comment #27 ... if someone registers an event handler that fires with this code, it will keep firing when comment #27 is implemented. So the situation isn't desperate IMHO.

Does everyone agree that comment #27 is what we should do? Ian?
Comment 32 Boris Zbarsky [:bz] (TPAC) 2006-05-15 11:15:08 PDT
I still think that we should just fire a single event.  Either a bubbling event on the root element or a non-bubbling event on the window.  <body ononline=..."> will Just Work because that handler gets set on the Window anyway in Gecko.  No need to mess with the body specially in this code.

The problem is that other UAs do things in various interesting ways wrt <body> event handlers (at least for onload), so I'm not sure what all Ian wants to specify here...
Comment 33 Hixie (not reading bugmail) 2006-05-15 11:47:33 PDT
So long as <body ononline=""> works in HTML and window.addEventListener('online', ...) works always, I'm happy. I'll make the spec compatible with whatever you set up.
Comment 34 Boris Zbarsky [:bz] (TPAC) 2006-05-15 11:59:13 PDT
One other concern -- if the document has no elements at all, should the window still get online/offline events?  I'd assume so....  In which case the simplest  thing for us to do is to fire to the Window.  sicking, not sure how this interacts with the XML events stuff.
Comment 35 Hixie (not reading bugmail) 2006-05-15 12:16:22 PDT
(we should also try to be consistent with other <body>/window events, like onload)
Comment 36 Boris Zbarsky [:bz] (TPAC) 2006-05-15 12:21:36 PDT
onload is a non-bubbling event fired at the Window only in Gecko, fwiw.
Comment 37 Blake Kaplan (:mrbkap) 2006-05-17 06:40:01 PDT
Brendan went ahead and checked in the |return NS_OK|;

A couple of random drive-by questions about a couple of the nsGlobalWindow.cpp portions of the patch:
  -- Why can we simply ignore the return value of nsContentUtils::DispatchTrustedEvent?
  -- Why say |nsCOMPtr<nsIDocument> doc(do_QueryInterface(mDocument))| when we already have mDoc on nsGlobalWindow?
Comment 38 Boris Zbarsky [:bz] (TPAC) 2006-06-08 07:45:08 PDT
It looks like this patch never initializes mFireOfflineStatusChangeEventOnThaw in the constructor...  Bug 320982 has a patch that fixes that.
Comment 39 Nickolay_Ponomarev 2007-05-11 02:46:34 PDT
jresig wrote http://developer.mozilla.org/en/docs/Online/Offline_Events

I haven't found any mochitests for this... Without looking at the code, I assume it's possible to force offline mode via the ioservice and test the events and navigator.onLine.
Comment 40 Nickolay_Ponomarev 2007-06-21 12:11:14 PDT
Tests were checked in as part of bug 336682.
Comment 41 William J. Edney 2008-03-30 22:16:03 PDT
Folks -

I know this bug has already been marked as FIXED, but I'm confused. I was under the impression that even in the FF 2.0 days, before these events were implemented, that navigator.onLine would change based on an 'autodetect' as to whether the network was up or not and would change its value accordingly. Also, given the testcase here, I was under the assumption that FF 3.0 would not only change the property, but fire these events as the network adaptor was enabled / disabled dynamically.

None of these things seem to occur when I enable/disable my network adaptor in the Control Panel in Window or in the Network system preferences panel on Mac (don't have a Linux box to test). My expectations are that in FF 2.0, when the network adaptor was taken down, the 'navigator.onLine' property would be false and that in FF 3.0, not only would the property be false, but I'd get the event.

I apologize if I'm spamming this bug with dumb questions, but I'm not seeing the behavior I thought I'd see and I very well may be missing something here.

If someone knows of a bug that is better suited to these kinds of questions, let me know.

Thanks!

Cheers,

- Bill
Comment 42 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-03-31 00:50:00 PDT
Not sure if the firefox online/offline state is hooked up to the system online/offline state. It might vary between platforms too. I suggest you file a separate bug on that unless you can find a filed one already.

In any even this bug is the wrong place for this discussion. Please ask in the mozilla.dev.tech.dom newsgroup if you are having trouble.

Note You need to log in before you can comment on or make changes to this bug.