Bug 58999 - RGBA colors in outlines show overpainting at the corners
Summary: RGBA colors in outlines show overpainting at the corners
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Ben Wells
URL:
Keywords:
Depends on: 59911
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-20 09:12 PDT by Dimitri Glazkov (Google)
Modified: 2011-06-09 18:10 PDT (History)
9 users (show)

See Also:


Attachments
Patch (27.54 KB, patch)
2011-04-28 23:57 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch (221.63 KB, patch)
2011-05-09 02:28 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch (221.65 KB, patch)
2011-05-11 18:00 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch that landed at r86314 (221.71 KB, patch)
2011-05-11 20:08 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch to clean up tests, test expectations, and add FIXMEs to code. (23.79 KB, patch)
2011-05-12 20:08 PDT, Ben Wells
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2011-04-20 09:12:39 PDT
In this example: http://jsfiddle.net/dglazkov/TD5Rc/, we shouldn't see darker dots in the corners.
Comment 1 Ben Wells 2011-04-28 23:57:52 PDT
Created attachment 91646 [details]
Patch
Comment 2 Ben Wells 2011-04-29 00:04:10 PDT
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?
Comment 3 Simon Fraser (smfr) 2011-04-29 08:39:00 PDT
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.
Comment 4 Ben Wells 2011-05-01 16:11:49 PDT
(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.
Comment 5 Ben Wells 2011-05-02 23:00:21 PDT
Here is an example of the same problem with borders: http://jsfiddle.net/YZPpm/
Comment 6 Simon Fraser (smfr) 2011-05-02 23:03:00 PDT
(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.
Comment 7 Ben Wells 2011-05-03 23:37:51 PDT
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.
Comment 8 Ben Wells 2011-05-09 02:28:30 PDT
Created attachment 92774 [details]
Patch
Comment 9 Simon Fraser (smfr) 2011-05-09 08:28:38 PDT
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.
Comment 10 krinklemail 2011-05-11 07:26:39 PDT
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/
Comment 11 Simon Fraser (smfr) 2011-05-11 07:45:13 PDT
Borders are fixed. Try a recent build.
Comment 12 Ben Wells 2011-05-11 18:00:00 PDT
Created attachment 93226 [details]
Patch
Comment 13 Ben Wells 2011-05-11 18:03:47 PDT
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
Comment 14 Ben Wells 2011-05-11 18:10:03 PDT
Comment on attachment 93226 [details]
Patch

Patch failed to apply, will resubmit after getting up to tip
Comment 15 Ben Wells 2011-05-11 20:08:22 PDT
Created attachment 93237 [details]
Patch that landed at r86314
Comment 16 Simon Fraser (smfr) 2011-05-11 21:37:57 PDT
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)).
Comment 17 WebKit Commit Bot 2011-05-12 00:02:58 PDT
The commit-queue encountered the following flaky tests while processing attachment 93237 [details]:

http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: ap@webkit.org)
http/tests/xmlhttprequest/xmlhttprequest-50ms-download-dispatch.html bug 52016 (author: jchaffraix@webkit.org)
The commit-queue is continuing to process your patch.
Comment 18 WebKit Commit Bot 2011-05-12 00:04:42 PDT
Comment on attachment 93237 [details]
Patch that landed at r86314

Clearing flags on attachment: 93237

Committed r86314: <http://trac.webkit.org/changeset/86314>
Comment 19 WebKit Commit Bot 2011-05-12 00:04:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 James Robinson 2011-05-12 09:05:14 PDT
No, the new rendering on chromium windows and linux is a regression.
Comment 22 James Robinson 2011-05-12 11:29:49 PDT
(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.
Comment 23 Simon Fraser (smfr) 2011-05-12 12:34:40 PDT
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.
Comment 24 Ben Wells 2011-05-12 20:08:08 PDT
Created attachment 93394 [details]
Patch to clean up tests, test expectations, and add FIXMEs to code.
Comment 25 WebKit Commit Bot 2011-05-12 22:22:18 PDT
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: alice.liu@apple.com)
The commit-queue is continuing to process your patch.
Comment 26 WebKit Commit Bot 2011-05-12 22:26:05 PDT
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>
Comment 27 WebKit Commit Bot 2011-05-12 22:26:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Ryosuke Niwa 2011-06-09 02:32:29 PDT
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
Comment 29 James Kozianski 2011-06-09 18:10:44 PDT
(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.