Bug 42840 - image element with src attribute can't be replaced by content: url() style
: image element with src attribute can't be replaced by content: url() style
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To: Nobody
data:text/html,%3Cimg%20src=%22http:/...
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-22 11:59 PDT by Leo Yang
Modified: 2011-12-02 00:05 PST (History)
13 users (show)

See Also:


Attachments
Patch (5.69 KB, patch)
2010-07-22 12:34 PDT, Leo Yang
abarth: review-
Details | Formatted Diff | Diff
Revised patch (6.76 KB, patch)
2010-07-27 15:54 PDT, Leo Yang
darin: review-
darin: commit‑queue-
Details | Formatted Diff | Diff
Rebased patch (10.79 KB, patch)
2010-09-06 20:31 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Removed extra space from change log (10.78 KB, patch)
2010-09-06 21:03 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Change boolean to enum (10.93 KB, patch)
2010-12-23 21:52 PST, Leo Yang
darin: review-
darin: commit‑queue-
Details | Formatted Diff | Diff
Patch v5 (9.09 KB, patch)
2011-09-26 01:11 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Patch for landing (7.50 KB, patch)
2011-12-01 20:37 PST, Leo Yang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Yang 2010-07-22 11:59:01 PDT
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().
Comment 1 Leo Yang 2010-07-22 12:34:01 PDT
Created attachment 62327 [details]
Patch
Comment 2 Ojan Vafai 2010-07-22 13:02:57 PDT
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?
Comment 3 Leo Yang 2010-07-22 13:26:56 PDT
(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.
Comment 4 Ojan Vafai 2010-07-22 13:42:55 PDT
> 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?
Comment 5 Adam Barth 2010-07-27 08:38:46 PDT
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
Comment 6 Leo Yang 2010-07-27 15:54:41 PDT
Created attachment 62762 [details]
Revised patch
Comment 7 Eric Seidel 2010-08-06 13:53:48 PDT
Comment on attachment 62762 [details]
Revised patch

What if the generated context is invalid?
Comment 8 Leo Yang 2010-08-09 07:16:06 PDT
(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.
Comment 9 Darin Adler 2010-08-29 13:20:16 PDT
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.
Comment 10 Leo Yang 2010-09-06 20:31:07 PDT
Created attachment 66680 [details]
Rebased patch
Comment 11 Leo Yang 2010-09-06 21:03:05 PDT
Created attachment 66682 [details]
Removed extra space from change log
Comment 12 Adam Barth 2010-12-23 00:30:20 PST
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.
Comment 13 Leo Yang 2010-12-23 21:52:21 PST
Created attachment 77401 [details]
Change boolean to enum

It makes sense to change boolean to enumeration for RenderImageResource.
Comment 14 Adam Barth 2011-01-04 10:49:24 PST
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.
Comment 15 Darin Adler 2011-01-04 13:17:19 PST
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.
Comment 16 Darin Adler 2011-01-04 13:17:52 PST
Thanks for tackling this. I’m sorry I didn’t spot this bigger issue earlier.
Comment 17 Daniel Bates 2011-09-22 18:41:48 PDT
*** Bug 67721 has been marked as a duplicate of this bug. ***
Comment 18 Adam Roben (:aroben) 2011-09-22 20:03:05 PDT
As noted in bug 67721, sometimes things work just fine in Release builds.
Comment 19 Radar WebKit Bug Importer 2011-09-23 10:15:45 PDT
<rdar://problem/10175703>
Comment 20 Leo Yang 2011-09-26 01:11:51 PDT
Created attachment 108642 [details]
Patch v5
Comment 21 Darin Adler 2011-12-01 11:38:30 PST
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.
Comment 22 Leo Yang 2011-12-01 20:37:10 PST
Created attachment 117555 [details]
Patch for landing
Comment 23 WebKit Review Bot 2011-12-02 00:05:27 PST
Comment on attachment 117555 [details]
Patch for landing

Clearing flags on attachment: 117555

Committed r101753: <http://trac.webkit.org/changeset/101753>
Comment 24 WebKit Review Bot 2011-12-02 00:05:34 PST
All reviewed patches have been landed.  Closing bug.