Skip to content

Replace alpha flag with a PaintRenderingContext2DSettings object #448

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.

Sign up for GitHub

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

Merged
bfgeek merged 7 commits into from
Sep 8, 2017
+28 −17
Merged

Replace alpha flag with a PaintRenderingContext2DSettings object #448

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6c7ce5d
added contextAttributes
xidachen Aug 17, 2017
df0c249
Changed to ContextAttributes
xidachen Aug 18, 2017
f7796c4
replace one more place
xidachen Aug 18, 2017
66a64a6
address comments
xidachen Aug 21, 2017
2a24fd7
change to PaintRenderingContext2DSettings
xidachen Aug 30, 2017
fe4d70d
address comments
xidachen Sep 6, 2017
e4ba80c
rename function to contextOptions
xidachen Sep 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 21 additions & 12 deletions css-paint-api/Overview.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,13 +138,22 @@ interface PaintWorkletGlobalScope : WorkletGlobalScope {
};
</pre>

The {{ContextAttributes}} allows the settings for the rendering context associated
with the paint canvas. Right now, the {{ContextAttributes}} has a boolean flag
indicating if the paint canvas contains an alpha channel.
<pre class='idl'>
dictionary ContextAttributes {
Copy link
Copy Markdown
Contributor

@bfgeek bfgeek Aug 18, 2017

Choose a reason for hiding this comment

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

might be nice to namespace this for the spec e.g. {{PaintContextAttributes}}, but doesn't really matter as not user visible.

Copy link
Copy Markdown
Contributor Author

@xidachen xidachen Aug 21, 2017

Choose a reason for hiding this comment

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

I am not very certain about this. I notice that there is a proposal for CanvasRenderingContext2DSettings in here: https://github.com/WICG/canvas-color-space/blob/master/CanvasColorSpaceProposal.md

Currently there is only one alpha, but the proposal make it including color space attributes. Maybe in the future we should make ContextAttributes inherit from it? Or maybe just directly using it for PaintRenderingContext2D?

Copy link
Copy Markdown
Contributor

@bfgeek bfgeek Aug 21, 2017

Choose a reason for hiding this comment

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

@flackr thoughts?

Copy link
Copy Markdown
Contributor

@flackr flackr Aug 25, 2017

Choose a reason for hiding this comment

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

It makes sense that we might eventually want to support settings like color space. However, in the event that we do this I think we might still only want to support a subset of CanvasRenderingContext2DSettings just like PaintRenderingContext2D implements a subset of CanvasRenderingContext2D. How about PaintRenderingContext2DSettings for consistency?

Copy link
Copy Markdown
Contributor Author

@xidachen xidachen Aug 25, 2017

Choose a reason for hiding this comment

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

Yes, I'd support that. PaintRenderingContext2DSettings is a better name, because it is more clear that it is used for PaintRenderingContext2D.
Question: do we need to start a discussion somewhere and bring other people onboard, like CSS discussion group?

Copy link
Copy Markdown
Contributor

@bfgeek bfgeek Aug 21, 2017

Choose a reason for hiding this comment

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

PaintContextAttributes to match below.

boolean alpha = true;
};
</pre>

