The default bug view has changed. See this FAQ.

Need to sort out behavior of WHATWG online/offline events

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
11 years ago
2 years ago

People

(Reporter: bz, Assigned: cajbir)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 4 obsolete attachments)

Bug tracking discussion in bug 336359 to make sure we don't ship an alpha and then change things around....
Flags: blocking1.9a1?
*** Bug 340616 has been marked as a duplicate of this bug. ***
Flags: blocking1.9a1? → blocking1.9+
So I still think we need to fix this a lot earlier than 1.9 final...
Roc, it'd be great if you could fix this before 1.9 beta
Assignee: general → roc
(Assignee)

Comment 4

10 years ago
Created attachment 256753 [details] [diff] [review]
Adds 'online=...', 'onoffline=...' 

This patch allows:

<body onoffline=... ononline=... >
document.body.onoffline=...
document.body.ononline=...
Assignee: roc → chris.double
Status: NEW → ASSIGNED
Attachment #256753 - Flags: superreview?
Attachment #256753 - Flags: review?
(Assignee)

Comment 5

10 years ago
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.
(Assignee)

Comment 6

10 years ago
Created attachment 256754 [details]
offline event testcase
(Assignee)

Comment 7

10 years ago
Created attachment 256755 [details]
online event testcase
> 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.

(Assignee)

Comment 9

10 years ago
> 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.
> 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.
(Assignee)

Comment 11

10 years ago
Thanks, I'm on the case and will do a new patch.
(Assignee)

Comment 12

10 years ago
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.
(Assignee)

Comment 13

10 years ago
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.
Attachment #256753 - Attachment is obsolete: true
Attachment #256753 - Flags: superreview?
Attachment #256753 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #257278 - Flags: superreview?(jst)
Attachment #257278 - Flags: review?(jst)
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).
Attachment #257278 - Flags: superreview?(jst)
Attachment #257278 - Flags: superreview-
Attachment #257278 - Flags: review?(jst)
Attachment #257278 - Flags: review?(dbaron)
Attachment #257278 - Flags: review-
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.
Attachment #257278 - Flags: review?(dbaron) → review+
(Assignee)

Comment 16

10 years ago
Created attachment 258381 [details] [diff] [review]
version 3 of offline event patch

Fixed the points raised in the review.
Attachment #257278 - Attachment is obsolete: true
Attachment #258381 - Flags: superreview?(jst)
Attachment #258381 - Flags: review?(jst)
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!
Attachment #258381 - Flags: superreview?(jst)
Attachment #258381 - Flags: superreview+
Attachment #258381 - Flags: review?(jst)
Attachment #258381 - Flags: review+
(Assignee)

Comment 18

10 years ago
Created attachment 258487 [details] [diff] [review]
final version of patch

Removes the .get() as recommended.
Attachment #258381 - Attachment is obsolete: true
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Removing the .get() broke gcc builds. I reverted that to fix the bustage.

Comment 21

10 years ago
(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.
Flags: in-testsuite?
OS: Linux → All
Hardware: PC → All

Comment 22

10 years ago
(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

10 years ago
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.
Attachment #264623 - Flags: superreview?(jst)
Attachment #264623 - Flags: review?(jst)

Updated

10 years ago
Blocks: 380538
> 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

10 years ago
bz: I filed bug 380618 with a testcase.
Blocks: 380618

Updated

10 years ago
Attachment #264623 - Flags: superreview?(jst)
Attachment #264623 - Flags: superreview+
Attachment #264623 - Flags: review?(jst)
Attachment #264623 - Flags: review+

Comment 26

10 years ago
Created attachment 269257 [details] [diff] [review]
add mochitests, updated version
Attachment #264623 - Attachment is obsolete: true

Comment 27

10 years ago
Checked the tests in.
Flags: in-testsuite? → in-testsuite+

Comment 28

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/420bc7afeb18
You need to log in before you can comment on or make changes to this bug.