Bug 58999

Created attachment 91646 [details] Patch

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 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.

(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.

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

(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.

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.

Created attachment 92774 [details] Patch

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/

Borders are fixed. Try a recent build.

Created attachment 93226 [details] Patch

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 on attachment 93226 [details] Patch Patch failed to apply, will resubmit after getting up to tip

Created attachment 93237 [details] Patch that landed at r86314

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)).

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 on attachment 93237 [details] Patch that landed at r86314 Clearing flags on attachment: 93237 Committed r86314: <http://trac.webkit.org/changeset/86314>

All reviewed patches have been landed. Closing bug.

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?

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

(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 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.

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

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 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>

All reviewed patches have been landed. Closing bug.

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

(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.