Bug 26285 – Removing an image's src attribute, or setting it to null, via scripting, doesn't change the image

• Component: DOM
• Status: RESOLVED CONFIGURATION CHANGED

<rdar://problem/6951846>
Removing an image's src attribute, or setting it to null, via scripting, doesn't update the UI.

Ex:
image.src = "";
image.removeAttribute('src');

Safari will still show the previous image.
Firefox will (as expected) empty the image.

Attached is a simple reduction.

Created attachment 31117 [details]
Test case

Seems like WebKit is in compliance with HTML5 here.

[Jaehun Lim]

(In reply to comment #2)
> Seems like WebKit is in compliance with HTML5 here.

mitz, is the current WebKit behavior(show the previous image when src is removed) right ?


I found below lines in HTML5 standard document(in progress).

If the src attribute is not set and either the alt attribute is set to the empty string or the alt attribute is not set at all
-> The element represents nothing.

(http://www.w3.org/TR/2012/WD-html5-20121025/the-img-element.html#the-img-element)

I think "represents nothing" means "paints nothing". In my opinion, clearing the previous image is right. It's more intuitive, too. How do you think about this ?

[Jaehun Lim]

Any comments ?

(In reply to comment #4)
> Any comments ?
I think this is a bug and chromium has fixed it. You can find more discussions in the URL http://code.google.com/p/chromium/issues/detail?id=123017 and how to fix it in the URL http://src.chromium.org/viewvc/blink?view=rev&rev=155294 .

[Jaehun Lim]

Created attachment 220802 [details]
Patch

[Jaehun Lim]

(In reply to comment #5)
> (In reply to comment #4)
> > Any comments ?
> I think this is a bug and chromium has fixed it. You can find more discussions in the URL http://code.google.com/p/chromium/issues/detail?id=123017 and how to fix it in the URL http://src.chromium.org/viewvc/blink?view=rev&rev=155294 .

Thanks for your comment. I uploaded the patch.

[Build Bot]

Comment on attachment 220802 [details]
Patch

Attachment 220802 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5684834550677504

New failing tests:
canvas/philip/tests/2d.drawImage.image.incomplete.omitted.html

[Build Bot]

Created attachment 220808 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 220802 [details]
Patch

Attachment 220802 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5799468838420480

New failing tests:
canvas/philip/tests/2d.drawImage.image.incomplete.omitted.html

[Build Bot]

Created attachment 220812 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Jaehun Lim]

Created attachment 220813 [details]
Patch

[Build Bot]

Comment on attachment 220813 [details]
Patch

Attachment 220813 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6613354672029696

New failing tests:
canvas/philip/tests/2d.drawImage.image.incomplete.omitted.html

[Build Bot]

Created attachment 220816 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 220813 [details]
Patch

Attachment 220813 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4903867762343936

New failing tests:
canvas/philip/tests/2d.drawImage.image.incomplete.omitted.html

[Build Bot]

Created attachment 220821 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Jaehun Lim]

Created attachment 220962 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 220962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220962&action=review

> Source/WebCore/rendering/RenderImage.cpp:283
> -    if (m_imageResource->errorOccurred())
> +    if (m_imageResource->errorOccurred() || !m_imageResource->hasImage())

This patch could benefit a lot from per-function comments in ChangeLog. For example, I don't understand why this change was needed. What are the circumstances when imageResource has an image, even though an error occurred?

> LayoutTests/canvas/philip/tests/2d.drawImage.image.incomplete.omitted-expected.txt:2
> +

This change is not mentioned in ChangeLog. Why is it expected?

[Jaehun Lim]

Comment on attachment 220962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220962&action=review

Thanks comments.
I update ChangeLogs and LayoutTests.

>> Source/WebCore/rendering/RenderImage.cpp:283
>> +    if (m_imageResource->errorOccurred() || !m_imageResource->hasImage())
> 
> This patch could benefit a lot from per-function comments in ChangeLog. For example, I don't understand why this change was needed. What are the circumstances when imageResource has an image, even though an error occurred?

While removing 'src' attribute, the CachedImage was cleared. So m_imageResource->errorOccurred() returns false. In this case, both errorOccurred() and hasImage() return false.

See:
bool RenderImageResource::errorOccurred() const                                                                                                                                                                                   
{
    return m_cachedImage && m_cachedImage->errorOccurred();
}

But after removing 'src', the intrinsic size should be changed to 0x0 or 'alt' text size.

>> LayoutTests/canvas/philip/tests/2d.drawImage.image.incomplete.omitted-expected.txt:2
>> +
> 
> This change is not mentioned in ChangeLog. Why is it expected?

Update ChangeLog.

[Jaehun Lim]

Created attachment 221436 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 221436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221436&action=review

Thank you for looking into this, this is a good bug to fix.

I think that this change makes the code substantially more fragile and confusing though. Some comments inline.

More importantly, this needs a lot more test coverage:

- what happens when initial src points to a 404 resource, and we later remove the attribute?
- what happens when removing src from an element that's currently loading (this test only runs after unload)?
- what are the DOM events being dispatched in all of these cases?

I think that adding such tests may prompt some code changes too.

> Source/WebCore/loader/ImageLoader.cpp:259
> +        } else
> +            updateRenderer();

The added code reads really strangely. It's essentially:

if (newImage == oldImage)
    updateRenderer();

How is this a good thing? What are the other cases when this function is called with newImage == oldImage, and can updating the renderer have unintended consequences for those?

And why do we get the same CachedImage in this case when removing the src? Conceptually, there should be no CachedImage, because we are not loading anything.

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Any comments ?
> > I think this is a bug and chromium has fixed it. You can find more discussions in the URL http://code.google.com/p/chromium/issues/detail?id=123017 and how to fix it in the URL http://src.chromium.org/viewvc/blink?view=rev&rev=155294 .
> 
> Thanks for your comment. I uploaded the patch.

You are welcome.

[Alexey Proskuryakov]

*** Bug 127680 has been marked as a duplicate of this bug. ***

[Ahmad Saleem]

Created attachment 459936 [details]
Safari 15.5 matches other browsers

I am unable to reproduce this issue in Safari 15.5 on macOS 12.4 as shown in the attached picture and it is inline with other browsers. Thanks!

Thanks for testing. I am also seeing this behaving as expected now.