Fire "error" event when img src is null or empty

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
10 months ago

People

(Reporter: bugzilla, Assigned: edgar)

Tracking

(Depends on: 1 bug, 4 keywords)

Trunk
mozilla51
dev-doc-complete, html5, site-compat, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [webcompat], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; de; rv:1.9.0.19) Gecko/2010031422 Firefox/3.0.19 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; de; rv:1.9.1.13) Gecko Firefox/3.5.13

Build an Imageloader with javascript, i use an onerror handler for broken images.
In few situations the "src" for images is an empty value. In this case, they will not fireing an error. (same in Opera 9.64)

I'm not shure, is that a bug or a feature? 

Reproducible: Always

Steps to Reproduce:
1.use the script (Additional Information)
2.load them
3.take a look
Actual Results:  
in FF 3.0 you will see 5 errors include empty and null source given. (same IE7/8 Safari3)
in FF 3.5 you will see 3 errors not emtpy and null source given. (same Opera9.64)


<!DOCTYPE html>
<html>
<head>
<script type="text/javascript">
window.onload=function(){
	var report = function(val){
        var p = document.createElement('p');
        p.innerHTML=val;
        document.getElementsByTagName('body')[0].appendChild(p);
    };

    var images=new Array('http://www.mozilla.org/images/template/screen/logo_footer.png','','breakimage','',null,false);
    
    
    for(i=0;i<images.length;i++){
        var image= new Image();
        if(typeof(image)!='object') image=document.createElement('img');
        image.onload=function(){
			report('load: ' + this.alt);
		};
        image.onerror=function(){
			report('error: ' + this.alt);
		};
        image.src=images[i];
		image.alt=images[i]
    }
}
</script>
</head>
<body>
</body>
</html>

Updated

7 years ago
OS: Windows Server 2003 → All
(Reporter)

Comment 1

7 years ago
OK, lets try again... simplified case, use something like this:
<img src="" onerror="alert('not loaded becouse nothing to load');" />
in FF3.0.x will see error in afterwards versions you will see no error=!?

Is this held back wanted or not?!
Hardware: x86_64 → All

Comment 2

7 years ago
The simple testcase doesn't fire onerror in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b8pre) Gecko/20101128 Firefox/4.0b8pre, but does in Chrome.
Component: General → General
Keywords: testcase
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Hmm.  Looks like the current HTML5 draft calls for an error event here.  That looks like a spec change...

Ms2ger, want to take this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
I'll have a look. Do you happen to know off the top of your head whether this check should be in nsHTMLImageElement::SetAttr or in one of the three LoadImages it calls?

For reference:

http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-March/025507.html
http://html5.org/tools/web-apps-tracker?from=4840&to=4841

Updated

7 years ago
Component: General → DOM: Core & HTML
Keywords: html5
QA Contact: general → general
(Reporter)

Comment 5

7 years ago
@all I think this is not an HTML 5 specific „problem“.
Per se  since ff 3.1 but now seems to me it is not a Bug, It is an intended behaviour.

@Ms2ger I don't realy understand your question or hint,
you mean somthing like this: if(!imageURL) imageURL=false; ?
Otherwise  for my understanding,  an exist image-Object with an empty src is an error image. 
Remember the X/broken replace image in oldschool times i mean this was an indication that result from a fire event?

however 

ONE I sees on the basis of the references by Ms2ger my asks was already placed and apparently also in the HTML5/DOM draft/Revision „the problem“ to be considered wants. 


By the way i saw this:
http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#error-codes only "valid non-empty URL potentially surrounded by spaces"  for video/audio tag. 
I do not hope that will similar confusions appear ;)
Ms2ger, we have the check already.  See http://hg.mozilla.org/mozilla-central/file/60d9203fdc35/content/base/src/nsImageLoadingContent.cpp#l593

We just need to add some error event firing there.

Reporter, this was in fact an intended behavior when the code was written per the HTML5 spec, but then the spec changed on us.
Created attachment 494665 [details] [diff] [review]
Patch v1

Ah, the fourth LoadImage. Thanks.
Assignee: nobody → Ms2ger
Status: NEW → ASSIGNED
Attachment #494665 - Flags: review?(Olli.Pettay)

Updated

7 years ago
Attachment #494665 - Flags: review?(Olli.Pettay) → review+

Updated

7 years ago
Attachment #494665 - Flags: approval2.0?

Updated

