Closed
Bug 1264769
Opened 8 years ago
Closed 8 years ago
Image elements should dispatch loadstart and loadend events
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: jdm, Assigned: ben.tian)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [tw-dom] btpp-active)
Attachments
(2 files, 5 obsolete files)
According to the specification (https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data) in step 10 we should be dispatching a loadstart event at the image element, and we should be dispatching a loadend event after any error or load event after that. Neither Blink nor Safari seem to implement this yet, according to http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4061 .
Updated•8 years ago
|
Whiteboard: [tw-dom] btpp-fixlater
Assignee | ||
Comment 1•8 years ago
|
||
Assignee: nobody → btian
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Whiteboard: [tw-dom] btpp-fixlater → [tw-dom] btpp-active
Assignee | ||
Comment 3•8 years ago
|
||
Rebase on the latest m-c.
Attachment #8758657 -
Attachment is obsolete: true
Assignee | ||
Comment 4•8 years ago
|
||
Rebase on the latest m-c.
Attachment #8758658 -
Attachment is obsolete: true
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8769955 [details] [diff] [review] Patch 1 (v2): Dispatch loadend event for image loading Henri, Can you review my patches that make images elements dispatch loadstart & loadend events per comment 0? Patch 1 dispatches loadend event and Patch 2 dispatches loadstart event. Try result is in https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fb0066a73f5
Attachment #8769955 -
Flags: review?(hsivonen)
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8769956 [details] [diff] [review] [final] Patch 2: Dispatch loadstart event for image loading, r=hsivonen Patch 2 adds an argument |aLoadStart| in method |nsImageLoadingContent::LoadImage| [1] to indicate whether to fire loadstart event here. |aLoadStart| is true by default and set to false if [2] already fired loadstart event. [1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsImageLoadingContent.cpp#785 [2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsImageLoadingContent.cpp#739
Attachment #8769956 -
Flags: review?(hsivonen)
Comment 7•8 years ago
|
||
Comment on attachment 8769955 [details] [diff] [review] Patch 1 (v2): Dispatch loadend event for image loading Review of attachment 8769955 [details] [diff] [review]: ----------------------------------------------------------------- A note in the spec says: "The progress and loadend events are not fired for multipart/x-mixed-replace image streams." How is this taken care of in the code? ::: dom/webidl/EventHandler.webidl @@ +59,5 @@ > attribute EventHandler onkeyup; > attribute EventHandler onload; > attribute EventHandler onloadeddata; > attribute EventHandler onloadedmetadata; > + attribute EventHandler onloadend; I don't see this in the spec. Is this a spec bug or a patch bug?
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #7) > A note in the spec says: "The progress and loadend events are not fired for > multipart/x-mixed-replace image streams." How is this taken care of in the > code? I missed that note. Cancel review first and will revise my patch. > ::: dom/webidl/EventHandler.webidl > @@ +59,5 @@ > > attribute EventHandler onkeyup; > > attribute EventHandler onload; > > attribute EventHandler onloadeddata; > > attribute EventHandler onloadedmetadata; > > + attribute EventHandler onloadend; > > I don't see this in the spec. Is this a spec bug or a patch bug? Do you mean the additional onloadend event handler? Comment 0 mentions to fire loadend event after any error or load event, so I add onloadend event handler at the same place as existing onload event handler [1] of image element [2][3]. [1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/EventHandler.webidl#60 [2] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLImageElement.webidl#21 [3] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLElement.webidl#103
Assignee | ||
Updated•8 years ago
|
Attachment #8769955 -
Flags: review?(hsivonen)
Comment 9•8 years ago
|
||
(In reply to Ben Tian [:btian] from comment #8) > (In reply to Henri Sivonen (:hsivonen) from comment #7) > > A note in the spec says: "The progress and loadend events are not fired for > > multipart/x-mixed-replace image streams." How is this taken care of in the > > code? > > I missed that note. Cancel review first and will revise my patch. > > > ::: dom/webidl/EventHandler.webidl > > @@ +59,5 @@ > > > attribute EventHandler onkeyup; > > > attribute EventHandler onload; > > > attribute EventHandler onloadeddata; > > > attribute EventHandler onloadedmetadata; > > > + attribute EventHandler onloadend; > > > > I don't see this in the spec. Is this a spec bug or a patch bug? > > Do you mean the additional onloadend event handler? Yes. > Comment 0 mentions to > fire loadend event after any error or load event, so I add onloadend event > handler at the same place as existing onload event handler [1] of image > element [2][3]. But not all events are supposed to have corresponding onfoo handlers. Since the spec has onloadstart but onloadend, it's likely that this is a spec bug, but we should find out!
Comment 10•8 years ago
|
||
I filed a spec bug: https://github.com/whatwg/html/issues/1551 . Let's see what the outcome of the spec bug is.
See Also: → https://github.com/whatwg/html/issues/1551
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #9) > But not all events are supposed to have corresponding onfoo handlers. Since > the spec has onloadstart but onloadend, it's likely that this is a spec bug, > but we should find out! I don't get it. It's gecko implementation that has onloadstart only but the spec defines both loadstart and loadend event handlers, right? Also gecko's onloadstart seems for media element [1] instead for image element only. [1] Step 8 in https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-load-algorithm
Flags: needinfo?(hsivonen)
Reporter | ||
Comment 12•8 years ago
|
||
The spec says to fire "loadstart" and "loadend" events. The specification's WebIDL for GlobalEventHandlers only includes an EventHandler attribute for onloadstart, however: https://html.spec.whatwg.org/multipage/webappapis.html#globaleventhandlers .
Comment 14•8 years ago
|
||
The spec was changed to include onloadend, so it's now OK to add it to Gecko, too.
Assignee | ||
Comment 15•8 years ago
|
||
Change: - do not fire loadend event for multipart/x-mixed-replace image streams
Attachment #8769955 -
Attachment is obsolete: true
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8772663 [details] [diff] [review] [final] Patch 1: Dispatch loadend event for image loading, r=hsivonen Henri, Can you review my revised loadend event patch that handles multipart/x-mixed-replace image streams? I've verified with bug 733553 test case [1] that no loadend event is dispatched after load events for multipart/x-mixed-replaced image streams. [1] https://dxr.mozilla.org/mozilla-central/source/image/test/mochitest/test_bug733553.html Try result is in https://treeherder.mozilla.org/#/jobs?repo=try&revision=8552eb18a6ed
Attachment #8772663 -
Flags: review?(hsivonen)
Updated•8 years ago
|
Attachment #8769956 -
Flags: review?(hsivonen) → review+
Updated•8 years ago
|
Attachment #8772663 -
Flags: review?(hsivonen) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8769956 -
Attachment description: Patch 2 (v2): Dispatch loadstart event for image loading → [final] Patch 2: Dispatch loadstart event for image loading, r=hsivonen
Assignee | ||
Updated•8 years ago
|
Attachment #8772663 -
Attachment description: Patch 1 (v3): Dispatch loadend event for image loading → [final] Patch 1: Dispatch loadend event for image loading, r=hsivonen
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 17•8 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/780f7036c084 Part 1: Dispatch loadend event for image loading. r=hsivonen https://hg.mozilla.org/integration/mozilla-inbound/rev/7b9d3d0c09f5 Part 2: Dispatch loadstart event for image loading. r=hsivonen
Keywords: checkin-needed
Backed out for apparently causing wpt tests to fail like https://treeherder.mozilla.org/logviewer.html#?job_id=33865970&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/ba5c202573c0
Flags: needinfo?(btian)
Assignee | ||
Comment 19•8 years ago
|
||
Change: - remove invalid-src.html.ini for the web platform test since it becomes passed Quick verification on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9284ee06408f
Attachment #8772663 -
Attachment is obsolete: true
Assignee | ||
Comment 20•8 years ago
|
||
Re-try of comment 19 patch on all tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=632ecda014e3 Set checkin-needed for updated patches.
Flags: needinfo?(btian)
Keywords: checkin-needed
Comment 21•8 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a4313b1f4a54 Part 1: Dispatch loadend event for image loading. r=hsivonen https://hg.mozilla.org/integration/mozilla-inbound/rev/ee163e1d915a Part 2: Dispatch loadstart event for image loading. r=hsivonen
Keywords: checkin-needed
Comment 22•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a4313b1f4a54 https://hg.mozilla.org/mozilla-central/rev/ee163e1d915a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee | ||
Comment 23•8 years ago
|
||
Depend on bug 1292063 per bug 1298725 comment 8.
Depends on: 1292063
Updated•8 years ago
|
Comment 24•8 years ago
|
||
Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/05cf3d96ba3c Backed out changeset ee163e1d915a for fixing regression of bug 1298725
Comment 25•8 years ago
|
||
backout bugherder |
Merge of backout: https://hg.mozilla.org/mozilla-central/rev/05cf3d96ba3c
Updated•8 years ago
|
Target Milestone: mozilla51 → ---
Assignee | ||
Comment 26•8 years ago
|
||
Rebase on the latest m-c since dependent bug 1292063 is resolved. Verified try result [1] again and video playback of bug 1298725 link [2]. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=538ca2f28eca [2] http://www.exploratorium.edu/strandbeest
Attachment #8769956 -
Attachment is obsolete: true
Comment 28•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a1444fdc6ab3 Patch 2: Dispatch loadstart event for image loading, r=hsivonen
Keywords: checkin-needed
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a1444fdc6ab3
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 30•7 years ago
|
||
I have documented these additions, by adding entries for the event handlers to: https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers Creating pages for the handlers: https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onloadstart https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onloadend Updating the event pages: https://developer.mozilla.org/en-US/docs/Web/Events/loadstart https://developer.mozilla.org/en-US/docs/Web/Events/loadend And adding a note to the Fx52 release notes: https://developer.mozilla.org/en-US/Firefox/Releases/52#DOM_HTML_DOM Let me know if these look OK. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 32•7 years ago
|
||
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #30) > Creating pages for the handlers: > https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/ > onloadstart > https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/ > onloadend In above two, the event handlers should be fired on image element (like [1]) instead of window. The rest looks good to me. [1] http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4061
Flags: needinfo?(btian) → needinfo?(cmills)
Comment 33•7 years ago
|
||
(In reply to Ben Tian [:btian] from comment #32) > (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #30) > > Creating pages for the handlers: > > https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/ > > onloadstart > > https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/ > > onloadend > > In above two, the event handlers should be fired on image element (like [1]) > instead of window. The rest looks good to me. > > [1] http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4061 Ah, thanks for catching that. Fixed now.
Flags: needinfo?(cmills)
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•