<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.
(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 ?
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 .
Created attachment 220802 [details] Patch
(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 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
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 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
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
Created attachment 220813 [details] Patch
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
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 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
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
Created attachment 220962 [details] Patch
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 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.
Created attachment 221436 [details] Patch
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.
*** Bug 127680 has been marked as a duplicate of this bug. ***
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.