Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add versions to CanvasRenderingContext2D for features in CanvasPath mixin #6393

Merged
merged 4 commits into from
Jul 17, 2020

Conversation

queengooborg
Copy link
Collaborator

@queengooborg queengooborg commented Jul 13, 2020

A number of features in the CanvasRenderingContext2D API are a part of a mixin known as CanvasPath. As described in the spec, this mixin is also included in the Path2D interface.

This PR splits the functions of the CanvasPath mixin into its own file, and adds the missing version numbers for when support was added based upon manual testing (checking to see if the function is defined on CanvasRenderingContext2D), as well as fixing some instances where support was indicated as false for certain browsers which is inaccurate (Firefox Android, Safari iOS, WebView...).

The new file also references MDN pages that have not been created, so they will need to be written up as well.

@ghost ghost added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Jul 13, 2020
@jpmedley
Copy link
Collaborator

There's no way to withdraw approvals.

Shouldn't these methods be listed on the APIs that use the mixin? A mixin is just a convenience to save browsers from duplicating information in multiple places. Not only is there (as far as I know) no requirement for vendors to use these, duplicating this on MDN forces web developers to understand what's in browsers' source code. Such information is not available on all browsers.

@jpmedley
Copy link
Collaborator

@Elchi3

@Elchi3
Copy link
Member

Elchi3 commented Jul 14, 2020

(I guess this PR is filed to force us to have the mixin debate?)


I'm actually the original author of the Canvas API docs and my take on the mixin debate for canvas has been to not expose these to MDN readers when I wrote these docs. I haven't looked into this API in a long time, but as far as I know CanvasPath is a spec IDL mixin to share functionality between CanvasRenderingContext2D and OffscreenCanvasRenderingContext2D.

So, with this PR, you would need to present the information as

  • CanvasPath.rect.canvas2dcontext.__compat
  • CanvasPath.rect.offscreencontext.__compat
  • CanvasPath.ellipse.canvas2dcontext.__compat
  • CanvasPath.ellipse.offscreencontext.__compat

Or maybe as:

  • CanvasPath.canvas2dcontext.
    • rect.__compat
    • ellipse.__compat
  • CanvasPath.offscreencontext.
    • rect.__compat
    • ellipse.__compat

And the MDN pages at https://developer.mozilla.org/docs/Web/API/CanvasPath/rect and https://developer.mozilla.org/docs/Web/API/CanvasPath/ellipse will need to talk about the Canvas2D context and the OffScreenCanvas context. It will need to have 2 spec links also for both contexts, it will need to have 2 examples for each context, etc.

So, if we would merge this PR, there is quite some follow up work to be done on MDN. Who would do this work?


The alternative, and this is the road I was going with canvas docs, is to flatten the spec mixins. That is, ignore that idl files have mixins like CanvasPath, CanvasText, and friends and document the methods and properties under the interface that is exposed to web developers, which in this case are CanvasRenderingContext2D and OffscreenCanvasRenderingContext2D.

In BCD, this would mean:

  • CanvasRenderingContext2D.rect.__compat
  • OffscreenCanvasRenderingContext2D.rect.__compat
  • CanvasRenderingContext2D.ellipse.__compat
  • OffscreenCanvasRenderingContext2D.ellipse.__compat

On MDN, this would mean:


Which way to go here has been argued about at random times in the last years. I prefer more to flatten and not expose mixins on MDN and to web devs. This means creating more MDN pages, but I think it is worth it. Other people would prefer to not flatten and present mixins to web devs and then also have less pages on MDN. An argument is that information is duplicated, so you have e.g. two quite similar pages that talked about rect. I actually think the pages would be different enough that's it's worth having both.

So given this new PR, I'm curious if you have any new insights that would favor mixins in BCD and MDN?
Also, if you prefer mixins, are you planning to use the mixins used by the spec? What if browsers use different mixins? What if the spec decides to re-organize mixins?


See also

@queengooborg
Copy link
Collaborator Author

That all makes sense, yeah -- I had a few of those questions when I was working on this PR myself. To summarize, we should probably undo the separation, and instead just update the version numbers on CanvasRenderingContext2D and duplicate it onto CanvasRenderingContext2D and Path2D?

@jpmedley
Copy link
Collaborator

(I guess this PR is filed to force us to have the mixin debate?)

No. It just worked out that way. 🙂

@Elchi3
Copy link
Member

Elchi3 commented Jul 15, 2020

To summarize, we should probably undo the separation

Yes.

Update the version numbers on CanvasRenderingContext2D

Yes.

duplicate it onto CanvasRenderingContext2D and Path2D?

No. Members of Path2D likely have different compat versions.

@queengooborg queengooborg changed the title Add CanvasPath mixin API Add versions to CanvasRenderingContext2D for features in CanvasPath mixin Jul 15, 2020
api/CanvasRenderingContext2D.json Outdated Show resolved Hide resolved
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Vinyl! 👍

@Elchi3 Elchi3 merged commit e4b9484 into mdn:master Jul 17, 2020
@queengooborg queengooborg deleted the api/CanvasPath branch July 17, 2020 08:34
@jpmedley
Copy link
Collaborator

duplicate it onto CanvasRenderingContext2D and Path2D?

No. Members of Path2D likely have different compat versions.

It might be better to just say, don't assume either way. Sometimes interfaces with mixins are created together. Sometimes the mixin isn't created until it's recognized that some feature set needs to be on multiple interfaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants