Summary: | Web Inspector: Visual sidebar should support multiple backgrounds | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Timothy Hatcher <timothy> | ||||||||||||||
Component: | Web Inspector | Assignee: | 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
Timothy Hatcher
2015-08-21 10:22:37 PDT
Not to mention, it only makes sense to set offset, width, height, etc when an image is supplied. 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. Created attachment 261095 [details]
Patch
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. Screenshot? 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.
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. Clearing r? since this patch doesn't apply any more. It is pretty close to done, though. Created attachment 263760 [details]
Patch
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 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. 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. (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 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 Created attachment 263908 [details]
Patch
Rebased
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 Created attachment 263911 [details]
Patch
Forgot --binary in diff
Comment on attachment 263911 [details] Patch Clearing flags on attachment: 263911 Committed r191493: <http://trac.webkit.org/changeset/191493> All reviewed patches have been landed. Closing bug. |