Bug 70471 - Seaming on border corners with mixed colour alpha borders
Summary: Seaming on border corners with mixed colour alpha borders
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ben Wells
URL:
Keywords:
Depends on: 72062
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-19 20:59 PDT by Ben Wells
Modified: 2011-11-16 16:57 PST (History)
4 users (show)

See Also:


Attachments
Testcase - div on right shows seaming (653 bytes, text/html)
2011-10-19 20:59 PDT, Ben Wells
no flags Details
Patch for comment (needs test) (3.63 KB, patch)
2011-10-20 16:33 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch with test (15.83 KB, patch)
2011-11-14 22:33 PST, Ben Wells
no flags Details | Formatted Diff | Diff
Patch with better test (23.60 KB, patch)
2011-11-15 18:59 PST, 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 Ben Wells 2011-10-19 20:59:13 PDT
Created attachment 111721 [details]
Testcase - div on right shows seaming

If you have a border edge which has some transparency against another edge which also has transparency but a different color, seaming is evident. This doesn't happen with dashed or dotted borders. Also, it doesn't happen with skia.
Comment 1 Simon Fraser (smfr) 2011-10-19 21:27:42 PDT
Might depend on whether we draw with antialiasing or not.
Comment 2 Ben Wells 2011-10-19 21:34:13 PDT
Yes, with dashed / dotted we clip out the polygon (which uses anti-aliasing), with the others we just use drawConvexPolygon with the border side shape. I have a simple fix for this to clip out if there is transparency but need to do some more testing first.

Skia draws polygons slightly differently when antialiasing is off (less pixels are drawn), which is why it doesn't happen with skia.
Comment 3 Ben Wells 2011-10-20 16:33:24 PDT
Created attachment 111871 [details]
Patch for comment (needs test)
Comment 4 Ben Wells 2011-10-20 16:35:26 PDT
Comment on attachment 111871 [details]
Patch for comment (needs test)

This patch fixes the problem by using anti-aliasing on mitred edges (via the edge clip polygon) in some more cases.

An alternative (and simpler) fix would be to make the top and bottom edge polygons slightly smaller to avoid overdrawing, however that will introduce gaps on skia.
Comment 5 WebKit Review Bot 2011-10-20 18:37:58 PDT
Comment on attachment 111871 [details]
Patch for comment (needs test)

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

New failing tests:
fast/borders/mixed-border-styles.html
Comment 6 Simon Fraser (smfr) 2011-11-10 15:09:51 PST
Comment on attachment 111871 [details]
Patch for comment (needs test)

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

Looks OK but could use a test.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Seems like you could have a pixel test for this.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1409
> +        bool clipAdjacentSide1 = colorNeedsAntiAliasAtCorner(side, adjacentSide1, edges) && mitreAdjacentSide1;
> +        bool clipAdjacentSide2 = colorNeedsAntiAliasAtCorner(side, adjacentSide1, edges) && mitreAdjacentSide2;

You're calling colorNeedsAntiAliasAtCorner(side, adjacentSide1, edges) twice here.
Comment 7 Ben Wells 2011-11-14 22:33:28 PST
Created attachment 115098 [details]
Patch with test
Comment 8 Simon Fraser (smfr) 2011-11-15 11:09:31 PST
Comment on attachment 115098 [details]
Patch with test

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1527
> +        bool clipForStyle = styleRequiresClipPolygon(edgeToRender.style) && (mitreAdjacentSide1 || mitreAdjacentSide2);
> +        bool clipAdjacentSide1 = colorNeedsAntiAliasAtCorner(side, adjacentSide1, edges) && mitreAdjacentSide1;
> +        bool clipAdjacentSide2 = colorNeedsAntiAliasAtCorner(side, adjacentSide2, edges) && mitreAdjacentSide2;
> +        bool shouldClip = clipForStyle || clipAdjacentSide1 || clipAdjacentSide2;

If you avoided the local variables, the test could short-circuit, which would be slightly more efficient (if not quite as readable).

> LayoutTests/fast/borders/border-mixed-alpha.html:11
> +    div {
> +        width: 20px;
> +        height: 20px;
> +        display: inline-block;
> +        border-width: 40px;
> +        border-color: rgba(0, 255, 0, 0.5);
> +        border-right-color: rgba(0, 0, 255, 0.5);
> +    }
> +    .dashed { border-style: dashed; }

Can you make everything bigger here, so that the test fills the image (without overflowing).
Comment 9 Ben Wells 2011-11-15 18:59:58 PST
Created attachment 115296 [details]
Patch with better test
Comment 10 Ben Wells 2011-11-15 19:05:47 PST
Comment on attachment 115296 [details]
Patch with better test

I've made the test result fill much more of the space.

The variables which could be removed to enable short-circuit evaluation are all used later; if the test short circuits they will be needed so any savings will be lost.
Comment 11 WebKit Review Bot 2011-11-16 16:57:46 PST
Comment on attachment 115296 [details]
Patch with better test

Clearing flags on attachment: 115296

Committed r100528: <http://trac.webkit.org/changeset/100528>
Comment 12 WebKit Review Bot 2011-11-16 16:57:51 PST
All reviewed patches have been landed.  Closing bug.