1264769 - Image elements should dispatch loadstart and loadend events

Status: RESOLVED FIXED

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

Description

[Josh Matthews [:jdm]]

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 .

Comment 1

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): Dispatch loadend event for image loading

Comment 2

[Ben Tian (inactive)]

Attachment: Patch 2 (v1): Dispatch loadstart event for image loading

Comment 3

[Ben Tian (inactive)]

Attachment: Patch 1 (v2): Dispatch loadend event for image loading

Rebase on the latest m-c.

Comment 4

[Ben Tian (inactive)]

Attachment: [final] Patch 2: Dispatch loadstart event for image loading, r=hsivonen

Rebase on the latest m-c.

Comment 5

[Ben Tian (inactive)]

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

Comment 6

[Ben Tian (inactive)]

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

Comment 7

[Henri Sivonen (: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?

Comment 8

[Ben Tian (inactive)]

(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

Comment 9

[Henri Sivonen (: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!

Comment 10

[Henri Sivonen (:hsivonen)]

I filed a spec bug: https://github.com/whatwg/html/issues/1551 . Let's see what the outcome of the spec bug is.

Comment 11

[Ben Tian (inactive)]

(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

Comment 12

[Josh Matthews [:jdm]]

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 13

[Ben Tian (inactive)]

I see. Thanks for pointing out.

Comment 14

[Henri Sivonen (:hsivonen)]

The spec was changed to include onloadend, so it's now OK to add it to Gecko, too.

Comment 15

[Ben Tian (inactive)]

Attachment: [final] Patch 1: Dispatch loadend event for image loading, r=hsivonen

Change:
- do not fire loadend event for multipart/x-mixed-replace image streams

Comment 16

[Ben Tian (inactive)]

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if 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

Comment 19

[Ben Tian (inactive)]

Attachment: [final] Patch 1: Dispatch loadend event for image loading, r=hsivonen

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

Comment 20

[Ben Tian (inactive)]

Re-try of comment 19 patch on all tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=632ecda014e3

Set checkin-needed for updated patches.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a4313b1f4a54
https://hg.mozilla.org/mozilla-central/rev/ee163e1d915a

Comment 23

[Ben Tian (inactive)]

Depend on bug 1292063 per bug 1298725 comment 8.

Comment 24

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05cf3d96ba3c
Backed out changeset ee163e1d915a for fixing regression of bug 1298725

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/05cf3d96ba3c

Comment 26

[Ben Tian (inactive)]

Attachment: [final] Patch 2: Dispatch loadstart event for image loading, r=hsivonen

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

Comment 27

[Ben Tian (inactive)]

checkin-needed for comment 26 patch.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1444fdc6ab3
Patch 2: Dispatch loadstart event for image loading, r=hsivonen

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a1444fdc6ab3

Comment 30

[Chris Mills [:cmills]]

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!

Comment 31

[Sebastian Zartner [:sebo]]

As of comment 30.

Sebastian

Comment 32

[Ben Tian (inactive)]

(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

Comment 33

[Chris Mills [: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.