Bug 148310

Summary: Web Inspector: Visual sidebar should support multiple backgrounds
Product: WebKit Reporter: Timothy Hatcher <timothy>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, graouts, hi, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 147563    
Attachments:
Description Flags
Patch
none
After patch is applied
none
Patch
none
Patch
timothy: review+, commit-queue: commit-queue-
Patch
commit-queue: commit-queue-
Patch none

Description Timothy Hatcher 2015-08-21 10:22:37 PDT
The background section assumes one image right now. The background image items should be a section like the box shadow or transitions.
Comment 1 Radar WebKit Bug Importer 2015-08-21 10:23:00 PDT
<rdar://problem/22380794>
Comment 2 Timothy Hatcher 2015-08-21 10:28:14 PDT
Not to mention, it only makes sense to set offset, width, height, etc when an image is supplied.
Comment 3 Devin Rousso 2015-09-11 22:49:09 PDT
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.
Comment 4 Devin Rousso 2015-09-13 20:38:13 PDT
Created attachment 261095 [details]
Patch
Comment 5 BJ Burg 2015-09-15 11:15:19 PDT
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.
Comment 6 Timothy Hatcher 2015-09-17 13:35:55 PDT
Screenshot?
Comment 7 Devin Rousso 2015-09-23 10:12:49 PDT
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.
Comment 8 Devin Rousso 2015-09-23 10:49:34 PDT
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.
Comment 9 BJ Burg 2015-10-18 15:55:24 PDT
Clearing r? since this patch doesn't apply any more. It is pretty close to done, though.
Comment 10 Devin Rousso 2015-10-21 16:50:38 PDT
Created attachment 263760 [details]
Patch
Comment 11 Timothy Hatcher 2015-10-22 08:07:33 PDT
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.
Comment 12 Devin Rousso 2015-10-22 09:42:52 PDT
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.
Comment 13 Timothy Hatcher 2015-10-22 13:10:16 PDT
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.
Comment 14 Devin Rousso 2015-10-22 13:15:32 PDT
(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.
Comment 15 WebKit Commit Bot 2015-10-22 17:18:10 PDT
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
Comment 16 Devin Rousso 2015-10-22 23:35:31 PDT
Created attachment 263908 [details]
Patch

Rebased
Comment 17 WebKit Commit Bot 2015-10-22 23:37:05 PDT
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
Comment 18 Devin Rousso 2015-10-23 00:25:44 PDT
Created attachment 263911 [details]
Patch

Forgot --binary in diff
Comment 19 WebKit Commit Bot 2015-10-23 01:11:07 PDT
Comment on attachment 263911 [details]
Patch

Clearing flags on attachment: 263911

Committed r191493: <http://trac.webkit.org/changeset/191493>
Comment 20 WebKit Commit Bot 2015-10-23 01:11:11 PDT
All reviewed patches have been landed.  Closing bug.