Last Comment Bug 336682 - Need to sort out behavior of WHATWG online/offline events
: Need to sort out behavior of WHATWG online/offline events
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: cajbir (:cajbir)
: Hixie (not reading bugmail)
Mentors:
: 340616 (view as bug list)
Depends on: 336359
Blocks: 380538 380618
  Show dependency treegraph
 
Reported: 2006-05-04 19:38 PDT by Boris Zbarsky [:bz]
Modified: 2015-05-26 20:24 PDT (History)
15 users (show)
dbaron: blocking1.9+
asqueella: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Adds 'online=...', 'onoffline=...' (8.66 KB, patch)
2007-02-27 20:53 PST, cajbir (:cajbir)
no flags Details | Diff | Splinter Review
offline event testcase (1.09 KB, text/html)
2007-02-27 20:57 PST, cajbir (:cajbir)
no flags Details
online event testcase (1.08 KB, text/html)
2007-02-27 20:57 PST, cajbir (:cajbir)
no flags Details
SVG offline/online testcase (1.05 KB, image/svg+xml)
2007-03-04 15:03 PST, cajbir (:cajbir)
no flags Details
version 2 of offline event patch (9.60 KB, patch)
2007-03-04 15:11 PST, cajbir (:cajbir)
jst: review-
dbaron: review+
jst: superreview-
Details | Diff | Splinter Review
version 3 of offline event patch (10.50 KB, patch)
2007-03-12 21:50 PDT, cajbir (:cajbir)
jst: review+
jst: superreview+
Details | Diff | Splinter Review
final version of patch (10.49 KB, patch)
2007-03-13 18:12 PDT, cajbir (:cajbir)
no flags Details | Diff | Splinter Review
add mochitests (9.76 KB, patch)
2007-05-12 16:45 PDT, Nickolay_Ponomarev
jst: review+
jst: superreview+
Details | Diff | Splinter Review
add mochitests, updated version (9.59 KB, patch)
2007-06-21 12:08 PDT, Nickolay_Ponomarev
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2006-05-04 19:38:07 PDT
Bug tracking discussion in bug 336359 to make sure we don't ship an alpha and then change things around....
Comment 1 Boris Zbarsky [:bz] 2006-06-06 20:52:43 PDT
*** Bug 340616 has been marked as a duplicate of this bug. ***
Comment 2 Boris Zbarsky [:bz] 2006-12-07 19:02:58 PST
So I still think we need to fix this a lot earlier than 1.9 final...
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2007-01-18 16:40:18 PST
Roc, it'd be great if you could fix this before 1.9 beta
Comment 4 cajbir (:cajbir) 2007-02-27 20:53:04 PST
Created attachment 256753 [details] [diff] [review]
Adds 'online=...', 'onoffline=...' 

This patch allows:

<body onoffline=... ononline=... >
document.body.onoffline=...
document.body.ononline=...
Comment 5 cajbir (:cajbir) 2007-02-27 20:56:03 PST
The list from Bug 340616 was:

1)  <body ononline="">  should work
2)  Sort out where to actually fire the event and whether it bubbles
3)  Blake's stuff from bug 336359 comment 37

Number (1) is covered by the attached patch. Number (2) works as per bug 336359 comment 27. Attached 2 test cases that show the events for offline and online. These are the same cases from bug 336359 but modified for working on the offline and online events.
Comment 6 cajbir (:cajbir) 2007-02-27 20:57:04 PST
Created attachment 256754 [details]
offline event testcase
Comment 7 cajbir (:cajbir) 2007-02-27 20:57:32 PST
Created attachment 256755 [details]
online event testcase
Comment 8 Boris Zbarsky [:bz] 2007-02-27 21:20:44 PST
> Number (2) works as per bug 336359 comment 27. 

Not unless you're changing it in your patch....  Note that said comment discusses both the HTML and the non-HTML case, so testing both is necessary to see whether it works per that comment.  In particular, the non-HTML case doesn't work as that comment describes.  If we do change to do what bug 336359 comment 27 says, note also bug 336359 comment 34.

Comment 9 cajbir (:cajbir) 2007-02-27 22:18:25 PST
> In particular, the non-HTML case doesn't work as that comment describes. 

What is a non-HTML case where events can be added? I confess I'm only familiar with HTML in the browser. Is it SVG? Something else?

I'll make the necessary changes regarding comment 34 as well.
Comment 10 Boris Zbarsky [:bz] 2007-02-27 22:32:19 PST
> What is a non-HTML case where events can be added? 

Pretty much any XML document; just add them via addEventListener.  You'll need a script in a namespace that we know about like XHTML or whatnot, but the _document_ need not be HTML.  As a simple test, take a valid XHTML document that adds the events, save it with a .xml extension (or for a web server send is as application/xml instead of application/xhtml+xml).

But yes, SVG would do fine too.
Comment 11 cajbir (:cajbir) 2007-02-27 22:35:46 PST
Thanks, I'm on the case and will do a new patch.
Comment 12 cajbir (:cajbir) 2007-03-04 15:03:21 PST
Created attachment 257277 [details]
SVG offline/online testcase

