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

Pass-through options from DOM.context2d to getContext #273

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Dec 20, 2021

The third argument to DOM.context2d can be an options object with {scale, colorSpace}, allowing the creation of wide-gamut graphics on supported hardware and software.

Note: this necessitates a try/catch because Safari will throw if the os requirements are not met.

Tests/demo: https://observablehq.com/@fil/colorspace-display-p3-context2d

Draft, mostly for discussion.

@Fil Fil requested a review from mbostock December 20, 2021 14:58
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

We shouldn’t code for specific options and should instead support passthrough. If options is an object, we pass it through to canvas.getContext, and we don’t add any try-catch around it. For backwards compatibility, if options is a number, it’s promoted to an options object {scale}.

Although, I’m also tempted to do nothing here. For the most part the DOM API in standard library is deprecated. It’s better to just have people write against the standard APIs so that when code is copy-pasted outside of Observable there are no gratuitous dependencies on the standard library.

@Fil
Copy link
Contributor Author

Fil commented Dec 20, 2021

It's fine to just do nothing. Easy enough to make this helper outside of stlib for special cases. (But it's very useful in general to not have to worry about dpr.)

@Fil Fil changed the title colorSpace: "display-p3" Pass-through options from DOM.context2d to getContext Dec 20, 2021
Comment on lines 3 to 4
if (options == null) {
options = {};
Copy link
Member

Choose a reason for hiding this comment

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

It’d be better to be strict here and only promote options to an object when it is undefined, ideally using default arguments (options = {}). If the user passes in null explicitly, we should respect that and pass null to canvas.getContext.

if (options == +options) {
if (options == null) {
options = {};
} else if (options == +options) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be strict based on the type (typeof options === "number") rather than using coercion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to be fully compatible with the current implementation, I wanted to support dpr passed as a string. Not sure why anyone would do that, though :)

Copy link
Member

Choose a reason for hiding this comment

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

Full compatibility is not practical; someone could pass an object that implements valueOf, and this would have worked with the old API, but is ambiguous with named options under the new API. We should only support the intended pattern of passing a number.

scale = +options;
options = {};
Copy link
Member

Choose a reason for hiding this comment

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

This is probably fine, but it’d be slightly better to pass undefined, I think.

src/dom/context2d.js Outdated Show resolved Hide resolved
@Fil
Copy link
Contributor Author

Fil commented Oct 5, 2022

we might want to document the willReadFrequently boolean option as well?

…ale, colorSpace}, allowing the creation of wide-gamut graphics on supported hardware and software.

Note: this necessitates a try/catch because Safari will throw if the os requirements are not met.

Tests/demo: https://observablehq.com/@fil/colorspace-display-p3-context2d
@mootari
Copy link
Member

mootari commented Oct 5, 2022

I believe the options handling can be simplified:

export default function(width, height, options = {}) {
  const {
    scale = devicePixelRatio,
    ...contextOptions
  } = !isNaN(options) ? {scale: options} : options;
  const canvas = document.createElement("canvas");
  canvas.width = width * scale;
  canvas.height = height * scale;
  canvas.style.width = width + "px";
  const context = canvas.getContext("2d", contextOptions);
  context.scale(scale, scale);
  return context;
}

@mootari mootari self-requested a review October 5, 2022 16:06
@Fil Fil requested a review from mbostock October 5, 2022 16:08
@Fil Fil marked this pull request as ready for review October 5, 2022 16:09
Comment on lines +2 to +4
const {scale = devicePixelRatio, ...contextOptions} = !isNaN(options)
? {...(options != null && {scale: options})}
: options;
Copy link
Member

@mootari mootari Oct 5, 2022

Choose a reason for hiding this comment

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

Suggested change
const {scale = devicePixelRatio, ...contextOptions} = !isNaN(options)
? {...(options != null && {scale: options})}
: options;
const {scale = devicePixelRatio, ...contextOptions} = options && !isNaN(options)
? {scale: +options}
: options;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0 is currently working as before (in the sense that it returns a canvas of size 0)—it would be simpler if we didn't support this "feature".

Copy link
Member

Choose a reason for hiding this comment

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

With the additional null check in the earlier comment we can remove options && here.

Copy link
Member

Choose a reason for hiding this comment

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

(btw, that parseFloat was the result of a brainfart - I meant to write +options)

src/dom/context2d.mjs Outdated Show resolved Hide resolved
@@ -41,9 +41,9 @@ html`<canvas width=960 height=500>`

If you are using [2D Canvas](https://www.w3.org/TR/2dcontext/) (rather than [WebGL](https://webglfundamentals.org/)), you should use [DOM.context2d](#DOM_context2d) instead of DOM.canvas for automatic pixel density scaling.

<a href="#DOM_context2d" name="DOM_context2d">#</a> DOM.<b>context2d</b>(<i>width</i>, <i>height</i>[, <i>dpi</i>]) [<>](https://github.com/observablehq/stdlib/blob/main/src/dom/context2d.mjs "Source")
<a href="#DOM_context2d" name="DOM_context2d">#</a> DOM.<b>context2d</b>(<i>width</i>, <i>height</i>[, <i>options</i>]) [<>](https://github.com/observablehq/stdlib/blob/main/src/dom/context2d.mjs "Source")
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we list both signatures here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's there: "As a shorthand notation, options having only scale can be specified as a number." (could be phrased better?)

Copy link
Member

Choose a reason for hiding this comment

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

I feel the shorthand should be mentioned earlier and emphasized, since it's the format that you'll encounter throughout Observable and the notation that you'll commonly use to disable dpr scaling.

canvas.height = height * dpi;
export default function (width, height, options = {}) {
const {scale = devicePixelRatio, ...contextOptions} = !isNaN(options)
? {...(options != null && {scale: options})}
Copy link
Member

@mootari mootari Oct 5, 2022

Choose a reason for hiding this comment

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

There's no need for the destructuring here, and the null check should be explicit. If options is undefined, we already get {}.

Suggested change
? {...(options != null && {scale: options})}
? options === null ? {} : {scale: options}

@Fil
Copy link
Contributor Author

Fil commented Mar 23, 2023

Chrome (>= 111) supports display-p3
https://developer.chrome.com/articles/high-definition-css-color-guide/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants