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
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
: data:text/html,%3Cimg%20src=%22http:/...
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2010-07-22 11:59 PST by
Modified: 2011-12-02 00:05 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-07-22 11:59:01 PST
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 From 2010-07-22 12:34:01 PST -------
Created an attachment (id=62327) [details]
Patch
------- Comment #2 From 2010-07-22 13:02:57 PST -------
(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?
------- Comment #3 From 2010-07-22 13:26:56 PST -------
(In reply to comment #2)
> (From update of attachment 62327 [details] [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 From 2010-07-22 13:42:55 PST -------
> 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 From 2010-07-27 08:38:46 PST -------
(From update of attachment 62327 [details])
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 From 2010-07-27 15:54:41 PST -------
Created an attachment (id=62762) [details]
Revised patch
------- Comment #7 From 2010-08-06 13:53:48 PST -------
(From update of attachment 62762 [details])
What if the generated context is invalid?
------- Comment #8 From 2010-08-09 07:16:06 PST -------
(In reply to comment #7)
> (From update of attachment 62762 [details] [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 From 2010-08-29 13:20:16 PST -------
(From update of attachment 62762 [details])
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 From 2010-09-06 20:31:07 PST -------
Created an attachment (id=66680) [details]
Rebased patch
------- Comment #11 From 2010-09-06 21:03:05 PST -------
Created an attachment (id=66682) [details]
Removed extra space from change log
------- Comment #12 From 2010-12-23 00:30:20 PST -------
(From update of attachment 66682 [details])
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 From 2010-12-23 21:52:21 PST -------
Created an attachment (id=77401) [details]
Change boolean to enum

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

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