Bug 89958 – REGRESSION(r107836): box shadow not drawn for opaque images with an opaque background

• Component: Layout and Rendering
• Status: RESOLVED FIXED

r107836 made it possible for the box shadow to be painted as part of the background, however RenderImage does not paint its background when it believes it to be obscured.

Reported in Chromium: http://code.google.com/p/chromium/issues/detail?id=134243

Created attachment 149485 [details]
Patch

[WebKit Review Bot]

Comment on attachment 149485 [details]
Patch

Attachment 149485 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13099353

New failing tests:
fast/box-shadow/image-box-shadow.html

[WebKit Review Bot]

Created attachment 149493 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Radar WebKit Bug Importer]

<rdar://problem/11748840>

Created attachment 149641 [details]
Patch ready for review.

[Eric Seidel (no email)]

Comment on attachment 149641 [details]
Patch ready for review.

LGTM.

[Simon Fraser (smfr)]

Comment on attachment 149641 [details]
Patch ready for review.

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

> Source/WebCore/rendering/RenderImage.cpp:452
> +    return !backgroundIsObscured() && RenderBoxModelObject::boxShadowShouldBeAppliedToBackground(bleedAvoidance);

Might be cleaner as:

if (!RenderBoxModelObject::boxShadowShouldBeAppliedToBackground(bleedAvoidance))
  return false;

return ! backgroundIsObscured()

> LayoutTests/fast/box-shadow/image-box-shadow.html:28
> +function addTestImage(color, type) {
> +  var canvas = document.createElement('canvas');
> +  document.body.appendChild(canvas);
> +  canvas.width = canvas.height = 1;
> +  var ctx = canvas.getContext("2d");
> +  ctx.fillStyle = color;
> +  ctx.fillRect(0, 0, 1, 1);
> +  var div = document.createElement('div');
> +  var img = document.createElement('img');
> +  img.src = canvas.toDataURL(type);
> +  div.appendChild(img);

Why does this test and the reference need a canvas? Can't you just compare an opaque <img> with a non-opaque one?

[Simon Fraser (smfr)]

Comment on attachment 149641 [details]
Patch ready for review.

r- to avoid review clobber

[Simon Fraser (smfr)]

Also, the behavior of canvas.toDataURL changes under HiDPI, so using it in tests is not ideal.

(In reply to comment #7)
> Why does this test and the reference need a canvas? Can't you just compare an opaque <img> with a non-opaque one?

The canvas is only used to generate the images. I was trying to avoid adding additional image files, is it preferable to add them?

(In reply to comment #9)
> Also, the behavior of canvas.toDataURL changes under HiDPI, so using it in tests is not ideal.

I see, I assume since I'm just generating and stretching a 1x1 image this wont cause an issue in this particular test.

Created attachment 149662 [details]
Patch

r?

(In reply to comment #7)
> Might be cleaner as:
> 
> if (!RenderBoxModelObject::boxShadowShouldBeAppliedToBackground(bleedAvoidance))
>   return false;
> 
> return ! backgroundIsObscured()

Done.

(In reply to comment #7)
> Why does this test and the reference need a canvas? Can't you just compare an opaque <img> with a non-opaque one?

I've added image resources and removed the canvas dependency.

[WebKit Review Bot]

Comment on attachment 149662 [details]
Patch

Clearing flags on attachment: 149662

Committed r121314: <http://trac.webkit.org/changeset/121314>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.