support evenodd fill rule

[wsw0108]

fix #384

[LinusU]

Do you think that you could add in a small test as well? I would appreciate it a lot, otherwise I'll try and do it myself. It could just draw some black and white shape that would cause a specific pixel to be black when using "nonzero" and white when using "evenodd", or vice versa.

Also, do you think that it makes sense to merge this now, and later add support to clip and isPointInPath, or should we add that right away?

[wsw0108]

Well, both clip and isPointInPath has an optional parameter 'fillRule'.

I will trying to implement them with some tests then.

[LinusU]

Thank you!

[wsw0108]

@LinusU done

[LinusU]

If I'm not misstaken, the clip and isPointInPath will retain the previous value of the fill rule if called first with evenodd and then without it.

What do you think about always calling out setFillRule which instead of cairo_fill_rule_t would take a v8::Handle<v8::Value>. That value could then be checked against ->IsUndefined() in which case it would set the rule to CAIRO_FILL_RULE_WINDING, otherwise if ->IsString() it would get the string and check it for valid fill strategies.

This function could then simply be called in all three functions with e.g. info[2]. No ->Length() check would be needed since it should return a v8::Undefined value if the index is out of bounds.

I think that this approach would be very nice 👍

[wsw0108]

1. I am not sure about that, need to check it
2. According to the syntax of clip:

void ctx.clip();
void ctx.clip(fillRule);
void ctx.clip(path, fillRule);

The index of fillRule can be 0 or 1, so info[0] may be the value of parameter path

[wsw0108]

The changes to setFillRule are better.

[wsw0108]

@LinusU From https://html.spec.whatwg.org/multipage/scripting.html#drawing-paths-to-the-canvas, I can't find the description about below:

If I'm not misstaken, the clip and isPointInPath will retain the previous value of the fill rule if called first with evenodd and then without it.

Could you help to verify?

[LinusU]

I don't think you need cairo_save and cairo_restore here?

[LinusU]

I meant that the code currently did that, and that it was a bug :)

I still think is the case though, could you add a test that tries doing first with evenodd, and then without a rule...

[wsw0108]

So for code following:

ctx.rect(0, 0, 100, 50);
ctx.rect(50, 0, 50, 100);
ctx.fill('evenodd');
console.log(ctx.isPointInPath(70, 10, 'evenodd')); // 1
console.log(ctx.isPointInPath(70, 10)); // 2

Do you mean that

• 1: false
• 2: false

But I test it at https://codepen.io/anon/pen/PNMLqe, the result is

• 1: false
• 2: true

[LinusU]

I would like it to be 1: false, 2: true, but I believe that with the current code in this pull request it will be 1: false, 2: false

[wsw0108]

Test added.

I have consider that isPointInPath() is just a test method, so it should not affect the cairo context.
Then the cairo_save()/cairo_restore() should be used to build the cairo context for this testing.

And if I remove cairo_save()/cairo_restore(), the test updated will fail.

[LinusU]

Then you need to add cairo_save() and cairo_restore() to the other functions as well, but I think it's cleaner and more performant to just always set fill rule, that way you won't need to deal with saving the context.

If you make a similar test for e.g. fill you will see that that is currently broken as well...

[wsw0108]

@LinusU You are right.
Now it just set fill rule, and the test cases are update to follow the spec.

[LinusU]

Looks awesome 🙌 I will merge when I get home

[LinusU]

Landed manually, sorry for the delay, thank you for your contribution 🙌

[LinusU]

faae57a v1.4.0