Bug 231205 – Web Inspector: Show color space for canvases in the Graphics tab on the overview cards

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Patrick Angle]

<rdar://81828683>

[Patrick Angle]

Created attachment 440146 [details]
Patch v1.0

[Patrick Angle]

Created attachment 440148 [details]
Screenshot of Patch v1.0 /w Display P3 Color Space

[EWS Watchlist]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Patrick Angle]

Created attachment 440149 [details]
Screenshot of Patch v1.0 /w sRGB Color Space and no color space

[Devin Rousso]

Comment on attachment 440146 [details]
Patch v1.0

View in context: https://bugs.webkit.org/attachment.cgi?id=440146&action=review

r=me

On one hand, it's nice to expose info like this at a cursory glance as that can make finding the right `<canvas>` easier.  On the other hand, why are we picking this as the only thing to show of all the context attributes?  2D is also gonna get support for `alpha` soon™.  Should we also show that?  I personally think `colorSpace` is likely more useful than `alpha`, but that's just my experience and may not represent the way other developers use `<canvas>`.  I guess it just feels a bit odd that we'd pick one over the other, especially when most pages probably don't use more than a handful of `<canvas>` that likely can be distinguished based on the current content.

It's also odd that we don't have anything for WebGL either.  But that's an even more complex problem since there's more context attributes :/

I guess in summary I personally wouldn't add this as it's kinda "picking favorites" IMO, but I can see a use for it so I'll let you decide :)

> Source/JavaScriptCore/inspector/protocol/Canvas.json:21
> +            "enum": ["srgb", "display-p3" ]

Style: space before `]`

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:473
> +}

Style: missing `;`

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:111
> +                subtitle.textContent = `(${WI.Canvas.displayNameForColorSpace(this.representedObject.contextAttributes.colorSpace)})`;

NIT: I'd personally use `"(" + ... + ")"` as it's clearer to me that you're wrapping a string in parenthesis

[Patrick Angle]

(In reply to Devin Rousso from comment #5)
> Comment on attachment 440146 [details]
> Patch v1.0
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440146&action=review
> On one hand, it's nice to expose info like this at a cursory glance as that
> can make finding the right `<canvas>` easier.  On the other hand, why are we
> picking this as the only thing to show of all the context attributes?  2D is
> also gonna get support for `alpha` soon™.  Should we also show that?  I
> personally think `colorSpace` is likely more useful than `alpha`, but that's
> just my experience and may not represent the way other developers use
> `<canvas>`.  I guess it just feels a bit odd that we'd pick one over the
> other, especially when most pages probably don't use more than a handful of
> `<canvas>` that likely can be distinguished based on the current content.
> 
> It's also odd that we don't have anything for WebGL either.  But that's an
> even more complex problem since there's more context attributes :/
> 
> I guess in summary I personally wouldn't add this as it's kinda "picking
> favorites" IMO, but I can see a use for it so I'll let you decide :)

There is certainly some amount of "picking favorites" happening here, and we will never be able to show every attribute of a context on the overview card, but I feel Display P3 (and rich color in general) to be an important enough bit to surface here. Part of this comes down to awareness - it will benefit folks who didn't realize you could control the colorspace to see the current value on the overview, which will hopefully lead them to say "oh, colorspace is a thing for this context?" and consider for themselves whether their content and users would benefit from a wider color space.

[Devin Rousso]

(In reply to Patrick Angle from comment #6)
> (In reply to Devin Rousso from comment #5)
> > Comment on attachment 440146 [details]
> > Patch v1.0
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=440146&action=review
> > On one hand, it's nice to expose info like this at a cursory glance as that can make finding the right `<canvas>` easier.  On the other hand, why are we picking this as the only thing to show of all the context attributes?  2D is also gonna get support for `alpha` soon™.  Should we also show that?  I personally think `colorSpace` is likely more useful than `alpha`, but that's just my experience and may not represent the way other developers use `<canvas>`.  I guess it just feels a bit odd that we'd pick one over the other, especially when most pages probably don't use more than a handful of `<canvas>` that likely can be distinguished based on the current content.
> > 
> > It's also odd that we don't have anything for WebGL either.  But that's an even more complex problem since there's more context attributes :/
> > 
> > I guess in summary I personally wouldn't add this as it's kinda "picking favorites" IMO, but I can see a use for it so I'll let you decide :)
> 
> There is certainly some amount of "picking favorites" happening here, and we will never be able to show every attribute of a context on the overview card, but I feel Display P3 (and rich color in general) to be an important enough bit to surface here. Part of this comes down to awareness - it will benefit folks who didn't realize you could control the colorspace to see the current value on the overview, which will hopefully lead them to say "oh, colorspace is a thing for this context?" and consider for themselves whether their content and users would benefit from a wider color space.

you could make that same argument for many (if not all) of the context attributes tho 😅 (e.g. "oh hey I can make my context not have a transparent background!")

with (current) 2D controls I think yeah `colorSpace` is probably the most important (tho `alpha` could be _very_ useful for a wide variety of situations), but for WebGL I'm not as convinced (tho yeah `colorSpace` doesn't exist there, but might soon?)

[Patrick Angle]

Created attachment 440269 [details]
Patch v1.1

[Patrick Angle]

Comment on attachment 440269 [details]
Patch v1.1

I'd like to move forward with this for the time being - we should absolutely re-evaluate what extra info we show here as more attributes are supported, as WebGL color space support possibly becomes a thing, etc..

[EWS]

Committed r283572 (242536@main): <https://commits.webkit.org/242536@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440269 [details].