Closed Bug 1457166 Opened 2 years ago Closed 2 years ago

Figure out what's the right target of the online / offline events.

Categories

(Core :: DOM: Events, enhancement, P2)

Product:
Core
Component:
DOM: Events
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: site-compat)

Attachments

(1 file)

Bug 1457166: Fire online / offline events at the window.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
Per https://html.spec.whatwg.org/#event-offline we should fire it on the window, but we currently try the <body> (only on X/HTML documents), then the doc element, then the document.

Boris spotted this in bug 1455885.

This bug is to figure out what's going on with those.
(In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05) from comment #0)
> Per https://html.spec.whatwg.org/#event-offline we should fire it on the
> window, but we currently try the <body> (only on X/HTML documents), then the
> doc element, then the document.

That's not quite right, the logic is something like: try <body> if it's an X/HTML document, doc element if not. So in an HTML doc without a body the target would be the document, not the doc element.
So I checked the blame for this code, and it is:

  Bug 336359. Fire WHATWG online/offline events when the browser offline status changes. r+sr=darin
  roc+%cs.cmu.edu <roc+%cs.cmu.edu>

So asking the authors of the code here would probably not be too helpful.  ;)

I did test and Chrome targets the window with the online/offline events.  So does Safari (which is annoying to test because it seems to not have a built-in online mode toggle; I had to turn off networking in the OS to test it).

Sounds like we should just switch to firing at the window.
Assignee: nobody → emilio
Comment on attachment 8971319 [details]
Bug 1457166: Fire online / offline events at the window.

https://reviewboard.mozilla.org/r/240084/#review245900

::: dom/base/nsGlobalWindowInner.cpp:5237
(Diff revision 1)
>      name.AssignLiteral("online");
>    }
> -  // The event is fired at the body element, or if there is no body element,
> -  // at the document.
> -  nsCOMPtr<EventTarget> eventTarget = mDoc.get();
> -  if (mDoc->IsHTMLOrXHTML()) {
> +  nsContentUtils::DispatchTrustedEvent(mDoc,
> +                                       static_cast<EventTarget*>(this),
> +                                       name,
> +                                       true,

Per spec, I believe canBubble here should be false.  Spec says to "fire an event" with nothing else mentioned, which uses the default values in https://dom.spec.whatwg.org/#dictdef-eventinit
Attachment #8971319 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> Comment on attachment 8971319 [details]
> Bug 1457166: Fire online / offline events at the window.
> 
> https://reviewboard.mozilla.org/r/240084/#review245900
> 
> ::: dom/base/nsGlobalWindowInner.cpp:5237
> (Diff revision 1)
> >      name.AssignLiteral("online");
> >    }
> > -  // The event is fired at the body element, or if there is no body element,
> > -  // at the document.
> > -  nsCOMPtr<EventTarget> eventTarget = mDoc.get();
> > -  if (mDoc->IsHTMLOrXHTML()) {
> > +  nsContentUtils::DispatchTrustedEvent(mDoc,
> > +                                       static_cast<EventTarget*>(this),
> > +                                       name,
> > +                                       true,
> 
> Per spec, I believe canBubble here should be false.  Spec says to "fire an
> event" with nothing else mentioned, which uses the default values in
> https://dom.spec.whatwg.org/#dictdef-eventinit

Yeah, blink does that as well, so will change:

  https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/dom/document.cc?l=554&rcl=0605d3e7008461baaf5e0f7647c99ec5053c9ec4
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b543167ab064
Fire online / offline events at the window. r=bz
Priority: -- → P2
Backed out changeset b543167ab064 (bug 1457166) for mochitest failures on test_bug336682_1.html

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/faf594a73b96eddc467938ec537c6f29265f31ef

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b543167ab064bc66d789e54711243fda677a453f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&selectedJob=175956464

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=175956464&repo=mozilla-inbound&lineNumber=3282

[task 2018-04-27T12:12:49.657Z] 12:12:49     INFO - TEST-START | dom/events/test/test_bug336682_1.html
[task 2018-04-27T12:12:49.800Z] 12:12:49     INFO - GECKO(1641) | ++DOMWINDOW == 51 (0x7fa51f407800) [pid = 1687] [serial = 87] [outer = 0x7fa5200b9c00]
[task 2018-04-27T12:12:50.105Z] 12:12:50     INFO - TEST-INFO | started process screentopng
[task 2018-04-27T12:12:51.058Z] 12:12:51     INFO - TEST-INFO | screentopng: exit 0
[task 2018-04-27T12:12:51.061Z] 12:12:51     INFO - Buffered messages logged at 12:12:50
[task 2018-04-27T12:12:51.062Z] 12:12:51     INFO - TEST-PASS | dom/events/test/test_bug336682_1.html | navigator.onLine should be true, since we've just set nsIIOService.offline to false 
[task 2018-04-27T12:12:51.063Z] 12:12:51     INFO - Buffered messages finished
[task 2018-04-27T12:12:51.064Z] 12:12:51     INFO - TEST-UNEXPECTED-FAIL | dom/events/test/test_bug336682_1.html | handlers called in the right order: <body onoffline='...'> is called, gState=1, expectedStates=3,4 
[task 2018-04-27T12:12:51.064Z] 12:12:51     INFO -     makeHandler/<@dom/events/test/test_bug336682.js:36:5
[task 2018-04-27T12:12:51.065Z] 12:12:51     INFO -     makeBodyHandler/<@dom/events/test/test_bug336682_1.html:32:5
[task 2018-04-27T12:12:51.065Z] 12:12:51     INFO -     onoffline@dom/events/test/test_bug336682_1.html:1:32
[task 2018-04-27T12:12:51.066Z] 12:12:51     INFO -     doTest@dom/events/test/test_bug336682.js:58:3
[task 2018-04-27T12:12:51.069Z] 12:12:51     INFO -     @dom/events/test/test_bug336682_1.html:58:3
[task 2018-04-27T12:12:51.070Z] 12:12:51     INFO -     rval@SimpleTest/SimpleTest.js:146:17
[task 2018-04-27T12:12:51.071Z] 12:12:51     INFO -     EventHandlerNonNull*this.addLoadEvent@SimpleTest/SimpleTest.js:171:13
[task 2018-04-27T12:12:51.074Z] 12:12:51     INFO -     @SimpleTest/SimpleTest.js:1444:5
Flags: needinfo?(emilio)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb451c84fd3c
Fire online / offline events at the window. r=bz
Ugh, the tests are duplicated for xul and html... Fixed.
Flags: needinfo?(emilio)
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10702 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/eb451c84fd3c
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.