<div class='note'>
Note: The shape of the class should be:
<pre class='lang-javascript'>
class MyPaint {
static get inputProperties() { return ['--foo']; }
static get inputArguments() { return ['&lt;color>']; }
static get alpha() { return true; }
static get ContextAttributes() { return {alpha: true}; }
Copy link
Copy Markdown
Contributor

@bfgeek bfgeek Aug 18, 2017

Choose a reason for hiding this comment

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

I think should be lowerCamelCase, "contextAttributes"


paint(ctx, size, styleMap) {
// Paint code goes here.
Expand All @@ -169,7 +178,7 @@ A <dfn>paint definition</dfn> is a <a>struct</a> which describes the information
- <dfn for="paint definition">input properties</dfn> which is a <a>list</a> of
<code>DOMStrings</code>.

- A <dfn for="paint definition">context alpha flag</dfn>.
- A <dfn for="paint definition">ContextAttributes object</dfn>.

A <dfn>document paint definition</dfn> is a <a>struct</a> which describes the information
needed by the <a>document</a> about the author defined <<image>> function (which can be referenced
Expand All @@ -181,7 +190,7 @@ by the paint function). It consists of:
- A <dfn for="document paint definition">input argument syntaxes</dfn> which is a <a>list</a> of
parsed [[css-properties-values-api-1#supported-syntax-strings]].

- A <dfn for="document paint definition">context alpha flag</dfn>.
- A <dfn for="document paint definition">ContextAttributes object</dfn>.

Registering Custom Paint {#registering-custom-paint}
====================================================
Expand Down Expand Up @@ -244,15 +253,15 @@ called, the user agent <em>must</em> run the following steps:

2. <a for=list>Append</a> |parsedSyntax| to |inputArgumentSyntaxes|.

12. Let |alphaValue| be the result of <a>Get</a>(|paintCtor|, "alpha").
12. Let |alphaValue| be the result of <a>Get</a>(|paintCtor|, "ContextAttributes.alpha").
Copy link
Copy Markdown
Contributor

@bfgeek bfgeek Aug 18, 2017

Choose a reason for hiding this comment

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

This should be...

  1. Let |contextAttributesValue| be the result of Get(|paintCtor|, "contextAttributes").

  2. Let |contextAttributes| be the result of converting |contextAttributesValue| to a {{PaintContextAttributes}}, .... etc etc etc


13. Let |alpha| be <code>true</code> if |alphaValue| is undefined, otherwise let it be the
result of <a>converting</a> |alphaValue| to a <a>boolean</a>. If an exception is
<a>thrown</a>, rethrow the exception and abort all these steps.

Note: Setting <code>alpha</code> is <code>false</code> allows user agents to anti-alias text
an addition to performing "visibility" optimizations, e.g. not painting an image behind
the paint image as the paint image is opaque.
Note: Setting <code>ContextAttributes.alpha</code> is <code>false</code> allows user agents
to anti-alias text in addition to performing "visibility" optimizations, e.g. not
painting an image behind the paint image as the paint image is opaque.

14. If the result of <a>IsConstructor</a>(|paintCtor|) is false, <a>throw</a> a <a>TypeError</a>
and abort all these steps.
Expand All @@ -277,7 +286,7 @@ called, the user agent <em>must</em> run the following steps:

- <a for="paint definition">input properties</a> being |inputProperties|.

- <a for="paint definition">context alpha flag</a> being |alpha|.
- <a for="paint definition">ContextAttributes object</a> being {alpha: |alpha|}.
Copy link
Copy Markdown
Contributor

@bfgeek bfgeek Aug 18, 2017

Choose a reason for hiding this comment

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

... being |contextAttributes|.


20. <a for=map>Set</a> |paintDefinitionMap|[|name|] to |definition|.

Expand All @@ -293,7 +302,7 @@ called, the user agent <em>must</em> run the following steps:
- <a for="document paint definition">input argument syntaxes</a> being
|inputArgumentSyntaxes|.

- <a for="document paint definition">context alpha flag</a> being |alpha|.
- <a for="document paint definition">ContextAttributes object</a> being {alpha: |alpha|}.
Copy link
Copy Markdown
Contributor

@bfgeek bfgeek Aug 18, 2017

Choose a reason for hiding this comment

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

as above.


3. If |documentPaintDefinitionMap|[|name|] <a for=map>exists</a>, run the following steps:

Expand All @@ -305,13 +314,13 @@ called, the user agent <em>must</em> run the following steps:
3. If |existingDocumentDefinition| and |documentDefinition| are not equivalent, (that is
<a for="document paint definition">input properties</a>, <a for="document paint
definition">input argument syntaxes</a>, and <a for="document paint
definition">context alpha flag</a> are different), then:
definition">ContextAttributes object</a> are different), then:

<a for=map>Set</a> |documentPaintDefinitionMap|[|name|] to <code>"invalid"</code>.

Log an error to the debugging console stating that the same class was registered
with different <code>inputProperties</code>, <code>inputArguments</code>, or
<code>alpha</code>.
<code>ContextAttributes</code>.

4. Otherwise, <a for=map>set</a> |documentPaintDefinitionMap|[|name|] to
|documentDefinition|.
Expand Down Expand Up @@ -605,7 +614,7 @@ When the user agent wants to <dfn>invoke a paint callback</dfn> given |name|, |i
8. Let |renderingContext| be the result of <a>create a PaintRenderingContext2D object</a> given:
Copy link
Copy Markdown
Contributor

@bfgeek bfgeek Aug 21, 2017

Choose a reason for hiding this comment

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

has the "create a PaintRenderingContext2D object" algorithm been updated to receive a contextAttributes object?

Copy link
Copy Markdown
Contributor Author

@xidachen xidachen Aug 30, 2017

Choose a reason for hiding this comment

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

Good catch, I have updated that section.

- "width" - The width given by |concreteObjectSize|.
- "height" - The height given by |concreteObjectSize|.
- "alpha" - The <a for="paint definition">context alpha flag</a> given by |definition|.
- "context_attributes" - The <a for="paint definition">ContextAttributes object</a> given by |definition|.

Note: The |renderingContext| is not be re-used between invocations of paint. Implicitly this
means that there is no stored data, or state on the |renderingContext| between
Expand Down