Closed
Bug 1457166
Opened 3 years ago
Closed 3 years ago
Figure out what's the right target of the online / offline events.
Categories
(Core :: DOM: Events, enhancement, P2)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
(Keywords: site-compat)
Attachments
(1 file)
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.
Assignee | ||
Comment 1•3 years ago
|
||
(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.
![]() |
||
Comment 2•3 years ago
|
||
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 | ||
Updated•3 years ago
|
Assignee: nobody → emilio
Comment hidden (mozreview-request) |
![]() |
||
Comment 4•3 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 5•3 years ago
|
||
(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
Updated•3 years ago
|
Priority: -- → P2
Comment 7•3 years ago
|
||
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
Assignee | ||
Comment 9•3 years ago
|
||
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.
Comment 12•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eb451c84fd3c
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Upstream PR merged
Comment 14•3 years ago
|
||
Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/online-offline-events-are-no-longer-fired-on-document-and-document-body/
Keywords: site-compat
You need to log in
before you can comment on or make changes to this bug.
Description
•