Bug 58999 – RGBA colors in outlines show overpainting at the corners

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dimitri Glazkov (Google)]

In this example: http://jsfiddle.net/dglazkov/TD5Rc/, we shouldn't see darker dots in the corners.

[Ben Wells]

Created attachment 91646 [details]
Patch

[Ben Wells]

The patch just submitted fixes this bug for outlines for most cases. It doesn't fix the case where the outline is around a box of width and / or heigh or 0 pixels. More importantly it doesn't fix borders which also have the problem, and from looking at the code there is probably a similar problem for table cells.

The current fix is in a function paintOutline, which calls another function drawLineForBoxSide. I believe to fix the other cases mentioned above (borders, 0 pixel sized boxes) the best approach would be to remove the change in paintOutline and instead make a change in drawLineForBoxSide.

Comments, anyone?

[Simon Fraser (smfr)]

Comment on attachment 91646 [details]
Patch

It would be great if you could rename oc, ow and os in a commit before this one, to make the code more readable.

I still see some overdraw in your 'alphaaqua' examples, so this code isn't perfect.

[Ben Wells]

(In reply to comment #3)
> (From update of attachment 91646 [details])
> It would be great if you could rename oc, ow and os in a commit before this one, to make the code more readable.
> 
> I still see some overdraw in your 'alphaaqua' examples, so this code isn't perfect.

Sure, renaming those variables is a good idea, they're a little cryptic as is.

You're right, outlines around boxes with 0 pixel width or height still have problems. The code specifically doesn't fix that situation as it causes asserts to fail lower down the call stack, and to fix these cases a different approach is needed. To fix the same problems with borders will also need changes further down the call stack, and I think that's a better place to fix this anyway.

Over the weekend I tested the bug on a Windows and Linux box with Chrome / Chromium and noticed that it doesn't have the problem reported. I need to investigate this a bit more, as it suggests the problem is in a different area.

[Ben Wells]

Here is an example of the same problem with borders: http://jsfiddle.net/YZPpm/

[Simon Fraser (smfr)]

(In reply to comment #5)
> Here is an example of the same problem with borders: http://jsfiddle.net/YZPpm/
Borders have been fixed. Try a newer build.

[Ben Wells]

Thanks Simon, I see what you mean. I've had a look at the changes you made to fix the same problem in borders and am working on a fix using layer transparency. It should be ready soon, once I've had a look at the skia behaviour.

[Ben Wells]

Created attachment 92774 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 92774 [details]
Patch

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

r+ but please consider a followup to change RenderObject to fill a path when possible.

> Source/WebCore/rendering/RenderObject.cpp:1058
> +    bool useTransparencyLayer = outlineColor.hasAlpha();
> +    if (useTransparencyLayer) {
> +        graphicsContext->beginTransparencyLayer(static_cast<float>(outlineColor.alpha()) / 255);
> +        outlineColor = Color(outlineColor.red(), outlineColor.green(), outlineColor.blue());
> +    }

Transparency layers are pretty expensive, so it would be nice to have code here that fills a path, rather than drawing 4 sides independently, if the outline is solid and the color has alpha. It would be nice to just share code with border drawing if possible.

It's not just a dot in the outline, it also happens with border-color and in the entire corner:

Example:
    border: 10px solid rgba(0,0,0,0.3);

http://jsfiddle.net/TD5Rc/2/

[Simon Fraser (smfr)]

Borders are fixed. Try a recent build.

[Ben Wells]

Created attachment 93226 [details]
Patch

[Ben Wells]

New patch to keep the fix out of skia, for now. 

New bugs to be created:
- refactor to make outline drawing use the border code
- introduce optimisation for alpha'd solid borders / outlines with matching colors on all sides
- fix this bug for skia

[Ben Wells]

Comment on attachment 93226 [details]
Patch

Patch failed to apply, will resubmit after getting up to tip

[Ben Wells]

Created attachment 93237 [details]
Patch that landed at r86314

[Simon Fraser (smfr)]

Comment on attachment 93237 [details]
Patch that landed at r86314

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

> LayoutTests/platform/mac/fast/borders/outline-alpha-block-expected.txt:6
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x338
> +  RenderBlock {HTML} at (0,0) size 800x338
> +    RenderBody {BODY} at (8,8) size 784x322
> +      RenderText {#text} at (0,0) size 781x18

This could be a text test with pixel results (dumpAsText(true)).

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 93237 [details]:

http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: [e-mail])
http/tests/xmlhttprequest/xmlhttprequest-50ms-download-dispatch.html bug 52016 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 93237 [details]
Patch that landed at r86314

Clearing flags on attachment: 93237

Committed r86314: <http://trac.webkit.org/changeset/86314>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[anton muhin]

There are differences between WebKit and Chromium: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20-%20chromium.org&tests=fast%2Fborders%2Foutline-alpha-block.html%2Cfast%2Fborders%2Foutline-alpha-inline.html

Are they fine to rebaseline?

[James Robinson]

No, the new rendering on chromium windows and linux is a regression.

[James Robinson]

(In reply to comment #21)
> No, the new rendering on chromium windows and linux is a regression.

Ah, I didn't realize these were new tests.  The chromium windows/linux rendering is incorrect, so this should be marked as an = IMAGE in test_expectations.txt.  It's not a regression because we never rendered this case correctly.

[Simon Fraser (smfr)]

Comment on attachment 93237 [details]
Patch that landed at r86314

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

> Source/WebCore/rendering/RenderInline.cpp:1401
> +#if !USE(SKIA)

On second thoughts, this is pretty ugly. You should really be #ifdeffing based on some behavioral difference, not just the graphics library. If this is temporary, add a FIXME comment referencing a bug.

[Ben Wells]

Created attachment 93394 [details]
Patch to clean up tests, test expectations, and add FIXMEs to code.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 93394 [details]:

http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 93394 [details]
Patch to clean up tests, test expectations, and add FIXMEs to code.

Clearing flags on attachment: 93394

Committed r86413: <http://trac.webkit.org/changeset/86413>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

fast/borders/outline-alpha-inline.html and fast/borders/outline-alpha-block.html are somehow passing on Chromium Mac bots on build.webkit.org:
http://build.webkit.org/results/Chromium%20Mac%20Release%20(Tests)/r88434%20(8018)/results.html

[James Kozianski]

(In reply to comment #28)
> fast/borders/outline-alpha-inline.html and fast/borders/outline-alpha-block.html are somehow passing on Chromium Mac bots on build.webkit.org:
> http://build.webkit.org/results/Chromium%20Mac%20Release%20(Tests)/r88434%20(8018)/results.html

As discussed with Ben, I've removed them from test_expectations.txt in r88509.