Bug 119686 - Web Inspector: rich editing of CSS gradients
Summary: Web Inspector: rich editing of CSS gradients
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on: 129088 129089 129094
Blocks: 129095 129096
  Show dependency treegraph
 
Reported: 2013-08-12 08:49 PDT by Antoine Quint
Modified: 2014-02-20 14:05 PST (History)
6 users (show)

See Also:


Attachments
Patch (80.29 KB, patch)
2014-02-20 11:44 PST, Antoine Quint
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2013-08-12 08:49:01 PDT
In order to alleviate the complexity of the CSS gradients syntax and make it more visually efficient to edit them, we should offer rich editing of gradients within Web Inspector, for instance in the form of a popover allowing to edit the gradient type, direction and stops.
Comment 1 Radar WebKit Bug Importer 2013-08-12 08:50:23 PDT
<rdar://problem/14712396>
Comment 2 Antoine Quint 2013-08-12 09:11:41 PDT
I think that to do this right, we'll need backend support to do the parsing of the CSS value, which we should be able to do via CSSParser::parseGeneratedImage() and checking if the passed value is of type CSSGradientValue.
Comment 3 Dean Jackson 2013-08-12 13:53:38 PDT
(In reply to comment #2)
> I think that to do this right, we'll need backend support to do the parsing of the CSS value, which we should be able to do via CSSParser::parseGeneratedImage() and checking if the passed value is of type CSSGradientValue.

Note that CSSParser::parse*Gradient take CSSParserValueLists rather than strings so you'll have to treat this like a new stylesheet. And the output is designed to go onto a RenderStyle.
Comment 4 Antoine Quint 2013-10-01 05:06:34 PDT
Another option is to do the parsing in JS, there is existing work in that area, for instance Daniel Glazman's JSCSSP has support for parsing gradients: http://sources.disruptive-innovations.com/jscssp/trunk/src/parser/gradients.js.
Comment 5 Timothy Hatcher 2013-10-01 14:30:54 PDT
I think having our WebKit parser sense. But at the same time, if you use a newer Web Inspector with an older device you will be locked into the old syntax. Having a JS parser lets the Inspector handle new syntaxes that the device might not support but are new and valid.
Comment 6 Timothy Hatcher 2013-10-01 14:37:30 PDT
I bungled that last message.

"I think having our WebKit parser makes some sense."

"Having a JS CSS parser lets…"
Comment 7 Antoine Quint 2014-02-19 03:30:34 PST
Work on this feature is being done on my own WebKit fork at https://github.com/graouts/webkit/tree/gradients. It's mostly done, save for integration in the CSSStylesDeclarationTextEditor.
Comment 8 Antoine Quint 2014-02-20 03:43:00 PST
The blocking bugs are:

https://bugs.webkit.org/show_bug.cgi?id=129088
https://bugs.webkit.org/show_bug.cgi?id=129089
https://bugs.webkit.org/show_bug.cgi?id=129094

The good news is that they all have patches out for review, and the patch for this bug is ready to be sent out as soon as the blocking bugs have landed.
Comment 9 Antoine Quint 2014-02-20 04:14:34 PST
Some followup tasks:

http://webkit.org/b/129095
http://webkit.org/b/129096
Comment 10 Antoine Quint 2014-02-20 11:44:08 PST
Created attachment 224777 [details]
Patch
Comment 11 Timothy Hatcher 2014-02-20 12:43:37 PST
Comment on attachment 224777 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/CodeMirrorGradientEditingController.js:177
> +        var matches = this._angleInput.value.match(/º/g);

Should unicode escape the symbol.

> Source/WebInspectorUI/UserInterface/CodeMirrorGradientEditingController.js:180
> +            this._angleInput.value = angle + "º";

Ditto.

> Source/WebInspectorUI/UserInterface/CodeMirrorGradientEditingController.js:192
> +                this._angleInput.value = "180º";

Ditto.

> Source/WebInspectorUI/UserInterface/GradientSlider.css:103
> +    border: 1px solid rgba(0,0,0,0.25);

Spaces after the commas.

> Source/WebInspectorUI/UserInterface/GradientSlider.js:357
> +        if (selected)
> +            this._element.classList.add(WebInspector.GradientSliderKnob.SelectedClassName);
> +        else
> +            this._element.classList.remove(WebInspector.GradientSliderKnob.SelectedClassName);

This can be one line: this._element.classList.toggle(WebInspector.GradientSliderKnob.SelectedClassName, selected);

> Source/WebInspectorUI/UserInterface/Images/Checkers.svg:2
> +<svg xmlns="http://www.w3.org/2000/svg" width="6" height="6" fill="rgb(204, 204, 204)"><rect width="3" height="3" /><rect x="3" y="3" width="3" height="3"/></svg>
>  \ No newline at end of file

This should get the license header comment and have newlines.
Comment 12 Antoine Quint 2014-02-20 14:04:41 PST
Landed r164446 addressing Tim's feedback: <http://trac.webkit.org/changeset/164446>