Closed Bug 1264769 Opened 4 years ago Closed 3 years ago

Image elements should dispatch loadstart and loadend events

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

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 .
Whiteboard: [tw-dom] btpp-fixlater
Assignee: nobody → btian
Whiteboard: [tw-dom] btpp-fixlater → [tw-dom] btpp-active
Rebase on the latest m-c.
Attachment #8758657 - Attachment is obsolete: true
Rebase on the latest m-c.
Attachment #8758658 - Attachment is obsolete: true
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)
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 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?
(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
Attachment #8769955 - Flags: review?(hsivonen)
(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!
I filed a spec bug: https://github.com/whatwg/html/issues/1551 . Let's see what the outcome of the spec bug is.
(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)
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 .
I see. Thanks for pointing out.
Flags: needinfo?(hsivonen)
The spec was changed to include onloadend, so it's now OK to add it to Gecko, too.
Change:
- do not fire loadend event for multipart/x-mixed-replace image streams
Attachment #8769955 - Attachment is obsolete: true
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)
Attachment #8769956 - Flags: review?(hsivonen) → review+
Attachment #8772663 - Flags: review?(hsivonen) → review+
Attachment #8769956 - Attachment description: Patch 2 (v2): Dispatch loadstart event for image loading → [final] Patch 2: Dispatch loadstart event for image loading, r=hsivonen
Attachment #8772663 - Attachment description: Patch 1 (v3): Dispatch loadend event for image loading → [final] Patch 1: Dispatch loadend event for image loading, r=hsivonen
Keywords: checkin-needed
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
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
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
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
https://hg.mozilla.org/mozilla-central/rev/a4313b1f4a54
https://hg.mozilla.org/mozilla-central/rev/ee163e1d915a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1298725
Depend on bug 1292063 per bug 1298725 comment 8.
Depends on: 1292063
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05cf3d96ba3c
Backed out changeset ee163e1d915a for fixing regression of bug 1298725
Target Milestone: mozilla51 → ---
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
checkin-needed for comment 26 patch.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/a1444fdc6ab3
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
As of comment 30.

Sebastian
Flags: needinfo?(btian)
(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)
(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)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.