Bug 97247 - [chromium] REGRESSION Boundaries with alpha have wrong widths when div rotated and offset to non-integer coordinates
Summary: [chromium] REGRESSION Boundaries with alpha have wrong widths when div rotate...
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: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-20 12:07 PDT by Emil A Eklund
Modified: 2013-02-14 14:16 PST (History)
3 users (show)

See Also:


Attachments
Patch (20.58 KB, patch)
2012-09-20 12:14 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (20.80 KB, patch)
2012-09-20 16:48 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (20.82 KB, patch)
2012-09-20 17:43 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (20.89 KB, patch)
2012-09-21 15:52 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2012-09-20 12:07:31 PDT
The following draws two divs. Both are rotated by 45°, are offset to non-integer pixel coordinates and have 1px wide borders. The first div's border color is black, the second has an alpha of 0.99 (almost black):

data:text/html,<div style="position:absolute; left: 30.5px; top:30.3px; width: 30px; height: 30px; border: 1px solid rgba(0, 0, 0, 1.00); -webkit-transform: rotate(45deg);"></div><div style="position:absolute; left: 30.5px; top:90.3px; width: 30px; height: 30px; border: 1px solid rgba(0, 0, 0, 0.99); -webkit-transform: rotate(45deg);"></div>

On Chrome 22, the two divs look identical.

On Chrome 23, the second div has incorrect boundary widths: The top left boundary is 2px wide, the bottom right boundary is completely invisible.

Downstream chromium bug: http://code.google.com/p/chromium/issues/detail?id=148734
Comment 1 Emil A Eklund 2012-09-20 12:14:35 PDT
Created attachment 164963 [details]
Patch
Comment 2 Emil A Eklund 2012-09-20 12:16:48 PDT
I'm not entirely convinced that this approach is correct so I'd appreciate input from someone (levi) familiar with the logic.
Comment 3 Levi Weintraub 2012-09-20 13:15:19 PDT
Comment on attachment 164963 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:3048
> +        // Accumulate and account for sub-pixel rounding to ensure that we round in the right direction.

This would probably make more sense in a static inline function with a descriptive name.

> Source/WebCore/rendering/RenderLayer.cpp:3050
> +        LayoutUnit adjustedWidth = (subPixelAccumulation.width() + (delta.x() - roundedDelta.x())).abs();
> +        LayoutUnit adjustedHeight = (subPixelAccumulation.height() + (delta.y() - roundedDelta.y())).abs();

I don't like the named adjustedWidth/Height.

Why are we taking the absolute value?

> Source/WebCore/rendering/RenderLayer.cpp:3054
> +        while (adjustedWidth > 1)
> +            adjustedWidth -= 1;
> +        while (adjustedHeight > 1)
> +            adjustedHeight -= 1;

I can understand how this got over 1, but it should never be 2 or more. abs(subPixelAccumulation) is between 0 and 1, and abs(delta - roundedDelta) should be  <= 0.5. An assertion of this fact would be good. I don't like these while loops at all.
Comment 4 Levi Weintraub 2012-09-20 13:22:07 PDT
(In reply to comment #2)
> I'm not entirely convinced that this approach is correct so I'd appreciate input from someone (levi) familiar with the logic.

FWIW, this is definitely the right approach except those while loops :)
Comment 5 Emil A Eklund 2012-09-20 16:48:49 PDT
Created attachment 165012 [details]
Patch
Comment 6 Build Bot 2012-09-20 17:18:25 PDT
Comment on attachment 165012 [details]
Patch

Attachment 165012 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13916345
Comment 7 Emil A Eklund 2012-09-20 17:43:46 PDT
Created attachment 165020 [details]
Patch
Comment 8 Levi Weintraub 2012-09-21 15:47:58 PDT
Comment on attachment 165020 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:2979
> +static inline LayoutSize adjustSubPixelAccumulation(LayoutSize subPixelAccumulation, LayoutSize roundingAdjustment)

We talked in person about finding a better name for this. Otherwise this looks good.
Comment 9 Levi Weintraub 2012-09-21 15:49:27 PDT
Comment on attachment 165020 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:2982
> +    LayoutUnit w = (subPixelAccumulation.width() + roundingAdjustment.width()).abs();
> +    LayoutUnit h = (subPixelAccumulation.height() + roundingAdjustment.height()).abs();

Also, we should assert that the abs(w and h) is less than 2 or simply that the final results are less than 1.
Comment 10 Emil A Eklund 2012-09-21 15:52:12 PDT
Created attachment 165217 [details]
Patch
Comment 11 Bartosz Fabianowski 2012-10-26 02:18:42 PDT
Pinging this bug.

We are launching a feature in Chrome 24 that uses this kind of rotated div. It would be nice to get this fixed to avoid the visual glitch.

It seems that there is a patch that got stranded in review?
Comment 12 Emil A Eklund 2012-10-26 08:15:52 PDT
Comment on attachment 165217 [details]
Patch

There have been quite a few changes to the relevant part of the code since this patch was posted. Marking as obsolete for now but will try to post a new one in the next couple of days.
Comment 13 Emil A Eklund 2013-02-14 14:13:59 PST
This was fixed by r142638.
Comment 14 Levi Weintraub 2013-02-14 14:16:10 PST
Fantabulous!