Josh Matthews [:jdm] Reporter
	Description • 8 years ago

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 .

	Ben Tian (inactive) Assignee
	Comment 3 • 8 years ago

Rebase on the latest m-c.

	Ben Tian (inactive) Assignee
	Comment 4 • 8 years ago

Rebase on the latest m-c.

	Ben Tian (inactive) 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

	Ben Tian (inactive) 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

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

	Ben Tian (inactive) 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

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

	Henri Sivonen (:hsivonen)
	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.

	Ben Tian (inactive) 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

	Josh Matthews [:jdm] 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 .

	Ben Tian (inactive) Assignee
	Comment 13 • 8 years ago

I see. Thanks for pointing out.

	Henri Sivonen (:hsivonen)
	Comment 14 • 8 years ago

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

	Ben Tian (inactive) Assignee
	Comment 15 • 8 years ago

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

	Ben Tian (inactive) 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

	Pulsebot
	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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 8 years ago

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

	Ben Tian (inactive) 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

	Ben Tian (inactive) 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.

	Pulsebot
	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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 8 years ago
bugherder

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

	Ben Tian (inactive) Assignee
	Comment 23 • 8 years ago

Depend on bug 1292063 per bug 1298725 comment 8.

	Pulsebot
	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

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 8 years ago
backout bugherder

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

	Ben Tian (inactive) 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

	Ben Tian (inactive) Assignee
	Comment 27 • 8 years ago

checkin-needed for comment 26 patch.

	Pulsebot
	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

	Carsten Book [:Tomcat]
	Comment 29 • 8 years ago
bugherder

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

	Chris Mills [:cmills]
	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!

	Sebastian Zartner [:sebo]
	Comment 31 • 7 years ago

As of comment 30.

Sebastian

	Ben Tian (inactive) 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

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