Closed Bug 1321300 Opened 5 years ago Closed 5 years ago

Image events (onload/onerror) fire on each assigned src

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: homm86, Assigned: edgar)

References

(Depends on 1 open bug)

Details

(Whiteboard: [parity-chrome] [parity-edge][parity-ie11])

Attachments

(3 files, 1 obsolete file)

Attached file steps.js
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/602.2.14 (KHTML, like Gecko) Version/10.0.1 Safari/602.2.14

Steps to reproduce:

Open http://jsbin.com/koxequq/edit?js,output (the code is also attached).
Open developer console 



Actual results:

Starting from Firefox 50, when I'm assigning different src values to the Image object or HTMLImage tag within one js frame (ie when js is not interrupted by any events), Firefox can fire onload and onerror events for several assigned src values, not for only the last one. If you'll look at the demo, you'll see three console messages in Firefox 50:

>>> onerror "http://placehold.it/350x150/?0.2635036113664415"
>>> onload "http://placehold.it/350x150/?0.2635036113664415"
>>> onload "http://placehold.it/350x150/?0.2635036113664415"

The first for the wrong url '//:0', the second for previously cached image (in cachedImage object) nd the third is the actual loaded image (http://placehold.it/350x150/?0.2635036113664415).

As you can see, when all of these events fire, current im.src holds the last value (http://placehold.it/350x150/?0.2635036113664415), not the value for which the event actually happens.


Expected results:

I believe this is unwanted behavior for next reasons:

1. All known browsers (including IE7+, MS Edge, Opera 12, Chrome, Safari) fire delayed event only once for the last associated value. The exceptions are IE7 & IE8 — they can also fire events for each associated src, but they don't delay this and are doing in sync. As a result, in those browsers, im.src holds the current value for which the event is firing.

2. This breaks existing code. (https://uploadcare.com/widget/configure/2.10.1/?publicKey=demopublickey&previewStep=true — try to choose any image)

3. There is no way to get actual src value for which the event is fired from the event handler. im.src always holds the last src, not the actual one.
Component: Untriaged → DOM: Events
Product: Firefox → Core
Whiteboard: [parity-chrome] [parity-edge][parity-ie11]
Version: 50 Branch → Trunk
Component: DOM: Events → DOM
Flags: needinfo?(josh)
Flags: needinfo?(echen)
We still load image synchronously on non-responsive mode, see also bug 1076583.
Flags: needinfo?(josh)
Flags: needinfo?(echen)
Will bug 1076583 fix this? Is there anything we can/should do to fix the breakage reported in comment 0?
Flags: needinfo?(echen)
At least the tests for described cases should be added.
(In reply to Andrew Overholt [:overholt] from comment #2)
> Will bug 1076583 fix this? Is there anything we can/should do to fix the
> breakage reported in comment 0?

Yes, bug 1076583 will fix this.
I would like to leave this open for tracking.
Flags: needinfo?(echen)
(In reply to homm86 from comment #3)
> At least the tests for described cases should be added.

Sure, will add relevant test cases along with the fix.
Depends on: 1076583
Per chat with Edgar, this sufficiency exists in FF for a looong while. I managed to see the duplicates events on earlier versions, such as 49 and 47, so I modified the bug description to reflect the fact more.

hi homm86,
I tried to reproduce the website breakage but wasn't really sure I saw the problem. I could choose an image from my local disk and see the file attached correctly. Any steps I missed? Thank you.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(homm86)
Summary: FF50: Image events (onload/onerror) fire on each assigned src → Image events (onload/onerror) fire on each assigned src
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)

I'v tried the example (http://jsbin.com/koxequq/edit?js,output) in Firefox 49, and you are right, I see two onload events in Firefox 49. But there is no onerror, which fires in Firefox 50. Looks like this is the actual change from 49 to 50.

As of the real site (https://uploadcare.com/widget/configure/2.10.1/?publicKey=demopublickey&previewStep=true): I see loading errors every time I'm trying to choose a local image in Fiferox 50 and no errors in Firefox 49. I'm using fresh Installation without plugins.
(In reply to homm86 from comment #7)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> 
> I'v tried the example (http://jsbin.com/koxequq/edit?js,output) in Firefox
> 49, and you are right, I see two onload events in Firefox 49. But there is
> no onerror, which fires in Firefox 50. Looks like this is the actual change
> from 49 to 50.

Oh, I didn't notice the error event difference. Thank for point that out.

For the error event, I got following regression window from mozregression,

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ebd38bf78b58dd2e2af45a2265d958721a3ae345&tochange=066be6391756b49ee36358a34c8b7ee34ed1f82f

I believe the error event starts getting filed after bug 1264768.
Bug 1264768 itself is correct which follow the spec. However, we load image for each src attribute assignment if we are in non-responsive mode, so we get this error event unexpectedly. Bug 1076583 will fix this (load image from last associated src value only).

Another option is backing out bug 1264768 first, at least we have same behavior as 49 and earlier versions.
And reland it after bug 1076583 is fixed.
(In reply to Edgar Chen [:edgar][:echen] from comment #8)
> Bug 1076583 will fix this (load image
> from last associated src value only).

Bug 1076583 still has ongoing work so should we consider one of the backout options you proposed in comment 8 in the meantime?
Flags: needinfo?(echen)
(In reply to Andrew Overholt [:overholt] from comment #9)
> (In reply to Edgar Chen [:edgar][:echen] from comment #8)
> > Bug 1076583 will fix this (load image
> > from last associated src value only).
> 
> Bug 1076583 still has ongoing work so should we consider one of the backout
> options you proposed in comment 8 in the meantime?

I will back out bug 1264768.
Assignee: nobody → echen
Flags: needinfo?(echen)
Comment on attachment 8824917 [details] [diff] [review]
Back out bug 1264768 and add test, v1

Review of attachment 8824917 [details] [diff] [review]:
-----------------------------------------------------------------

This patch backs out bug 1264768 and add test.
Hi Henri, could help to review this? You could get more details on comment #8 and bug 1264768 comment #16.
I filed bug 1329603 for relanding bug 1264768.
Thank you.
Attachment #8824917 - Flags: review?(hsivonen)
Attachment #8824917 - Flags: review?(hsivonen) → review+
https://hg.mozilla.org/mozilla-central/rev/0aa9d60dab23 also backed out from central
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This bug is now forcing on the unexpected error event mentioned in comment #7. And backing out bug 1264768  fix this, so set status to FIXED.

The reset part (event fired on each assigned src) will be handled in bug 1076583.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: needinfo?(homm86)
Resolution: --- → FIXED
Comment on attachment 8825675 [details] [diff] [review]
Back out bug 1264768 and add test, v2, r=hsivonen

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1264768
[User impact if declined]: Additional error event get fired and might impact some websites, like https://uploadcare.com/widget/configure/2.10.1/?publicKey=demopublickey&previewStep=true.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just simply backed out bug 1264768 and fall back to what we behave in 49 and earlier versions.
[String changes made/needed]: None.
Attachment #8825675 - Flags: approval-mozilla-aurora?
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1264768
[User impact if declined]: Additional error event get fired and might impact some websites, like https://uploadcare.com/widget/configure/2.10.1/?publicKey=demopublickey&previewStep=true.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just simply backed out bug 1264768 and fall back to what we behave in 49 and earlier versions.
[String changes made/needed]: None.
Attachment #8827053 - Flags: approval-mozilla-beta?
Comment on attachment 8825675 [details] [diff] [review]
Back out bug 1264768 and add test, v2, r=hsivonen

Fix an issue related image event firing. Aurora52+.
Attachment #8825675 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8827053 [details] [diff] [review]
[beta] Back out bug 1264768 and add test, v2, r=hsivonen

Fix an issue related image event firing. Beta51+. Should be 51 RC.
Attachment #8827053 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.