7 years ago
Attachment #494665 - Flags: approval2.0? → approval2.0+
Created attachment 509485 [details] [diff] [review]
Patch for checkin
Attachment #494665 - Attachment is obsolete: true

Updated

7 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/82f92ddcec7f
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
I backed this patch as part of this pushlog <http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7e12e3e16e6c> because of the oranges it caused <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296852850.1296854923.2345.gz&fulltext=1>.

I'm not sure which one of the bugs was at fault, so I just backed them all out.  The assignee needs to investigate and make sure that his patch has not been the culprit, and then re-add checkin-needed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This patch was in fact the cause of the orange.
(All of the failures were in content/media/test/.)
No idea how video ends up calling into this, and not planning to put more time into this atm. I think we'll probably need to rewrite most of this to match the conceptual model in the spec.
Assignee: Ms2ger → nobody
Status: REOPENED → NEW
Flags: in-testsuite+
Target Milestone: mozilla2.0b12 → ---
> No idea how video ends up calling into this

Poster images, presumably?

I don't see how any rewriting is needed here; what we have is quite compatible with the conceptual model in the spec last I checked.
Matthew, does video create poster images with empty src?
It looks like it does, yes.  nsVideoFrame::UpdatePosterSource sets the image source to whatever is returned by nsHTMLVideoElement::GetPoster.  It'd be simple to add a check for zero length sources before setting them, if that's necessary.
Either that, or you need to do something sensible with the resulting error events when empty src is set (e.g. suppress them, or change tests to not fail on them, or whatever).
Comment on attachment 494665 [details] [diff] [review]
Patch v1

(bookkeeping)
Attachment #494665 - Flags: approval2.0+ → approval2.0-
Whiteboard: not-ready
This still seems like the right thing to do according to https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data. 

And it's the reason this UK hamburger restaurant site is broken in Firefox (desktop, not mobile, because they load different images for small viewports): http://www.burgerandlobster.com/home/

See https://github.com/webcompat/web-bugs/issues/2760#issuecomment-245163796 for more on that.
Summary: no onerror fireing by giving empty or null src value → Fire "error" event when img src is null or empty
Whiteboard: not-ready → not-ready, [webcompat]
This bug probably needs an owner....
Flags: needinfo?(overholt)
I pushed the equivalent of Ms2ger's patch to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b911c3d953a&selectedJob=27110781. No failures in media and the oranges seem unrelated. 

(I guess those content/media/test files don't exist anymore... maybe we can just reland the patch?)
I am willing to take this bug.
Assignee: nobody → echen
The patch needs a revision, we should only fire error event (no loadstart and loadend). See step 9.2. of https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data.
Comment hidden (mozreview-request)
Try result looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96970661c7aae8b072cf07b814c4909163f31ad2&filter-tier=1&group_state=expanded&selectedJob=27160821
It looks like the failures in media don't exist anymore.
Flags: needinfo?(overholt)
Edgar, could you please determine why the media failures are no longer present?  Do we no longer hit this codepath in those tests?  Do we hit it but it does not trigger test failure for some reason?  Something else?

I would really like to understand what the actual change was wrt media tests, because that determines what the web-visible behavior will be.
Flags: needinfo?(echen)
> because of the oranges it caused <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296852850.1296854923.2345.gz&fulltext=1>.

Too bad we don't ever bother to preserve the content at the URIs our tools generate.  :(  It's hard to tell now which tests were failing, given that broken link.  That makes it a bit harder to evaluate what the behavior on "the tests that used to fail" is now.  Pulling those old revisions and running the tests locally might work, though.

> I guess those content/media/test files don't exist anymore

They're in dom/media/test.
(In reply to Boris Zbarsky [:bz] from comment #26)
> Edgar, could you please determine why the media failures are no longer
> present?  Do we no longer hit this codepath in those tests?  Do we hit it
> but it does not trigger test failure for some reason?  Something else?
> 

nsVideoFrame set src on poster image only if poster attribute is not empty after bug 1283415.
So setting poster to "" won't trigger error event.
I am going to remove the patch of bug 1283415 to see which media tests will get failed.

> I would really like to understand what the actual change was wrt media
> tests, because that determines what the web-visible behavior will be.
(In reply to Edgar Chen [:edgar][:echen] from comment #28)
> nsVideoFrame set src on poster image only if poster attribute is not empty
> after bug 1283415.
> So setting poster to "" won't trigger error event.

And bug 938772 avoid setting src on poster image if video don't have a 'poster' attribute.
OK.  That answers my question about why the test failures went away, thanks!

Comment 31

11 months ago
mozreview-review
Comment on attachment 8789420 [details]
Bug 599975 - Fire error event for images with empty string src value;

https://reviewboard.mozilla.org/r/77542/#review76102

::: dom/base/nsImageLoadingContent.cpp:761
(Diff revision 1)
>      // No reason to bother, I think...
>      return NS_OK;
>    }
>  
> +  if (aNewURI.IsEmpty()) {
> +    // Cancel image requests and then fire only error event per spec.

OK.  I guess this works, though I'm 99% sure we don't do the right things in the "no src, no responsive selector stuff" case either.  But that should be a separate bug.

::: testing/web-platform/tests/html/semantics/embedded-content/the-img-element/invalid-src.html:27
(Diff revision 1)
> +async_test(function(t) {
> +    var img = document.getElementById("emptysrc");
> +    img.src = "";
> +
> +    // Setting src to empty string triggers only error event.
> +    // The errors should be queued in the event loop, so they should only trigger

This test doesn't check that the error event actually fires async.  It should (as should the previous test).

This test also doesn't check that loadend is not fired after the error event in the src="" case.  One way to set up another image thing that would queue an event from inside the error handler for this one, and make sure there was no loadend before that next thing fires.  Please fix that.

Oh, and we're not following the spec for the case when we _do_ fire both events: we're firing them off separate runnables, but the spec says to fire them off a single task.  Followup bug for that, please.

r=me with the above test fixes.
Attachment #8789420 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #31)
> Comment on attachment 8789420 [details]
> Bug 599975 - Fire error event for images with empty string src value;
> 
> Oh, and we're not following the spec for the case when we _do_ fire both
> events: we're firing them off separate runnables, but the spec says to fire
> them off a single task.  Followup bug for that, please.

Filed bug 1301625 for that.
Flags: needinfo?(echen)
(In reply to Boris Zbarsky [:bz] from comment #31)
> Comment on attachment 8789420 [details]
> Bug 599975 - Fire error event for images with empty string src value;
> 
> https://reviewboard.mozilla.org/r/77542/#review76102
> 
> :::
> testing/web-platform/tests/html/semantics/embedded-content/the-img-element/
> invalid-src.html:27
> (Diff revision 1)
> > +async_test(function(t) {
> > +    var img = document.getElementById("emptysrc");
> > +    img.src = "";
> > +
> > +    // Setting src to empty string triggers only error event.
> > +    // The errors should be queued in the event loop, so they should only trigger
> 
> This test doesn't check that the error event actually fires async.  It
> should (as should the previous test).

I thought this test does check error event is async. We add listener __after__ img.src setter, so if error event fires sync, we won't receive the event and test will timeout due to step_func_done() is not called.
Or am I missing something? Thanks.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 34

11 months ago
mozreview-review-reply
Comment on attachment 8789420 [details]
Bug 599975 - Fire error event for images with empty string src value;

https://reviewboard.mozilla.org/r/77542/#review76102

> This test doesn't check that the error event actually fires async.  It should (as should the previous test).
> 
> This test also doesn't check that loadend is not fired after the error event in the src="" case.  One way to set up another image thing that would queue an event from inside the error handler for this one, and make sure there was no loadend before that next thing fires.  Please fix that.
> 
> Oh, and we're not following the spec for the case when we _do_ fire both events: we're firing them off separate runnables, but the spec says to fire them off a single task.  Followup bug for that, please.

Will add test to make sure loadend is not fired in src="" case. Thanks
Comment hidden (mozreview-request)
>  We add listener __after__ img.src setter

Ah, good point. Excellent.  ;)
Flags: needinfo?(bzbarsky)

Comment 37

11 months ago
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/743d1b4c52db
Fire error event for images with empty string src value; r=bz

Comment 38

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/743d1b4c52db
Status: NEW → RESOLVED
Last Resolved: 7 years ago11 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

11 months ago
Keywords: dev-doc-needed, site-compat

Comment 39

11 months ago
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/img-with-empty-src-will-fire-error-event/
Whiteboard: not-ready, [webcompat] → [webcompat]

Updated

11 months ago
Depends on: 1308069

Updated

11 months ago
Depends on: 1308126
Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img
https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement
https://developer.mozilla.org/en-US/Firefox/Releases/51

Please let me know if there are any problems with these revisions!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.