-
Notifications
You must be signed in to change notification settings - Fork 440
Add more specific types for globalCompositeOperation #1206
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
Conversation
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
"hue", | ||
"saturation", | ||
"color", | ||
"luminosity" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, the spec actually defines more items when including <blend-mode>
. Checking implementations...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec is confusing for this property. The extra enum items come from <blend-mode>
but that also includes normal
which browsers don't seem to support and which MDN doesn't list. I didn't include normal
even though the spec technically would, I think that's in line with TypeScript's philosophy? Either way, the supported value source-over
is what most people would assume normal
does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gecko: https://searchfox.org/mozilla-central/rev/9a5f36b0ddb9cb8ae556fc5b45f8ccea0f0da6f8/dom/canvas/CanvasRenderingContext2D.cpp#4756-4782
WebKit: https://webkit-search.igalia.com/webkit/rev/13bfcede837af469200178969bd5c08bc0bc4518/Source/WebCore/platform/graphics/GraphicsTypes.cpp#38-74
Blink: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/graphics_types.cc;l=34-51;drc=de68be3f18ba99cc01d75903e167ca09bade253c
Seems all the values there are supported!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder there should be two separate enums BlendMode
and CompositeMode
but for now, lgtm.
lgtm, @github-actions? |
Merging because @saschanaz is a code-owner of all the changes - thanks! |
Add types for
CanvasRenderingContext2D.globalCompositeOperation
, which is currently astring
but should only accept certain values. The acceptable values come from MDN.Reported as a bug in TypeScript's main repo: microsoft/TypeScript#34673