Last Comment Bug 655926 - Add support for even-odd-rule filling and clipping
: Add support for even-odd-rule filling and clipping
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-09 19:44 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2013-01-03 15:39 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement canvas.mozFillRule (4.39 KB, patch)
2011-06-03 16:56 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
vladimir: review+
vladimir: superreview+
Details | Diff | Splinter Review
part 2: Test even-odd fill rule (6.67 KB, patch)
2011-06-09 00:28 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
joe: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-09 19:44:35 PDT
Andreas and I are hacking on a project that needs this.  It's pretty trivial for our cairo <canvas> backend; we just need to |cairo_set_fill_rule (cairo, CAIRO_FILL_RULE_EVEN_ODD);| when requested.  I'm not sure what the right API is; how about

  context.fillRule = "nonzero" | "evenodd".  Default is "nonzero".  fillRule is saved and restored in the same way as fillStyle.

(Probably needs to be |mozFillRule| initially.)

Does Azure support even-odd-rule fills yet?
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-09 20:35:21 PDT
Bas tells me that Azure doesn't support even-odd-rule fills (because canvas doesn't use it, hah), but it would trivial to add.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-09 20:50:51 PDT
Bas suggests "winding" instead of "nonzero", and indeed that's what the cairo constant is called too (CAIRO_FILL_RULE_WINDING).
Comment 3 Philip Taylor 2011-05-10 07:32:36 PDT
http://www.w3.org/TR/SVG/painting.html#FillRuleProperty uses "nonzero" and "evenodd", and consistency with web APIs seems more useful than consistency with Cairo.

How would this affect clip(), and isPointInPath(), and self-overlapping strokes?
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-10 08:20:07 PDT
(In reply to comment #3)
> http://www.w3.org/TR/SVG/painting.html#FillRuleProperty uses "nonzero" and
> "evenodd", and consistency with web APIs seems more useful than consistency
> with Cairo.

Agreed.  Thanks, I had forgotten about that.

> How would this affect clip(), and isPointInPath(),

These would be subject to fillRule.

> and self-overlapping strokes?

You mean whether the fillRule might cause them not to be stroked?  I don't know; I don't think it should.  http://dev.w3.org/html5/2dcontext/#dom-context-2d-stroke seems to imply it doesn't.  Does SVG specify anything?
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-03 16:56:59 PDT
Created attachment 537275 [details] [diff] [review]
Implement canvas.mozFillRule

(Vlad, are you still around doing sr's?)

Pretty trivial patch.  Tests and spec forthcoming, if interested parties agree on this impl.
Comment 6 Vladimir Vukicevic [:vlad] [:vladv] 2011-06-03 18:01:38 PDT
Comment on attachment 537275 [details] [diff] [review]
Implement canvas.mozFillRule

Looks good to me
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-09 00:28:34 PDT
Created attachment 538202 [details] [diff] [review]
part 2: Test even-odd fill rule

Simple tests, can expand as bugs arise.
Comment 8 Joe Drew (not getting mail) 2011-06-09 11:49:59 PDT
Comment on attachment 538202 [details] [diff] [review]
part 2: Test even-odd fill rule

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

There's a bunch of boilerplate (defining PI, assert, etc) in the test html files that can be removed if you want.

::: layout/reftests/canvas/evenodd-fill-sanity.html
@@ +14,5 @@
> +        assert("evenodd" == ctx.mozFillRule,
> +               "fillRule understands 'evenodd'");
> +        ctx.mozFillRule = "nonzero";
> +
> +        ctx.mozFillRule = "garbageLSKJDF 29879234";

You should probably move the '= "nonzero"' to just above the garbage.

@@ +23,5 @@
> +        assert("evenodd" == ctx.mozFillRule,
> +               "Garbage fillRule string has no effect");
> +        ctx.mozFillRule = "nonzero";
> +
> +        ctx.save();

Same here.
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-09 12:45:28 PDT
(In reply to comment #8)
> Comment on attachment 538202 [details] [diff] [review] [review]
> part 2: Test even-odd fill rule
> 
> Review of attachment 538202 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> There's a bunch of boilerplate (defining PI, assert, etc) in the test html
> files that can be removed if you want.

Done.  (The PI garbage was from a failed first attempt that used circles instead of rects.)

> 
> ::: layout/reftests/canvas/evenodd-fill-sanity.html
> @@ +14,5 @@
> > +        assert("evenodd" == ctx.mozFillRule,
> > +               "fillRule understands 'evenodd'");
> > +        ctx.mozFillRule = "nonzero";
> > +
> > +        ctx.mozFillRule = "garbageLSKJDF 29879234";
> 
> You should probably move the '= "nonzero"' to just above the garbage.
> 
> @@ +23,5 @@
> > +        assert("evenodd" == ctx.mozFillRule,
> > +               "Garbage fillRule string has no effect");
> > +        ctx.mozFillRule = "nonzero";
> > +
> > +        ctx.save();
> 
> Same here.

Didn't change, per IRC chat.
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-09 19:18:23 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/5f63b1c2b73a
http://hg.mozilla.org/integration/mozilla-inbound/rev/2c19e587bc8c

This will be dev-doc-needed when it lands on m-c and we finish spec'ing on whatwg.
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-10 08:18:42 PDT
Let's wait for docs until this spec is looking solid on whatwg (I'll post today).  Should be quick, this is a simple change.  I'd rather that devs don't use this until then, TBH.
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-15 19:45:46 PDT
No objections on whatwg.  (No comments at all, actually ...)
Comment 14 Philip Taylor 2011-06-16 09:04:12 PDT
Since when it was first proposed on the whatwg list, the only response I can find was <http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2009-April/019503.html>. You can probably ping Hixie on IRC or email if you want an immediate response from him instead of waiting months/years. (The content of the proposal seems straightforward and uncontroversial to me, so I wouldn't be surprised if it doesn't generate much discussion on the list.)
Comment 15 Hixie (not reading bugmail) 2011-07-25 11:36:02 PDT
Since this is now implemented, speccing it should be non-controversial. (The only reason I am delaying canvas features is that they all get implemented almost immediately, which means they have a high opportunity cost. This is a non-issue once they are already implemented!)
Comment 16 Eric Shepherd [:sheppy] 2011-08-09 08:10:26 PDT
Documentation updated:

https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D#Attributes

Added to Firefox 7 for developers.
Comment 17 Hixie (not reading bugmail) 2013-01-03 15:39:29 PST
This is now in the spec.
http://whatwg.org/html/#dom-context-2d-fillrule

Note You need to log in before you can comment on or make changes to this bug.