Bug 41576 – [chromium] Fix drawing of inset box shadows

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Nico Weber]

http://crbug.com/29427

[Nico Weber]

Created attachment 60466 [details]
patch

[Nico Weber]

Created attachment 60467 [details]
patch without junk

cq- because this needs rebaselining

[Nico Weber]

Eric, you reviewed the patch that this patch is fixing. Can you review this, too? It's just one line.

[Dimitri Glazkov (Google)]

You still have junk in the ChangeLog.

Hyatt, Mitz -- can you take a look?

Comment on attachment 60467 [details]
patch without junk

A method called GraphicsContext::canvasClip() sounds like a bad idea. According to GraphicsContext.h,

// This clip function is used only by <canvas> code

but this patch makes that comment false and illustrates why having this method is a bad idea. Moreover, the change log (which mentions a bunch of files that the patch doesn’t touch) does a very poor job of explaining the change.

I suggest eliminating canvasClip() and devising a better solution to the platform-specific problem it was intended to solve; ideally, such a solution will not require people writing cross-platform code to have to think about the platform-specific problem. As things stand, there is no way for someone writing rendering code to tell whether they should call clip() or canvasClip().

[Adam Langley]

(In reply to comment #5)
> but this patch makes that comment false and illustrates why having this method is a bad idea. Moreover, the change log (which mentions a bunch of files that the patch doesn’t touch) does a very poor job of explaining the change.

The point of canvas clip was that anti-aliased clipping causes seams when attempting to join two polygons. So the canvas folks didn't want it on by default, but everyone else was calling for my blood over anti-aliased rounded corners.

I'm afraid that the ChangeLog here is insufficient to understand the problem. Why does the layered clipping fail?

> I suggest eliminating canvasClip() and devising a better solution to the platform-specific problem it was intended to solve; ideally, such a solution will not require people writing cross-platform code to have to think about the platform-specific problem. As things stand, there is no way for someone writing rendering code to tell whether they should call clip() or canvasClip().

Possibly canvasClip should have been called aliasedClip. However, it's very easy to tell if you should be calling canvasClip: if you're not rendering to a <canvas> element then you shouldn't.

[Nico Weber]

> I'm afraid that the ChangeLog here is insufficient to understand the problem. Why does the layered clipping fail?

It doesn't seem to be compatible with how skia draws shadows. I'm not sure why not.

It works with non-inset shadows by accident, since these are drawn with a clipOut() path, which doesn't use the antialiasing trick (it should – aliased shadows are ugly. But then the problem would be visible with normal, non-inset shadows too).

I agree with mitz that the antialiasing trick should be made to work with shadows too and that my patch isn't a good idea.

[Adam Langley]

(In reply to comment #7)
> It works with non-inset shadows by accident, since these are drawn with a clipOut() path, which doesn't use the antialiasing trick (it should – aliased shadows are ugly. But then the problem would be visible with normal, non-inset shadows too).

clipOut doesn't anti-alias because that would involve duplicating the frame buffer every time a clip was in effect.

I don't suppose anyone wants to add the immediate mode anti-aliasing hack into Skia and save all this bother?

[Hajime Morrita]

Created attachment 75365 [details]
Patch

[Hajime Morrita]

Hi Nico, Dimitri, Darin, could you take a look?
This change is now small, and closed under platform/ chromium part.

[Nico Weber]

Makes sense. I'd love agl to have a look, too.

Maybe add

// The state is popped in applyAntiAliasedClipPaths()

to the line you added in clipPathAntiAliased, but LG from me.

Also, I guess the shadow edges still look aliased with your patch (but they will still look way better than they do now).

[Hajime Morrita]

Created attachment 75466 [details]
Patch

[Hajime Morrita]

Hi Nico, thank you for trying this!

(In reply to comment #11)
> Makes sense. I'd love agl to have a look, too.
Sure.

> 
> Maybe add
> 
> // The state is popped in applyAntiAliasedClipPaths()
Added.

> Also, I guess the shadow edges still look aliased with your patch (but they will still look way better than they do now).
Yes, In my understanding, this is because WebKit clips 
the background rectangle and the inset shadow separately, 
which causes weird appearance on the edge's partially-covered pixels.
I guess even Mac port has same problem because such clipping order is controlled by WebCore.
It might be possible to move clipping call down to RenderBox::paintBoxDecorationsWithSize().
(It is another story though.)

[Adam Langley]

Sorry for the delay. Please bug me often if you have a review outstanding!

LGTM

[Dimitri Glazkov (Google)]

Comment on attachment 75466 [details]
Patch

rs=me if agl says it's good.

[Eric Seidel (no email)]

Attachment 75466 [details] was posted by a committer and has review+, assigning to MORITA Hajime for commit.

[Hajime Morrita]

Committed r74089: <http://trac.webkit.org/changeset/74089>

[Hajime Morrita]

agl, Dimitri, thank you for taking a look! 
I was unnecessarily hesitate to shoot a ping :-/