Bug 42840 – image element with src attribute can't be replaced by content: url() style

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Leo Yang]

Image element with src attribute and content: url() style can't be rendered correctly.

For example:
<image src="http://domain-which-does-not-exist.com/not-exist.jpg" style="content: url(url-for-image-exist.jpg)" />

webkit renders it as blank page. Even if src points to an exist image, a blank page renders also. If you use debug build, when reload the page, there is an ASSERT failure in CachedResource::removeClient().

[Leo Yang]

Created attachment 62327 [details]
Patch

[Ojan Vafai]

Comment on attachment 62327 [details]
Patch

I tried <image src="http://google.com/images/logo.gif" style="content: url(http://www.google.com/intl/en_ALL/images/logos/images_logo_lg.gif)" />

This renders the src image in Firefox 3.6 and the generated image in Opera 10.6 and WebKit nightly. So, this patch changes WebKit to match the Firefox behavior, right? Is there a spec that covers this behavior that one of us is violating?

[Leo Yang]

(In reply to comment #2)
> (From update of attachment 62327 [details])
> I tried <image src="http://google.com/images/logo.gif" style="content: url(http://www.google.com/intl/en_ALL/images/logos/images_logo_lg.gif)" />
> 
> This renders the src image in Firefox 3.6 and the generated image in Opera 10.6 and WebKit nightly. So, this patch changes WebKit to match the Firefox behavior, right? Is there a spec that covers this behavior that one of us is violating?

I am using latest code (on the top of commit 6255afb56f275ef6bb3ebe6d5484ad65a01e2529), webkit renders nothing. This patch doesn't change behavior to match Firfox, instead, just renders generated image.

[Ojan Vafai]

> I am using latest code (on the top of commit 6255afb56f275ef6bb3ebe6d5484ad65a01e2529), webkit renders nothing. This patch doesn't change behavior to match Firfox, instead, just renders generated image.

Oh, interesting, nothing renders in Safari 5, but the generated image renders in WebKit nightly on Mac at r63854. Looks like this is fixed already or did this regress since r63854?

[Adam Barth]

Comment on attachment 62327 [details]
Patch

Why is this a manual test?  Can't we see this with a render tree dump or a pixel test?

WebCore/loader/ImageLoader.cpp:5
 +   * Copyright (C) Research In Motion Limited 2010. All rights reserved.
Please don't add your copyright when changing < 10 lines.

WebCore/rendering/RenderImage.h:7
 +   * Copyright (C) Research In Motion Limited 2010. All rights reserved.
ditto

[Leo Yang]

Created attachment 62762 [details]
Revised patch

[Eric Seidel (no email)]

Comment on attachment 62762 [details]
Revised patch

What if the generated context is invalid?

[Leo Yang]

(In reply to comment #7)
> (From update of attachment 62762 [details])
> What if the generated context is invalid?

I think it should behave as if this is an invalid image. Google Chrome 5.0 on Windows and Safari 5.0.1 on Windows behave as there is a broken image. Just as above discussion, this feature is broken in recently webkit trunk.

[Darin Adler]

Comment on attachment 62762 [details]
Revised patch

This is a good catch. That code in ImageLoader is indeed wrong.

But this patch will no longer apply, due to changes to ImageLoader.cpp that happened since it was posted.

> +        // If RenderImage is generated content we don't update the renderer either.

The above is not a helpful comment. It says what the code does, but not why.

As I understand the issue here, we want to change the image if this is a renderer we created, but not if it's a renderer that was created by other code.

The fix should probably go in ImageLoader::renderImageResource, which can return 0 if the renderer is not for the loaded image, but rather some other renderer.

Special casing the generated content version of RenderImage seems a little bit roundabout. Unfortunately, there is no longer a RenderImageGeneratedContent class to add a virtual function to, so we need a new approach to figuring out if this is an image renderer that the image loader should be overriding, or one it should not be. Somehow we have to distinguish the renderer created in RenderObject::createObject for generated content.

[Leo Yang]

Created attachment 66680 [details]
Rebased patch

[Leo Yang]

Created attachment 66682 [details]
Removed extra space from change log

[Adam Barth]

Comment on attachment 66682 [details]
Removed extra space from change log

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

I wanted to R+ this patch, but I didn't understand it.

> WebCore/rendering/RenderImageResource.h:71
> -    RenderImageResource();
> +    RenderImageResource(bool isGenerated);

I'd use an enum instead of a bool so we can understand the call-sites better.

[Leo Yang]

Created attachment 77401 [details]
Change boolean to enum

It makes sense to change boolean to enumeration for RenderImageResource.

[Adam Barth]

Comment on attachment 77401 [details]
Change boolean to enum

This patch looks fine to me, but, as a rule, I don't review patches in "WebCore/rendering" because I don't understand rendering.

[Darin Adler]

Comment on attachment 77401 [details]
Change boolean to enum

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

I don't think it’s right to make a special RenderImageResource.

The real issue here is that RenderObject::createObject creates a RenderImage, but classes like HTMLImageElement assume that if the renderer is a RenderImage it’s the one they created. I think we should make a different renderer class, or add a flag to RenderImage, not RenderImageResource. I think the different class is probably the best way to handle it.

I looked and found functions that make the incorrect assumption that an image is one created by the element and these are the ones I found:

    ImageLoader::renderImageResource
    HTMLImageElement::parseMappedAttribute
    HTMLImageElement::attach
    HitTestResult::absoluteImageURL
    absoluteURL functions generated by CodeGeneratorObjC.pm

Those should probably use a function that returns false for the images that were created based on style, rather than the isImage function, which returns true for those.

> WebCore/rendering/RenderImageResource.h:47
> +    static PassOwnPtr<RenderImageResource> create(Type type = Normal)

In the context of RenderImageResource, I don’t think “generated” is a clear enough term to be used. In this patch it means generated content, in other words, the CSS content attribute.

But we also have a class named GeneratedImage in WebCore/platform/graphics, and this refers to images that are created by an algorithm, such as gradient, rather than with image data.

[Darin Adler]

Thanks for tackling this. I’m sorry I didn’t spot this bigger issue earlier.

[Daniel Bates]

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

[Adam Roben (:aroben)]

As noted in bug 67721, sometimes things work just fine in Release builds.

[Radar WebKit Bug Importer]

<rdar://problem/10175703>

[Leo Yang]

Created attachment 108642 [details]
Patch v5

[Darin Adler]

Comment on attachment 108642 [details]
Patch v5

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

Patch is OK, but not great. I’d like to look at cleaner ways to do this in the future, but it seems fine to land this now.

> Source/WebCore/loader/ImageLoader.cpp:257
> -    if (renderer->isImage())
> +    if (renderer->isImage() && !static_cast<RenderImage*>(renderer)->isGeneratedContent())

This is a non-obvious check, and it needs a comment to make clear why this is correct.

We might even want to later rename renderImageResource longer term to make it clearer what the function returns.

It seems that given we already use a different class for the generated content image, the bit could be on RenderImageResource instead of on RenderImage. We could even use a virtual function instead of a bit.

[Leo Yang]

Created attachment 117555 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 117555 [details]
Patch for landing

Clearing flags on attachment: 117555

Committed r101753: <http://trac.webkit.org/changeset/101753>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.