Closed Bug 336359 Opened 18 years ago Closed 18 years ago

Fire WHATWG DOM events when going online or offline

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 1 obsolete file)

fix
18.50 KB, patch
darin.moz
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
testcase
1.01 KB, text/html
Details
First testcase: onload
1.02 KB, text/html
Details
Testcase for onclick
1.05 KB, text/html
Details
testcase IE6 for onload
436 bytes, text/html
Details 
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.
Attached patch fix — — Splinter Review 
darin, feel free to suggest another reviewer if this isn't up your alley
Attachment #220608 - Flags: superreview?(darin)
Attachment #220608 - Flags: review?(darin)
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
Attachment #220608 - Flags: superreview?(darin)
Attachment #220608 - Flags: superreview+
Attachment #220608 - Flags: review?(darin)
Attachment #220608 - Flags: review+
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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.  :(
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.
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?
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.
Alternativly the root element might make sense since you can declarativly attach handlers there.
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.
Depends on: 336605
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. :-)
> 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.
Attached file 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.
Attached file 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.
Attached file Testcase for onclick for IE6 (obsolete) — 
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
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.
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.
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
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.
Attachment #220843 - Attachment is obsolete: true
Yes, but is window.onload the same as <body onload="..."> in IE?  What's the |this| object in those onload event handlers?
Blocks: 336682
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?
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.
(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.


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...]
Won't doing <body ononline="..."> register a listener on the window? So we would in fact fire the listener no matter what.
> Won't doing <body ononline="..."> register a listener on the window?

Apparently not in all UAs, see the tests here.
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.
Apart from the namespace, that's exactly what the spec currently says.
I was about to report the same thing that was reported in comment 21.

So, is return NS_OK fine?
> 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...
(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?
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...
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.
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.
(we should also try to be consistent with other <body>/window events, like onload)
onload is a non-bubbling event fired at the Window only in Gecko, fwiw.
Depends on: 338255
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?
It looks like this patch never initializes mFireOfflineStatusChangeEventOnThaw in the constructor...  Bug 320982 has a patch that fixes that.
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.
Flags: in-testsuite?
Keywords: dev-doc-complete
Tests were checked in as part of bug 336682.
Flags: in-testsuite? → in-testsuite+
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
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: