Bug 148310 – Web Inspector: Visual sidebar should support multiple backgrounds

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

[Timothy Hatcher]

The background section assumes one image right now. The background image items should be a section like the box shadow or transitions.

[Radar WebKit Bug Importer]

<rdar://problem/22380794>

[Timothy Hatcher]

Not to mention, it only makes sense to set offset, width, height, etc when an image is supplied.

[Devin Rousso]

Just so I understand correctly, you are suggesting that CSV support be added to background?  If so, then I agree and will add a CSV editor (just like transition and box-shadow) for background and list all the editors underneath as options for each comma-separated background.

[Devin Rousso]

Created attachment 261095 [details]
Patch

[Blaze Burg]

Comment on attachment 261095 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/VisualStyleCommaSeparatedKeywordEditor.js:126
> +        let propertyValues = {};

I think it's fine to use an array of arrays (maybe called valueLists?) since you don't care about the keys after insertion, nor do you care about uniqueness.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleCommaSeparatedKeywordEditor.js:127
> +        let totalValues = 0;

counters should end with *Count. So, valuesCount.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleCommaSeparatedKeywordEditor.js:133
> +            let text = (existingProperty && existingProperty.value) || this._longhandProperties[property];

I feel like this fallback behavior should be a getter or helper method.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleCommaSeparatedKeywordEditor.js:136
> +                totalValues = propertyValues[property].length;

Use Math.max()

> Source/WebInspectorUI/UserInterface/Views/VisualStyleCommaSeparatedKeywordEditor.js:142
> +        let count = 0;

You can replace all this code with:

text = valueLists.map((list) => list.join(" ")).join(",");

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:788
> +        properties.background.element.style.marginTop = "3px";

Why is this specified inline?

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:820
> +        function noRemainingTreeItems() {

Why are these event listeners all inline? We don't do this anywhere else.

[Timothy Hatcher]

Screenshot?

[Devin Rousso]

Created attachment 261827 [details]
After patch is applied

I was also using inline functions because I couldn't see a way to make the listeners member functions without saving a whole bunch more variables to the class object.  I think I may have a way of getting it to work now.

[Devin Rousso]

Created attachment 261830 [details]
Patch

(In reply to comment #5)
> Comment on attachment 261095 [details]
> > Source/WebInspectorUI/UserInterface/Views/VisualStyleCommaSeparatedKeywordEditor.js:142
> > +        let count = 0;
> 
> You can replace all this code with:
> 
> text = valueLists.map((list) => list.join(" ")).join(",");

This did not work for me.  I'm actually not trying to concatenate each list together, but spread out the values (one from each) to generate the comma-separated value of background from the longhand properties.

[Blaze Burg]

Clearing r? since this patch doesn't apply any more. It is pretty close to done, though.

[Devin Rousso]

Created attachment 263760 [details]
Patch

[Timothy Hatcher]

Comment on attachment 263760 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/VisualStyleURLInput.js:63
> +        return "url(" + value + ")";

The background value can be anything, not just a url(). It can be a var() now or a gradient, etc. You should add a FIXME and file a follow up bug about that.

[Devin Rousso]

Comment on attachment 263760 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleURLInput.js:63
>> +        return "url(" + value + ")";
> 
> The background value can be anything, not just a url(). It can be a var() now or a gradient, etc. You should add a FIXME and file a follow up bug about that.

WebInspector.VisualStyleURLInput is only ever used for the background-image property.  As far as I can tell, the only values that can be used for background-image are "initial", "inherit", "none", and "url(<A URL>)".  Adding support for var() will actually require a huge change to most of the editors as they don't have support for that.

[Timothy Hatcher]

While thinking about it, it might be good to add special support for gradients here. We have a gradient editor, like the color picker. You could use that.

Maybe when you add a background, it lets you pick from "Image" or "Gradient". Then you will get the option to show the gradient editor instead of the textfield for a URL.

[Devin Rousso]

(In reply to comment #13)
> While thinking about it, it might be good to add special support for
> gradients here. We have a gradient editor, like the color picker. You could
> use that.
> 
> Maybe when you add a background, it lets you pick from "Image" or
> "Gradient". Then you will get the option to show the gradient editor instead
> of the textfield for a URL.

Oh good idea.  I'll add those.  I do think, however, that this patch is already large enough, so I would prefer to just make that into another patch if thats OK.

[WebKit Commit Bot]

Comment on attachment 263760 [details]
Patch

Rejecting attachment 263760 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 263760, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
mothy Hatcher']" exit_code: 255 cwd: /Volumes/Data/EWS/WebKit

Parsed 13 diffs from patch file(s).
patching file Source/WebInspectorUI/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
Original content of Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js mismatches at /Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply line 283.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Timothy Hatcher']" exit_code: 255 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/324067

[Devin Rousso]

Created attachment 263908 [details]
Patch

Rebased

[WebKit Commit Bot]

Comment on attachment 263908 [details]
Patch

Rejecting attachment 263908 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 263908, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
file without the binary data in line: "Binary files a/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js and b/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js differ".  Be sure to use the --binary flag when invoking "git diff" with diffs containing binary files. at /Volumes/Data/EWS/WebKit/Tools/Scripts/VCSUtils.pm line 804, <ARGV> line 82.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 25 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/325972

[Devin Rousso]

Created attachment 263911 [details]
Patch

Forgot --binary in diff

[WebKit Commit Bot]

Comment on attachment 263911 [details]
Patch

Clearing flags on attachment: 263911

Committed r191493: <http://trac.webkit.org/changeset/191493>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.