This adds events to the document root, the window and the document. The implementation of the offline event sending should send to the document root for non-html documents according to bug 336359 comment 27.
Comment 13 cajbir (:cajbir) 2007-03-04 15:11:18 PST
Created attachment 257278 [details] [diff] [review]
version 2 of offline event patch

Fixes previous patch to fire events to the root element of non HTML documents. if the document is an HTML document with no body, or a non-HTML document with no elements, the document gets the event.
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2007-03-12 15:50:07 PDT
Comment on attachment 257278 [details] [diff] [review]
version 2 of offline event patch

- In nsGlobalWindow::FireOfflineStatusEvent():
+  else {
+    nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(doc);
+    if(domDoc) {

No need for this QI, mDocument is already an nsIDOMDocument, so use it, and it's already null-checked at the top of this function. Also, earlier in this function you QI mDocument to an nsIDocument for the local variable "doc", but this class has an mDoc member of that type already set up for you. So please use that directly instead.

- In CSSParserImpl::ParseAttributeSelector():

+              "onoffline",
+              "ononline",

I'm not sure these belong in this list. Dbaron would know, requesting additional review from him for this portion only.

r- based on the first comment above, fix that and I'll stamp this (assuming dbaron agrees with the last chunk in this diff).
Comment 15 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-03-12 16:23:26 PDT
Comment on attachment 257278 [details] [diff] [review]
version 2 of offline event patch

Adding those to the list of case sensitive attributes is fine, although it should become irrelevant once we fix bug 357614.
Comment 16 cajbir (:cajbir) 2007-03-12 21:50:04 PDT
Created attachment 258381 [details] [diff] [review]
version 3 of offline event patch

Fixed the points raised in the review.
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2007-03-12 22:40:05 PDT
Comment on attachment 258381 [details] [diff] [review]
version 3 of offline event patch

- In nsGlobalWindow::FireOfflineStatusEvent():

+  nsCOMPtr<nsISupports> eventTarget = mDoc.get();

I forgot to mention this last time. You shouldn't need that .get() there (unless the code doesn't compile w/o it, which I wouldn't think is the case here).

Looks good, r+sr=jst!
Comment 18 cajbir (:cajbir) 2007-03-13 18:12:27 PDT
Created attachment 258487 [details] [diff] [review]
final version of patch

Removes the .get() as recommended.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-03-13 18:44:23 PDT
Checked in.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-03-13 19:01:30 PDT
Removing the .get() broke gcc builds. I reverted that to fix the bustage.
Comment 21 Nickolay_Ponomarev 2007-05-11 03:21:52 PDT
(In reply to comment #5)
> The list from Bug 340616 was:
> 
> 1)  <body ononline="">  should work
> 2)  Sort out where to actually fire the event and whether it bubbles
> 3)  Blake's stuff from bug 336359 comment 37
> 
For the sake of people reading this bug later, the updated specification is at 
http://www.whatwg.org/specs/web-apps/current-work/#offline

Our implementation seems to match the current description: "These events are in no namespace, do bubble, are not cancelable, have no default action, and use the normal Event interface. They must be fired on the body element. (As the events bubble, they will reach the Window object.)"

I'll try to write the mochitests for these this weekend.

The spec still doesn't mention "ononline" handlers (http://www.whatwg.org/specs/web-apps/current-work/#event3), while we implement those, while correctly ignoring window.ononline per bug 336359 comment 17. The XXX comment regarding ononline/onoffline handlers is still in the spec source, so I think they should also be tested.
Comment 22 Nickolay_Ponomarev 2007-05-12 10:11:23 PDT
(In reply to comment #21)
> those, while correctly ignoring window.ononline per bug 336359 comment 17.

I was mistaken, we actually don't treat 'ononline' properly. Like with 'onload', |window.ononline| returns the handler function for <body ononline="..."> when that is specified (this is text/html with a <!DOCTYPE HTML>). I don't see anything in the spec about that.

Also if you set window.ononline and don't specify a handler on the <body>, it gets called. Is any of these things a bug?
Comment 23 Nickolay_Ponomarev 2007-05-12 16:45:59 PDT
Created attachment 264623 [details] [diff] [review]
add mochitests

Test stuff. There are todo()s in the testcase. I'm not sure if window.ononline is a bug, but I'll file the other bug (with ononline="..." attribute not working sometimes) now.
Comment 24 Boris Zbarsky [:bz] 2007-05-13 19:53:48 PDT
> Also if you set window.ononline and don't specify a handler on the <body>, it
> gets called.

Er, really?  Does that break pages that try to have a |var ononline| or that assign something like a number to a global ononline variable?  See bug 336359 comment 12.

If it does, is that really what we want?
Comment 25 Nickolay_Ponomarev 2007-05-14 01:33:18 PDT
bz: I filed bug 380618 with a testcase.
Comment 26 Nickolay_Ponomarev 2007-06-21 12:08:14 PDT
Created attachment 269257 [details] [diff] [review]
add mochitests, updated version
Comment 27 Nickolay_Ponomarev 2007-06-21 12:10:32 PDT
Checked the tests in.

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