Bug 26285 - Removing an image's src attribute, or setting it to null, via scripting, doesn't change the image
Summary: Removing an image's src attribute, or setting it to null, via scripting, does...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar
: 127680 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-06-09 17:36 PDT by mitz
Modified: 2014-01-27 10:12 PST (History)
12 users (show)

See Also:


Attachments
Test case (829 bytes, text/html)
2009-06-09 17:37 PDT, mitz
no flags Details
Patch (6.66 KB, patch)
2014-01-09 22:27 PST, Jaehun Lim
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (454.05 KB, application/zip)
2014-01-09 23:19 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (491.54 KB, application/zip)
2014-01-09 23:47 PST, Build Bot
no flags Details
Patch (7.30 KB, patch)
2014-01-09 23:54 PST, Jaehun Lim
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (536.11 KB, application/zip)
2014-01-10 00:30 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (451.90 KB, application/zip)
2014-01-10 01:47 PST, Build Bot
no flags Details
Patch (7.81 KB, patch)
2014-01-11 23:49 PST, Jaehun Lim
no flags Details | Formatted Diff | Diff
Patch (8.80 KB, patch)
2014-01-16 20:06 PST, Jaehun Lim
ap: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2009-06-09 17:36:32 PDT
<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.
Comment 1 mitz 2009-06-09 17:37:44 PDT
Created attachment 31117 [details]
Test case
Comment 2 mitz 2010-04-16 16:07:04 PDT
Seems like WebKit is in compliance with HTML5 here.
Comment 3 Jaehun Lim 2012-11-01 01:40:33 PDT
(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 ?
Comment 4 Jaehun Lim 2012-11-07 18:19:56 PST
Any comments ?
Comment 5 kingysu 2014-01-08 01:19:01 PST
(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 .
Comment 6 Jaehun Lim 2014-01-09 22:27:09 PST
Created attachment 220802 [details]
Patch
Comment 7 Jaehun Lim 2014-01-09 22:27:56 PST
(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.
Comment 8 Build Bot 2014-01-09 23:19:16 PST
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
Comment 9 Build Bot 2014-01-09 23:19:19 PST
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
Comment 10 Build Bot 2014-01-09 23:47:23 PST
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
Comment 11 Build Bot 2014-01-09 23:47:26 PST
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
Comment 12 Jaehun Lim 2014-01-09 23:54:03 PST
Created attachment 220813 [details]
Patch
Comment 13 Build Bot 2014-01-10 00:30:34 PST
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
Comment 14 Build Bot 2014-01-10 00:30:36 PST
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
Comment 15 Build Bot 2014-01-10 01:47:02 PST
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
Comment 16 Build Bot 2014-01-10 01:47:05 PST
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
Comment 17 Jaehun Lim 2014-01-11 23:49:23 PST
Created attachment 220962 [details]
Patch
Comment 18 Alexey Proskuryakov 2014-01-13 21:58:53 PST
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?
Comment 19 Jaehun Lim 2014-01-16 20:03:08 PST
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.
Comment 20 Jaehun Lim 2014-01-16 20:06:14 PST
Created attachment 221436 [details]
Patch
Comment 21 Alexey Proskuryakov 2014-01-17 17:14:11 PST
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.
Comment 22 kingysu 2014-01-20 20:22:48 PST
(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.
Comment 23 Alexey Proskuryakov 2014-01-27 10:12:40 PST
*** Bug 127680 has been marked as a duplicate of this bug. ***