Closed Bug 336682 Opened 18 years ago Closed 17 years ago

Need to sort out behavior of WHATWG online/offline events

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: bzbarsky, Assigned: cajbir)

References

Details

Attachments

(5 files, 4 obsolete files)

offline event testcase
1.09 KB, text/html
Details
online event testcase
1.08 KB, text/html
Details
SVG offline/online testcase
1.05 KB, image/svg+xml
Details
final version of patch
10.49 KB, patch
Details | Diff | Splinter Review
add mochitests, updated version
9.59 KB, patch
Details | Diff | Splinter Review 
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
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?
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.
Attached file offline event testcase — 

    
    

    
  

      
      
      
      

        Attached file
          
        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.



    
    

    
  
> 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.

    
    

    
  
Thanks, I'm on the case and will do a new patch.

    
    

    
  


  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        version 2 of offline event patch (obsolete)
        — 
          — Splinter Review
      

    


  
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?

    
  

        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+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        version 3 of offline event patch (obsolete)
        — 
          — Splinter Review
      

    


  
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+

    
    

    
  


  
Removes the .get() as recommended.

        Attachment #258381 -
        Attachment is obsolete: true

    
    

    
  
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED

    
    

    
  
Removing the .get() broke gcc builds. I reverted that to fix the bustage.

    
    

    
  
(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

    
    

    
  
(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?


    
    

    
  

      
      
      
      

        Attached patch
          
          
        add mochitests (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
  
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?

    
    

    
  
bz: I filed bug 380618 with a testcase.
Blocks: 380618

        Attachment #264623 -
        Flags: superreview?(jst)

        Attachment #264623 -
        Flags: superreview+

        Attachment #264623 -
        Flags: review?(jst)

        Attachment #264623 -
        Flags: review+

    
    

    
  


  

        Attachment #264623 -
        Attachment is obsolete: true

    
    

    
  
Checked the tests in.
Flags: in-testsuite? → in-testsuite+
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: