Bug 105508 – Proposal: Add fillRule to canvas

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[James Ascroft-Leigh]

Created attachment 180297 [details]
First-cut patch to expose the webkitFillRule canvas property

When a canvas path that intersects itself is filled there are multiple strategies for how to select regions that receive the fill.  The SVG standard has adopted two such strategies [11] which are called nonzero (default) and evenodd.

SVG is not the only vector graphics protocol to support these two strategies.  Quoting [1]: "Some other languages/libraries that support even-odd fill (in alphabetical order): cairo, Direct2D, GDI, PDF, PostScript, Quartz, skia, and SVG."  In fact the WebKit abstract GraphicsContext [2], used for the implementation of CanvasRenderingContext2D, supports this property and uses the same nonzero/evenodd terminology to name the to strategies.

It has been proposed - in [3], [4] and [5] - that this property is incorporated into the HTML5 standard.  It has already been incorporated into the Mozilla Firefox web browser as the mozFillRule canvas attribute [6].  An application that makes use of this attribute is pdf.js [7] which is available as an experimental Google Chrome/Chromium plugin [8] but which fails to correctly render some PDFs due to this missing attribute in other browsers [10].

I propose that this property is added to WebKit's canvas as webkitFillRule.  Justification:

   - It is already a de-facto standard graphics operation with widespread implementation
   - The patch is likely to be small (example at [9]) since it only has to expose an existing graphics context property as a canvas property
   - The name webkitFillRule correctly implies that this is not yet a web standard property

As linked above, I have a patch which I have tested in a recent build of Chromium.  This will be my first attempt at a WebKit patch and I would really appreciate any assistance in improving the quality of the patch and getting it ready to merge.

References...

[1]  http://lists.w3.org/Archives/Public/public-whatwg-archive/2011Jun/0123.html
[2]  https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/graphics/GraphicsContext.cpp#L207
[3]  http://lists.w3.org/Archives/Public/public-whatwg-archive/2007May/0338.html
[4]  http://lists.w3.org/Archives/Public/public-whatwg-archive/2011Jun/0123.html
[5]  https://www.w3.org/Bugs/Public/show_bug.cgi?id=19932
[6]  http://hg.mozilla.org/mozilla-central/file/21195f52311c/dom/webidl/CanvasRenderingContext2D.webidl#l140
[7]  https://github.com/mozilla/pdf.js
[8]  https://github.com/mozilla/pdf.js/tree/master/extensions/chrome
[9]  https://github.com/WebKit/webkit/pull/3
[10] https://github.com/mozilla/pdf.js/issues/2351
[11] http://www.w3.org/TR/SVG/painting.html#FillProperties

[James Ascroft-Leigh]

Created attachment 180299 [details]
First-cut patch to expose the webkitFillRule canvas property

The first patch was reversed.  This one is in the correct direction.

[WebKit Review Bot]

Comment on attachment 180299 [details]
First-cut patch to expose the webkitFillRule canvas property

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

New failing tests:
inspector/profiler/canvas2d/canvas2d-api-changes.html

[WebKit Review Bot]

Comment on attachment 180299 [details]
First-cut patch to expose the webkitFillRule canvas property

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

New failing tests:
inspector/profiler/canvas2d/canvas2d-api-changes.html

[Dean Jackson]

I would like to see more discussion on the W3C and/or WHATWG lists before landing this. We generally only expose Web-facing API when it is in a draft specification, although it is interesting that Mozilla have an equivalent property.

[Rik Cabanier]

(In reply to comment #4)
> I would like to see more discussion on the W3C and/or WHATWG lists before landing this. We generally only expose Web-facing API when it is in a draft specification, although it is interesting that Mozilla have an equivalent property.

Yes, this new property will also intersect with the new Path API's.

[Build Bot]

Comment on attachment 180299 [details]
First-cut patch to expose the webkitFillRule canvas property

Attachment 180299 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15457292

New failing tests:
inspector/profiler/canvas2d/canvas2d-api-changes.html

[Build Bot]

Comment on attachment 180299 [details]
First-cut patch to expose the webkitFillRule canvas property

Attachment 180299 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15456303

New failing tests:
inspector/profiler/canvas2d/canvas2d-api-changes.html

[Ian 'Hixie' Hickson]

The reason it's not in the spec is that y'all haven't told the list that you're interested in implementing it — notice the emptiness under Safari/Chrome here for the "fillRule" row:

   http://wiki.whatwg.org/wiki/New_Features_Awaiting_Implementation_Interest

[Ian 'Hixie' Hickson]

(If you implement this exactly like Gecko has, don't prefix it, since that's then what I'll spec. If you want to do something different than Gecko, then mail the list and we'll figure out what it should be and then you can implement that, also without prefix. There shouldn't be a reason to put this in prefixed.)

[James Ascroft-Leigh]

I'm happy to put my name down as an "interested implementer" but I have not contributed to WebKit before so I am not sure if it is appropriate for me to make any declaration on behalf of Apple or Google.

I can confirm that my intention is for the property to behave the same way as mozFillRule and therefore I will rework the patch to name it fillRule (instead of the prefixed webkitFillRule).

[James Ascroft-Leigh]

I have started to document my (evolving) understanding of exactly how this should work.  This includes possible impacts on related areas of functionality that may need retesting after the implementation such as save().

https://github.com/jwal/webkit/wiki/fillRule-specification

I think the acceptance tests we need are fairly obvious from what I have so far.  Can anyone suggest something I have missed?

[James Ascroft-Leigh]

Created attachment 180692 [details]
Patch to expose fillRule canvas property

Patch updated to expose the property as fillRule (instead of the prefixed webkitFillRule).  I also tried to fix the failing test, let's see what the buildbot thinks of the change.

[James Ascroft-Leigh]

I am starting to add some automated tests for this new feature:

https://github.com/jwal/html-testsuite/compare/master

[Rik Cabanier]

(In reply to comment #10)
> I'm happy to put my name down as an "interested implementer" but I have not contributed to WebKit before so I am not sure if it is appropriate for me to make any declaration on behalf of Apple or Google.
> 
> I can confirm that my intention is for the property to behave the same way as mozFillRule and therefore I will rework the patch to name it fillRule (instead of the prefixed webkitFillRule).

You don't have to worry to make additions to WebKit. However, you should announce this feature on [e-mail] so the WebKit development community is aware of your work.

I agree with Ian that you shouldn't prefix your work. EOFill is a well understood, stable feature that won't change.

[Rik Cabanier]

(In reply to comment #12)
> Created an attachment (id=180692) [details]
> Patch to expose fillRule canvas property
> 
> Patch updated to expose the property as fillRule (instead of the prefixed webkitFillRule).  I also tried to fix the failing test, let's see what the buildbot thinks of the change.

I think this feature needs more discussion.
Specifically I'd like to not have the winding rule as part of the graphics state. There really is no reason since this is not a feature you want to have in the environment (like strokewidth, color, etc)

What I'd like to see:
- make the fill rule flag part of the path syntax
- have separate eofill/eoclip for clipping/drawing with the current path
- extend canvas prose so outlines are defined to be unaffected by winding rules.

[James Ascroft-Leigh]

Thanks for the comments about the way this feature is exposed through the canvas API.  I think that a discussion on this topic would be healthy but perhaps this is not the correct forum.  I have just re-raised this proposal on the whatwg mailing list:

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2013-January/038488.html

Hopefully we will get some more comments from there this time around.

Rik: As for your specific suggestions, clearly the eofill(), eoclip() and eoIsPointInPath() alternative would work.  I don't know how to decide between the two proposals except that one of them is more easily compatible with the Mozilla Firefox prefixed implementation.

As for the argument that the winding rule should not be part of the environment, I disagree that this would be a problem in practice.  Although I am not an expert canvas programmer, my initial impressions are that the API itself is inherently a stateful one.  As long as it is properly included in the state with save() and restore() then any client code wishing to make use of this feature wouldn't be much different for either of these APIs.

On the other hand, the only code I have seen which currently makes use of this in pdf.js might benefit from an eofill() function [1] instead of a fillRule property.  I suspect the reason it was added as a fillRule property in the first place was because this is how Cairo (a rendering backend used by Firefox) exposes the feature.  

How should we choose?

[1] It seems to want an eofill() method: https://github.com/mozilla/pdf.js/blob/1826daa1d522bc17015098e79cee7c6ca8fd3b36/src/canvas.js#L648

[James Ascroft-Leigh]

I have also raised this on the [e-mail] mailing list as suggested.

http://lists.webkit.org/pipermail/webkit-dev/2013-January/023209.html

[Rik Cabanier]

(In reply to comment #17)
> I have also raised this on the [e-mail] mailing list as suggested.
> 
> http://lists.webkit.org/pipermail/webkit-dev/2013-January/023209.html

Great! It's probably best to continue the conversation there.
I looked at Skia and they make winding part of the fill and clip call.

[Dirk Schulze]

(In reply to comment #18)
> (In reply to comment #17)
> > I have also raised this on the [e-mail] mailing list as suggested.
> > 
> > http://lists.webkit.org/pipermail/webkit-dev/2013-January/023209.html
> 
> Great! It's probably best to continue the conversation there.
> I looked at Skia and they make winding part of the fill and clip call.

(In reply to comment #15)
> (In reply to comment #12)
> > Created an attachment (id=180692) [details] [details]
> > Patch to expose fillRule canvas property
> > 
> > Patch updated to expose the property as fillRule (instead of the prefixed webkitFillRule).  I also tried to fix the failing test, let's see what the buildbot thinks of the change.
> 
> I think this feature needs more discussion.
> Specifically I'd like to not have the winding rule as part of the graphics state. There really is no reason since this is not a feature you want to have in the environment (like strokewidth, color, etc)
> 
> What I'd like to see:
> - make the fill rule flag part of the path syntax
> - have separate eofill/eoclip for clipping/drawing with the current path
> - extend canvas prose so outlines are defined to be unaffected by winding rules.

fillRule is already part of our graphics state. IIRC it is the default behavior on most 2d graphics libraries. IMO it makes most sense to add an fillRule attribute instead of changing the path syntax. And even if we could have clipRule and fillRule as separate attributes, it should not be a big deal to use the same attribute for both operations. Maybe it means that fillRule should be renamed to windingRule.

[Ian 'Hixie' Hickson]

This is now in the spec.
http://whatwg.org/html/#dom-context-2d-fillrule

[Eric Seidel (no email)]

Comment on attachment 180692 [details]
Patch to expose fillRule canvas property

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:427
> +    WindRule rule;
> +    if (!parseFillRule(s, rule))
> +        return;
> +    if (state().m_fillRule == rule)
> +        return;
> +    realizeSaves();
> +    modifiableState().m_fillRule = rule;
> +    GraphicsContext* c = drawingContext();
> +    if (!c)
> +        return;
> +    c->setFillRule(rule);

This is the only interesting bit of this whole change.  I don't feel up-to-date enough on GraphicsContext to review this part, but otherwise thsi chagne LGTM.

> Source/WebCore/platform/graphics/GraphicsTypes.cpp:111
> +    ASSERT(rule >= 0);
> +    ASSERT(rule < 2);
> +    const char* const names[2] = { "nonzero", "evenodd" };
> +    return names[rule];

I might have written this as an if (rule == NonZero) return "nonzero"; return "evenodd", to avoid any possible OOB, but this is also fine.

[James Ascroft-Leigh]

Created attachment 181308 [details]
Patch to expose fillRule canvas property

Rebase patch against trunk@138802

[James Ascroft-Leigh]

Comment on attachment 180692 [details]
Patch to expose fillRule canvas property

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:427
>> +    c->setFillRule(rule);
> 
> This is the only interesting bit of this whole change.  I don't feel up-to-date enough on GraphicsContext to review this part, but otherwise thsi chagne LGTM.

Indeed, and even this doesn't do very much apart from passing the call onto the lower level API.  To be honest I didn't think too hard about what should be done at this point but I took the other similar functions in the file as a template for this function.  For example https://github.com/jwal/webkit/blob/e8701c215ca234c43b119d3a3a1fe93f28deda34/Source/WebCore/platform/graphics/GraphicsTypes.cpp#L85

I tested this by creating a build of Chromium based on the modified WebKit and ran it on

  a) A modified version of the original webapp which originally led me to this missing feature
  b) Hand-crafted test case HTML files specifically for this new feature, based on canvas calls generated directly from the example SVG files in the SVG spec

I thought it was sufficient, for now, to perform just these directed tests on Chromium on GNU/Linux on x86_64.

>> Source/WebCore/platform/graphics/GraphicsTypes.cpp:111
>> +    return names[rule];
> 
> I might have written this as an if (rule == NonZero) return "nonzero"; return "evenodd", to avoid any possible OOB, but this is also fine.

I wrote it like this to be consistent with the other similar functions in this file.

Sorry the patch does not apply cleanly any more, the base version was: https://github.com/jwal/webkit/blob/e8701c215ca234c43b119d3a3a1fe93f28deda34/Source/WebCore/platform/graphics/GraphicsTypes.cpp#L85

[James Ascroft-Leigh]

Update:

After some discussion on the mailing list making this a fill-call-time option instead of a state property is being given the consideration it deserves.  There is a counter proposal to this one:

https://bugs.webkit.org/show_bug.cgi?id=106188

And there is also a proposal to change Mozilla Firefox to match that too:

https://bugzilla.mozilla.org/show_bug.cgi?id=827053

I personally believe that setting the winding rule as an option on the fill() call is a better API and am glad to see we aren't rushing into implementing an API we are going to regret later.