Summary: | Web Inspector: Display color swatches for p3 colors | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||||||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, webkit-bug-importer | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 203436 | ||||||||||||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2019-10-25 16:46:28 PDT
Created attachment 381980 [details]
Patch
Page with P3 colors: https://codepen.io/NV/pen/zVeewL?editors=1100 Created attachment 381981 [details]
[Image] With patch applied
Comment on attachment 381980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381980&action=review r-, we should have tests for this. You can add them to 'inspector/model/color.html'. > Source/WebInspectorUI/UserInterface/Models/Color.js:51 > + const matchRegExp = /^(?:#(?<hex>[0-9a-f]{3,8})|rgba?\((?<rgb>[^)]+)\)|(?<keyword>\w+)|color\(display-p3\s(?<p3>[^)]+)\)|hsla?\((?<hsl>[^)]+)\))$/i; It looks like we also support `srgb`. We should make this more general to support all the color spaces of the `color(...)` function. > Source/WebInspectorUI/UserInterface/Models/Color.js:584 > + return `color(display-p3 ${r} ${g} ${b} ${alpha})`; There should be a `/` before the `alpha`. ``` return `color(display-p3 ${r} ${g} ${b} / ${alpha})`; ``` Created attachment 382384 [details]
Patch
Comment on attachment 382384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382384&action=review > LayoutTests/inspector/model/color.html:240 > + testGood("color(display-p3 999 999 999/0.5)", WI.Color.Format.Function); Shouldn't the alpha for this also be `999`? > LayoutTests/inspector/model/color.html:245 > + testGood("color(display-p3 999 999 999 / 0.5)", WI.Color.Format.Function); Ditto > LayoutTests/inspector/model/color.html:250 > + testGood("color(display-p3 999 999 999/33%)", WI.Color.Format.Function); Ditto > LayoutTests/inspector/model/color.html:255 > + testGood("color(display-p3 999 999 999 / 33%)", WI.Color.Format.Function); Ditto > LayoutTests/inspector/model/color.html:324 > + InspectorTest.expectThat(color.alpha === 1, "the default alpha should be 1."); These aren't good descriptions. We should at least include the word "p3" in there somewhere (or better yet the entire color string) like "P3 color should have a default alpha of 1" (or "'color(display-p3 0 1 0.5)' should have a default alpha of 1"). > Source/WebInspectorUI/UserInterface/Models/Color.js:32 > + constructor(format, components, gamut = "sRGB") Please move this to be a fallback on the declaration. ``` this.gamut = gamut || "srgb"; ``` > Source/WebInspectorUI/UserInterface/Models/Color.js:177 > + let alpha = 1; I think you can simplify this: ``` let components = splitFunctionString(colorString); if (components.length !== 4 && components.length !== 5) return null; let gamut = components[0].toLowerCase(); if (gamut !== "srgb" && gamut !== "display-p3") return null; let alpha = 1; if (components.length === 5) alpha = parseFunctionAlpha(components[4]); function parseFunctionComponent(component) { let value = parseFloat(component); return Number.constrain(value, 0, 1); } return new WI.Color(WI.Color.Format.Function, [ parseFunctionComponent(components[1]), parseFunctionComponent(components[2]), parseFunctionComponent(components[3]), alpha, ], gamut); ``` > Source/WebInspectorUI/UserInterface/Models/Color.js:448 > + if (this.gamut.toLowerCase() !== "srgb") Why not just make it so that `WI.Color.fromString` calls `toLowerCase` so that we don't need to keep checking it? > Source/WebInspectorUI/UserInterface/Models/Color.js:590 > + let [r, g, b, alpha] = this._rgba; We should still call `this.rgba` just in case this is the first time we're getting the components. Comment on attachment 382384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382384&action=review >> Source/WebInspectorUI/UserInterface/Models/Color.js:32 >> + constructor(format, components, gamut = "sRGB") > > Please move this to be a fallback on the declaration. > ``` > this.gamut = gamut || "srgb"; > ``` Why? Having the default argument makes it immediately clear that `gamut` is optional. >> Source/WebInspectorUI/UserInterface/Models/Color.js:177 >> + let alpha = 1; > > I think you can simplify this: > ``` > let components = splitFunctionString(colorString); > if (components.length !== 4 && components.length !== 5) > return null; > > let gamut = components[0].toLowerCase(); > if (gamut !== "srgb" && gamut !== "display-p3") > return null; > > let alpha = 1; > if (components.length === 5) > alpha = parseFunctionAlpha(components[4]); > > function parseFunctionComponent(component) { > let value = parseFloat(component); > return Number.constrain(value, 0, 1); > } > > return new WI.Color(WI.Color.Format.Function, [ > parseFunctionComponent(components[1]), > parseFunctionComponent(components[2]), > parseFunctionComponent(components[3]), > alpha, > ], gamut); > ``` This would parse `color(display-p3 0 1 1 0.5)` as a valid color, but it isn't. It doesn't include `/` before the alpha. >> Source/WebInspectorUI/UserInterface/Models/Color.js:448 >> + if (this.gamut.toLowerCase() !== "srgb") > > Why not just make it so that `WI.Color.fromString` calls `toLowerCase` so that we don't need to keep checking it? Sounds good. >> Source/WebInspectorUI/UserInterface/Models/Color.js:590 >> + let [r, g, b, alpha] = this._rgba; > > We should still call `this.rgba` just in case this is the first time we're getting the components. It must always have `_rgba`. get rgba() { if (!this._rgba) this._rgba = this._hslaToRGBA(this._hsla); return this._rgba; } The color function doesn't support HSLA. Created attachment 382429 [details]
Patch
(In reply to Nikita Vasilyev from comment #8) > This would parse `color(display-p3 0 1 1 0.5)` as a valid color, but it > isn't. It doesn't include `/` before the alpha. On the other hand, we currently detect `rgb(255 / 255 / 0)` as a color, even though it isn't a valid CSS color. I can go either way here. Created attachment 382484 [details]
Patch
(In reply to Nikita Vasilyev from comment #10) > (In reply to Nikita Vasilyev from comment #8) > > This would parse `color(display-p3 0 1 1 0.5)` as a valid color, but it isn't. It doesn't include `/` before the alpha. > On the other hand, we currently detect `rgb(255 / 255 / 0)` as a color, even though it isn't a valid CSS color. I can go either way here. Given that we already support invalid (but close to being valid) colors, I think it's fine to do something similar here. Perhaps we should fix both in a followup. Comment on attachment 382384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382384&action=review >>> Source/WebInspectorUI/UserInterface/Models/Color.js:32 >>> + constructor(format, components, gamut = "sRGB") >> >> Please move this to be a fallback on the declaration. >> ``` >> this.gamut = gamut || "srgb"; >> ``` > > Why? Having the default argument makes it immediately clear that `gamut` is optional. This is just not our style. One problem with optional parameters is that passing _anything_ will override it, including falsy values like `null` or `undefined`. We've preferred the fallback approach for this reason. >>> Source/WebInspectorUI/UserInterface/Models/Color.js:590 >>> + let [r, g, b, alpha] = this._rgba; >> >> We should still call `this.rgba` just in case this is the first time we're getting the components. > > It must always have `_rgba`. > > get rgba() > { > if (!this._rgba) > this._rgba = this._hslaToRGBA(this._hsla); > return this._rgba; > } > > The color function doesn't support HSLA. I realize that this is the case now, but in the future we'd probably want to support switching formats from something like HSLA, in which case we may not have an `_rgba` yet. Also, `get rgba` would be a very quick return in that case, so I think we could use it purely on defensive grounds. Created attachment 382517 [details]
Patch
Comment on attachment 382517 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382517&action=review r=me, nice tests! Please create a followup bug for supporting `WI.Color.prototype.nextFormat` with `WI.Color.Format.ColorFunction` and add it as a FIXME inside the function's body. Otherwise, right now, we'd fail an assertion and log an error if a user tries to shift-click on a `color(...)` swatch. > Source/WebInspectorUI/UserInterface/Models/Color.js:35 > + this.gamut = gamut || "srgb"; We should pass in `this.gamut` when creating new `WI.Color` inside `WI.Color.prototype.copy` as well. > Source/WebInspectorUI/UserInterface/Models/Color.js:591 > + Style: unnecessary newline > Source/WebInspectorUI/UserInterface/Models/Color.js:648 > + Function: "color-format-function", Rather than just `Function`, can we use something more specific like `ColorFunction`? Given that rgb/rgba/hsl/hsla are also functions, the more general name could be confusing. > Source/WebInspectorUI/UserInterface/Models/Color.js:654 > + "color", NIT: is there a reason this is after "rgba" instead of just at the end as a new value? Regardless, we should try to be consistent with the ordering in `WI.Color.Format` (and all the `switch`). Created attachment 382594 [details]
Patch
Comment on attachment 382594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382594&action=review > Source/WebInspectorUI/UserInterface/Models/Color.js:399 > case WI.Color.Format.RGBA: Don't we need a case for `WI.Color.Format.ColorFunction` here too? Created attachment 382596 [details]
Patch
Comment on attachment 382596 [details] Patch Clearing flags on attachment: 382596 Committed r251933: <https://trac.webkit.org/changeset/251933> All reviewed patches have been landed. Closing bug. |