Closed Bug 599975 Opened 14 years ago Closed 8 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: bugzilla, Assigned: edgar)

References

(
URL
)

Details

(4 keywords, Whiteboard: [webcompat])

Attachments

(2 files, 1 obsolete file)

Patch for checkin
3.05 KB, patch
Details | Diff | Splinter Review
Bug 599975 - Fire error event for images with empty string src value;
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
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>
OS: Windows Server 2003 → All
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
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.
URL: data:text/html,<img src="" onerror="a...
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
URL: data:text/html,<img src="" onerror="a... → http://software.hixie.ch/utilities/js...
Component: General → DOM: Core & HTML
Keywords: html5
QA Contact: general → general
@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.
Attached patch Patch v1 (obsolete) — — Splinter Review 
Ah, the fourth LoadImage. Thanks.
Assignee: nobody → Ms2ger
Status: NEW → ASSIGNED
Attachment #494665 - Flags: review?(Olli.Pettay)
Attachment #494665 - Flags: review?(Olli.Pettay) → review+
Attachment #494665 - Flags: approval2.0?
Attachment #494665 - Flags: approval2.0? → approval2.0+

        Attachment #494665 -
        Attachment is obsolete: true

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/82f92ddcec7f
Status: ASSIGNED → RESOLVED
Closed: 14 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.
See Also:  → https://github.com/webcompat/web-bugs/issues/2760#issuecomment-245163796
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.
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 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)

    
    

    
  
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

    
    

    
      
  

    
    

    
  
>  We add listener __after__ img.src setter

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

    
    

    
  
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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/743d1b4c52db
Status: NEW → RESOLVED
Closed: 14 years ago → 8 years ago

          status-firefox51:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Whiteboard: not-ready, [webcompat] → [webcompat]

    
  
Depends on: 1308069

    
  
Depends on: 1308126

    
  
No longer depends on: 1308069
Regressions: 1308069

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: