Bug 178404

Summary: Web Inspector: [PARITY] Styles Redesign: Add color gradient, bezier curve, and spring inline widgets
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, mattbaker, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=178636
Bug Depends on: 178354    
Bug Blocks:    
Attachments:
Description Flags
Patch
hi: review-
Patch
none
Patch
hi: review+
Patch none

Nikita Vasilyev
Reported 2017-10-17 13:00:15 PDT
Add the following inline widgets: - Gradients, e.g. `background-image: linear-gradient(yellow, orange)`; - Bezier curves, e.g. `transition-timing-function: cubic-bezier(0.68, -0.55, 0.265, 1.55)`; - Spring functions; - Variables, e.g. `var(--toolbar-color)`.
Attachments
Patch (9.23 KB, patch)
2017-10-17 17:01 PDT, Nikita Vasilyev
hi: review-
Patch (9.22 KB, patch)
2017-10-20 12:43 PDT, Nikita Vasilyev
no flags
Patch (9.36 KB, patch)
2017-10-21 15:42 PDT, Nikita Vasilyev
hi: review+
Patch (9.60 KB, patch)
2017-10-25 13:37 PDT, Nikita Vasilyev
no flags
Radar WebKit Bug Importer
Comment 1 2017-10-17 13:00:48 PDT
Nikita Vasilyev
Comment 2 2017-10-17 17:01:12 PDT
Created attachment 324071 [details] Patch Static page for manual testing: http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/color.html This patch adds gradients, bezier curves, and spring functions. CSS variables are a little different, I'll add them in a follow up patch.
Devin Rousso
Comment 3 2017-10-18 12:02:38 PDT
Comment on attachment 324071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324071&action=review r- for the issue with nested var() > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:338 > + let text = rawTokens.map((token) => token.value).join(""); > + > + let gradient = WI.Gradient.fromString(text); Style: unnecessary newline. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:403 > + } else if (!isNaN(cubicBezierStartIndex) && token.value === ")") { Simply checking for ")" is not valid, as it is possible to nest `var()` inside `cubic-bezier(...)`. You will need to keep track of the matching "(" and ")" and only perform this logic once they are both even. Gradient already has this logic. This will also be the case for Color and Spring. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:422 > + _addSpringTokens(tokens) The logic in `_addSpringTokens` is almost identical to `_addCubicBezierTokens`, with the exception of: (429): token.value === "spring" (437): let spring = WI.Spring.fromString(text); (439): newTokens.push(this._createInlineSwatch(WI.InlineSwatch.Type.Spring, text, spring)); Considering that CubicBezier and Spring are both used for the same CSS properties, can you combine them and avoid this logic?
Matt Baker
Comment 4 2017-10-18 12:14:40 PDT
Comment on attachment 324071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324071&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:295 > + let value = event.data && event.data.value && event.data.value.toString(); InlineSwatch always includes event data when the value changes, so that check can be removed. I think it's also safe to assume that event.data implies event.data.toString(). With the exception of Color.prototype.toString (which can throw), all the InlineSwatch value objects (Color, CubicBezier, Spring, and the Gradient subclasses) will always return a string. It's not as concise, but you could rewrite this as: if (!event.data.value) return; try { let value = event.data.value.toString(); console.assert(value, "Expected value string.", event.data.value); if (!value) return; this._handleValueChange(); } catch (e) { console.error("Could not convert value to string.", e); } It's weird that we throw an exception here, but since we do it should be handled. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:321 > + // Gradient start Drop this comment. The variable name `gradientStartIndex` makes it clear. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:327 > + // Gradient end This one too. Also, this isn't necessarily the end of the gradient, as the comment on line 331 indicates. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:331 > + // Color function inside of a gradient. Since this could also be a var(), I'd make this comment more general: "// Matched a CSS function inside of the gradient." > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:401 > + // Cubic-bezier start Remove comment. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:404 > + // Cubic-bezier end Ditto. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:430 > + // Spring start Ditto. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:433 > + // Spring end Ditto.
Nikita Vasilyev
Comment 5 2017-10-19 11:56:59 PDT
Comment on attachment 324071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324071&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:403 >> + } else if (!isNaN(cubicBezierStartIndex) && token.value === ")") { > > Simply checking for ")" is not valid, as it is possible to nest `var()` inside `cubic-bezier(...)`. You will need to keep track of the matching "(" and ")" and only perform this logic once they are both even. Gradient already has this logic. > > This will also be the case for Color and Spring. Good catch! The old styles sidebar didn't detect `linear-gradient(#fff, var(--myBlue))` as a gradient, but I intent to fix it in the new one.
Nikita Vasilyev
Comment 6 2017-10-19 15:07:58 PDT
(In reply to Nikita Vasilyev from comment #5) > Comment on attachment 324071 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=324071&action=review > > >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:403 > >> + } else if (!isNaN(cubicBezierStartIndex) && token.value === ")") { > > > > Simply checking for ")" is not valid, as it is possible to nest `var()` inside `cubic-bezier(...)`. You will need to keep track of the matching "(" and ")" and only perform this logic once they are both even. Gradient already has this logic. > > > > This will also be the case for Color and Spring. > > Good catch! > > The old styles sidebar didn't detect `linear-gradient(#fff, var(--myBlue))` > as a gradient, but I intent to fix it in the new one. This case is tricky: linear-gradient(#fff, var(--myBlue)) This is one case when a text range of one widget, var(--myBlue), is inside of a text range of another widget, linear-gradient(...). At very least, when after modifying #fff using the gradient widget, the variable widget for var(--myBlue) should still work. When modifying var(--myBlue) using the gradient widget, should it replace var(--myBlue) with a new value, or should it update the variable? I'm not going to address these issue here, but it is something to think in the future.
Nikita Vasilyev
Comment 7 2017-10-19 17:28:19 PDT
(In reply to Nikita Vasilyev from comment #5) > Comment on attachment 324071 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=324071&action=review > > >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:403 > >> + } else if (!isNaN(cubicBezierStartIndex) && token.value === ")") { > > > > Simply checking for ")" is not valid, as it is possible to nest `var()` inside `cubic-bezier(...)`. You will need to keep track of the matching "(" and ")" and only perform this logic once they are both even. Gradient already has this logic. > > > > This will also be the case for Color and Spring. > > Good catch! > > The old styles sidebar didn't detect `linear-gradient(#fff, var(--myBlue))` > as a gradient, but I intent to fix it in the new one. None of this function can parse CSS variables, calc() or min/max() correctly: WI.CubicBezier.fromString WI.Spring.fromString WI.Color.fromString WI.Gradient.fromString E.g.: -> WI.Gradient.fromString("linear-gradient(#fff2db, var(--myVar))") <- null This seems non-trivial to fix and it isn't required for parity with the old styles sidebar, so I'm not going to do it in this task.
Nikita Vasilyev
Comment 8 2017-10-20 12:43:09 PDT
Created attachment 324430 [details] Patch (In reply to Matt Baker from comment #4) > Comment on attachment 324071 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=324071&action=review > > > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:295 > > + let value = event.data && event.data.value && event.data.value.toString(); > > InlineSwatch always includes event data when the value changes, so that > check can be removed. I think it's also safe to assume that event.data > implies event.data.toString(). With the exception of > Color.prototype.toString (which can throw), all the InlineSwatch value > objects (Color, CubicBezier, Spring, and the Gradient subclasses) will > always return a string. > > It's not as concise, but you could rewrite this as: > > if (!event.data.value) > return; > > try { > let value = event.data.value.toString(); > console.assert(value, "Expected value string.", event.data.value); > if (!value) > return; > this._handleValueChange(); > } catch (e) { > console.error("Could not convert value to string.", e); > } The old styled sidebar didn't have try/catch, see CSSStyleDeclarationTextEditor.propotype._inlineSwatchValueChanged: let value = event.data && event.data.value && event.data.value.toString(); We rarely use try/catch and it seems unnecessary here. I think it's better to replace it with console.error.
Devin Rousso
Comment 9 2017-10-20 21:45:57 PDT
Comment on attachment 324071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324071&action=review >>>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:403 >>>>> + } else if (!isNaN(cubicBezierStartIndex) && token.value === ")") { >>>> >>>> Simply checking for ")" is not valid, as it is possible to nest `var()` inside `cubic-bezier(...)`. You will need to keep track of the matching "(" and ")" and only perform this logic once they are both even. Gradient already has this logic. >>>> >>>> This will also be the case for Color and Spring. >>> >>> Good catch! >>> >>> The old styles sidebar didn't detect `linear-gradient(#fff, var(--myBlue))` as a gradient, but I intent to fix it in the new one. >> >> This case is tricky: >> >> linear-gradient(#fff, var(--myBlue)) >> >> This is one case when a text range of one widget, var(--myBlue), is inside of a text range of another widget, linear-gradient(...). At very least, when after modifying #fff using the gradient widget, the variable widget for var(--myBlue) should still work. >> >> When modifying var(--myBlue) using the gradient widget, should it replace var(--myBlue) with a new value, or should it update the variable? >> >> I'm not going to address these issue here, but it is something to think in the future. > > None of this function can parse CSS variables, calc() or min/max() correctly: > > WI.CubicBezier.fromString > WI.Spring.fromString > WI.Color.fromString > WI.Gradient.fromString > > E.g.: > > -> WI.Gradient.fromString("linear-gradient(#fff2db, var(--myVar))") > <- null > > This seems non-trivial to fix and it isn't required for parity with the old styles sidebar, > so I'm not going to do it in this task. That is a good point. Deriving and/or modifying the value of a variable would not be simple, especially if the variable has multiple overrides. I would think that in this case the `var(...)` would just get replaced with the resulting value, instead of changing the variable (the user could do that in the variable's declaration), but we could always make it work the other way. calc/min/max would probably be easier, but not as much if they involved multiple units. I think that we can just add a FIXME and follow up later.
Nikita Vasilyev
Comment 10 2017-10-21 15:42:06 PDT
Created attachment 324506 [details] Patch Added FIXME with Bug 178636 - Web Inspector: Styles: Make inline widgets work with CSS functions (var(), calc(), etc.)
Devin Rousso
Comment 11 2017-10-24 14:43:24 PDT
Comment on attachment 324506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324506&action=review r=me > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:339 > + newTokens.push(...tokens); It seems incorrect to push all of the tokens to newTokens, as we are already using some of them in the next else-if (343). Should we instead be using `rawTokens`? newTokens.push(...rawTokens); > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:358 > newTokens.push(...tokens); Ditto (339). > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:425 > + newTokens.push(...tokens); Ditto (339).
Nikita Vasilyev
Comment 12 2017-10-25 13:37:04 PDT
Created attachment 324883 [details] Patch (In reply to Devin Rousso from comment #11) > Comment on attachment 324506 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=324506&action=review > > r=me > > > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:339 > > + newTokens.push(...tokens); > > It seems incorrect to push all of the tokens to newTokens, as we are already > using some of them in the next else-if (343). Should we instead be using > `rawTokens`? Whoops, good catch!
WebKit Commit Bot
Comment 13 2017-10-25 14:09:48 PDT
Comment on attachment 324883 [details] Patch Clearing flags on attachment: 324883 Committed r223978: <https://trac.webkit.org/changeset/223978>
WebKit Commit Bot
Comment 14 2017-10-25 14:09:50 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.