1002466 - With Skia content, -moz-column-rule:dotted is not rendered at all

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

This causes the mochitest failures of the form:

 TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_visited_reftests.html | (20) != column-rule-1.html column-rule-1-notref.html

On https://tbpl.mozilla.org/?tree=Graphics

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

'solid' is correctly rendered, but 'dotted' is not rendered at all. Happens on Linux and Windows, not Mac.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: minimal testcase

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Some progress. (Thanks to :gw280 who spent time today to teach me Skia and Moz2D things).

The issue is that this stipple pattern has 'on' segments of length 0, which Moz2D thinks is fine (the rounded caps around it are meant to provide the round dots) but which Skia thinks means there is nothing to render. Thus, Skia is taking an early return, specifically in SkScan::AntiFillPath, here:

616	    if (ir.isEmpty()) {
617	        if (path.isInverseFillType()) {
618	            blitter->blitRegion(origClip);
619	        }
620	        return;
621	    }
(gdb) n
620	        return;
(gdb) bt 20
#0  SkScan::AntiFillPath (path=..., origClip=..., blitter=0x7fffffff8ec8, forceRLE=false)
    at /hack/mozilla-graphics/gfx/skia/trunk/src/core/SkScan_AntiPath.cpp:620
#1  0x00007ffff20de167 in SkScan::AntiFillPath (path=..., clip=..., blitter=0x7fffffff8ec8)
    at /hack/mozilla-graphics/gfx/skia/trunk/src/core/SkScan_AntiPath.cpp:732
#2  0x00007ffff20521df in SkDraw::drawPath (this=0x7fffffff9340, origSrcPath=..., 
    origPaint=..., prePathMatrix=0x50ff8001, pathIsMutable=false, drawCoverage=false)
    at /hack/mozilla-graphics/gfx/skia/trunk/src/core/SkDraw.cpp:1152
#3  0x00007ffff1ff4904 in SkDraw::drawPath (this=0x7fffffff9340, path=..., paint=..., 
    prePathMatrix=0x0, pathIsMutable=false)
    at /hack/mozilla-graphics/gfx/skia/trunk/include/core/SkDraw.h:50
#4  0x00007ffff1ff3bc1 in SkBitmapDevice::drawPath (this=0x7fffc6a6f200, draw=..., path=..., 
    paint=..., prePathMatrix=0x0, pathIsMutable=false)
    at /hack/mozilla-graphics/gfx/skia/trunk/src/core/SkBitmapDevice.cpp:420
#5  0x00007ffff2035dc1 in SkCanvas::drawPath (this=0x7fffc828c210, path=..., paint=...)
    at /hack/mozilla-graphics/gfx/skia/trunk/src/core/SkCanvas.cpp:2020
#6  0x00007fffeec2b776 in mozilla::gfx::DrawTargetSkia::Stroke (this=0x7fffc612cc80, 
    aPath=0x7fffc7797a90, aPattern=..., aStrokeOptions=..., aOptions=...)
    at /hack/mozilla-graphics/gfx/2d/DrawTargetSkia.cpp:415
#7  0x00007fffeed8725c in gfxContext::Stroke (this=0x7fffc76869c0)
    at /hack/mozilla-graphics/gfx/thebes/gfxContext.cpp:349
#8  0x00007ffff0b51f54 in nsCSSBorderRenderer::DrawDashedSide (this=0x7fffffff9ab8, 
    aSide=mozilla::css::eSideLeft)
    at /hack/mozilla-graphics/layout/base/nsCSSRenderingBorders.cpp:992
#9  0x00007ffff0b3e5a9 in nsCSSBorderRenderer::DrawBorders (this=0x7fffffff9ab8)
    at /hack/mozilla-graphics/layout/base/nsCSSRenderingBorders.cpp:1915
#10 0x00007ffff0b3beee in nsCSSRendering::PaintBorderWithStyleBorder (
    aPresContext=0x7fffc760c000, aRenderingContext=..., aForFrame=0x7fffcc9a02f0, 
    aDirtyRect=..., aBorderArea=..., aStyleBorder=..., aStyleContext=0x7fffcc99fbf0, 
    aSkipSides=7) at /hack/mozilla-graphics/layout/base/nsCSSRendering.cpp:573
#11 0x00007ffff0bf5175 in nsColumnSetFrame::PaintColumnRule (this=0x7fffcc9a02f0, 
    aCtx=0x7fffc76fa780, aDirtyRect=..., aPt=...)
    at /hack/mozilla-graphics/layout/generic/nsColumnSetFrame.cpp:114
#12 0x00007ffff0bf7f15 in PaintColumnRule (aFrame=0x7fffcc9a02f0, aCtx=0x7fffc76fa780, 
    aDirtyRect=..., aPt=...) at /hack/mozilla-graphics/layout/generic/nsColumnSetFrame.cpp:49
#13 0x00007ffff0bc3ee8 in nsDisplayGeneric::Paint (this=0x7fffc6a8c420, 
    aBuilder=0x7fffffffb238, aCtx=0x7fffc76fa780)
    at /hack/mozilla-graphics/layout/generic/../base/nsDisplayList.h:1846
#14 0x00007ffff0aee25d in mozilla::FrameLayerBuilder::PaintItems (this=0x7fffc66bbcc0, 
    aItems=..., aRect=..., aContext=0x7fffc76869c0, aRC=0x7fffc76fa780, 
    aBuilder=0x7fffffffb238, aPresContext=0x7fffc760c000, aOffset=..., aXScale=1, aYScale=1, 
    aCommonClipCount=0) at /hack/mozilla-graphics/layout/base/FrameLayerBuilder.cpp:3635
#15 0x00007ffff0aef27e in mozilla::FrameLayerBuilder::DrawThebesLayer (
    aLayer=0x7fffc6a8d000, aContext=0x7fffc76869c0, aRegionToDraw=..., 
    aClip=mozilla::layers::DRAW_SNAPPED, aRegionToInvalidate=..., 
    aCallbackData=0x7fffffffb238)
    at /hack/mozilla-graphics/layout/base/FrameLayerBuilder.cpp:3797
#16 0x00007fffeedf4f0d in mozilla::layers::BasicThebesLayer::PaintBuffer (
    this=0x7fffc6a8d000, aContext=0x7fffc76869c0, aRegionToDraw=..., 
    aExtendedRegionToDraw=..., aRegionToInvalidate=..., aDidSelfCopy=false, 
    aClip=mozilla::layers::DRAW_SNAPPED, 
---Type <return> to continue, or q <return> to quit---
    aCallback=0x7ffff0aeebc0 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*)>, aCallbackData=0x7fffffffb238)
    at /hack/mozilla-graphics/gfx/layers/basic/BasicThebesLayer.h:112
#17 0x00007fffeede97a7 in mozilla::layers::BasicThebesLayer::Validate (this=0x7fffc6a8d000, 
    aCallback=0x7ffff0aeebc0 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*)>, aCallbackData=0x7fffffffb238)
    at /hack/mozilla-graphics/gfx/layers/basic/BasicThebesLayer.cpp:200
#18 0x00007fffeede9957 in non-virtual thunk to mozilla::layers::BasicThebesLayer::Validate(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*) (this=0x7fffc6a8d1f8, 
    aCallback=0x7ffff0aeebc0 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*)>, aCallbackData=0x7fffffffb238) at Unified_cpp_gfx_layers1.cpp:218
#19 0x00007fffeede425e in mozilla::layers::BasicContainerLayer::Validate (
    this=0x7fffc6a8cc00, 
    aCallback=0x7ffff0aeebc0 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*)>, aCallbackData=0x7fffffffb238)
    at /hack/mozilla-graphics/gfx/layers/basic/BasicContainerLayer.cpp:122
(More stack frames follow...)

Comment 4

[George Wright (:gw280) (needinfo me!)]

Related: https://code.google.com/p/skia/issues/detail?id=1040

tldr: Skia considers zero length lines as degenerate enough to skip drawing them, even if the linecap style is set to round or square. In the latter two cases, on most 2d rasterisers (cairo, quartz, d2d, etc), the line segment has length zero but the linecap is still rendered, resulting in either a circular dot or a square to be rendered. This is also consistent with the SVG spec.

The bug linked above suggests that the Skia team doesn't consider this a valid bug due to internal API consistencies, so we may have to hack around this. My proposed solution is to check if any of the dash lengths are 0.0 and replace them with SK_ScalarNearlyZero.

Comment 5

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1002466-Use-SK_ScalarNearlyZero-instead-of-0.0f-.patch

Is this evil?

Comment 6

[George Wright (:gw280) (needinfo me!)]

(Sorry for hijacking your bug Benoit!)

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Well this does sound evil, but it might be OK in an evil world!

In my local testing 0.01 did make things look about right, so I do believe that this will make this bug disappear; but this also seems slightly unsatisfactory so I would like to keep debugging a bit more before giving up on "a better fix".

I'm not the right person to review Skia patches, I believe; but you could maybe push this to the Graphics tree without review and see what TBPL says? Or can you push Graphics to try?

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

And, you're not hijacking my bug, you're helping me. Thanks!

Comment 9

[George Wright (:gw280) (needinfo me!)]

(In reply to Benoit Jacob [:bjacob] from comment #7)
> I'm not the right person to review Skia patches, I believe; but you could
> maybe push this to the Graphics tree without review and see what TBPL says?

Way ahead of you :)

Comment 10

[George Wright (:gw280) (needinfo me!)]

Upstream and related bugs:

Skia:
https://code.google.com/p/skia/issues/detail?id=2498

Chromium:
http://crbug.com/157347

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8414838 [details] [diff] [review]
0001-Bug-1002466-Use-SK_ScalarNearlyZero-instead-of-0.0f-.patch

Review of attachment 8414838 [details] [diff] [review]:
-----------------------------------------------------------------

This is honestly just awful. I'd prefer if we wait to see if skia will fix this properly upstream, but you can land this if you need to in the interim.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Chrome doesn't currently support dotted borders so they don't run into this: http://www.impressivewebs.com/comparison-css-border-style/

It would be nice to fix this upstream, but it might not be super easy. Let's take this patch for now, just to avoid blocking on it.

Comment 13

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74399f9da0e5

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/74399f9da0e5