Bug 147570 – Web Inspector: Add VisualStyleDetailsPanel

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Create Visual style details panel for the CSS style sidebar.

[Devin Rousso]

Created attachment 258086 [details]
Patch

[Devin Rousso]

Created attachment 258199 [details]
Patch

[Devin Rousso]

Created attachment 258329 [details]
Patch

[Devin Rousso]

Created attachment 258458 [details]
Patch

[Devin Rousso]

Created attachment 258620 [details]
Patch

[Timothy Hatcher]

Comment on attachment 258620 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:48
> +        this._keywords.defaults = ["Inherit", "Initial"];
> +        this._keywords.boxModel = this._keywords.defaults.concat(["Auto"]);
> +        this._keywords.borderStyle = {
> +            basic: this._keywords.defaults.concat(["None", "Hidden", "Solid"]),
> +            advanced: ["Dashed", "Dotted", "Double", "Groove", "Hidden", "Inset", "None", "Outset", "Ridge", "Solid"]
> +        };
> +        this._keywords.borderWidth = this._keywords.defaults.concat(["Medium", "Thick", "Thin"]);

Would be good to comment that these are not localized because they match the CSS spec.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:183
> +        let initialTextList = this._currentStyle[WebInspector.VisualStyleDetailsPanel.InitialPropertySectionTextListSymbol];

A getter for this might be good since you repeat it a lot and it is rather long.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:294
> +        properties.display = new WebInspector.VisualStyleKeywordPicker("display", WebInspector.UIString("Type"), {
> +            basic: ["None", "Block", "Flex", "Inline", "Inline Block"],
> +            advanced: ["Compact", "Inline Flex", "Inline Table", "List Item", "Table", "Table Caption", "Table Cell", "Table Column", "Table Column Group", "Table Footer Group", "Table Header Group", "Table Row", "Table Row Group", " WAP Marquee", " Webkit Box", " Webkit Grid", " Webkit Inline Box", " Webkit Inline Grid"]
> +        });
> +        properties.visibility = new WebInspector.VisualStyleKeywordPicker("visibility", WebInspector.UIString("Visibility"), {
> +            basic: ["Hidden", "Visible"],
> +            advanced: ["Collapse"]
> +        });

Ditto comment about localization.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:305
> +        properties.boxSizing = new WebInspector.VisualStyleKeywordPicker("box-sizing", WebInspector.UIString("Sizing"), this._keywords.defaults.concat(["Border Box", "Content Box"]));
> +        properties.cursor = new WebInspector.VisualStyleKeywordPicker("cursor", WebInspector.UIString("Cursor"), {
> +            basic: ["Auto", "Default", "None", "Pointer", "Crosshair", "Text"],
> +            advanced: ["Context Menu", "Help", "Progress", "Wait", "Cell", "Vertical Text", "Alias", "Copy", "Move", "No Drop", "Not Allowed", "All Scroll", "Col Resize", "Row Resize", "N Resize", "E Resize", "S Resize", "W Resize", "NS Resize", "EW Resize", "NE Resize", "NW Resize", "SE Resize", "sW Resize", "NESW Resize", "NWSE Resize"]
> +        });

Ditto.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:316
> +        properties.opacity = new WebInspector.VisualStyleUnitSlider("opacity", WebInspector.UIString("Opacity"));
> +        properties.overflow = new WebInspector.VisualStyleKeywordPicker(["overflow-x", "overflow-y"], WebInspector.UIString("Overflow"), {
> +            basic: ["Initial", "Auto", "Hidden", "Scroll", "Visible"],
> +            advanced: ["Marquee", "Overlay", " Webkit Paged X", " Webkit Paged Y"]
> +        });

Ditto. Webkit should be WebKit. Though I am tempted to display prefixed things as "Paged Y (WebKit)", etc.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:374
> +            basic: ["Static", "Relative", "Absolute", "Fixed"],
> +            advanced: [" Webkit Sticky"]

Why a space prefix? Ditto about localization and Webkit.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:1039
> +WebInspector.VisualStyleDetailsPanel.propertyReferenceInfo = {

We should remove this until we can clear it internally. It will need attribution to MDN and "Mozilla Contributors". https://developer.mozilla.org/en-US/docs/MDN/About#Copyrights_and_licenses

[Devin Rousso]

Comment on attachment 258620 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:374
>> +            advanced: [" Webkit Sticky"]
> 
> Why a space prefix? Ditto about localization and Webkit.

This is sort of hacky, but the property editor superclass replaces all spaces with "-" so in order to get a leading dash, I added a space (which won't show up in an option element by the way).

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:1039
>> +WebInspector.VisualStyleDetailsPanel.propertyReferenceInfo = {
> 
> We should remove this until we can clear it internally. It will need attribution to MDN and "Mozilla Contributors". https://developer.mozilla.org/en-US/docs/MDN/About#Copyrights_and_licenses

Oh good point.  Is there a process that I have to go through to do that?

[Timothy Hatcher]

Comment on attachment 258620 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:1039
>>> +WebInspector.VisualStyleDetailsPanel.propertyReferenceInfo = {
>> 
>> We should remove this until we can clear it internally. It will need attribution to MDN and "Mozilla Contributors". https://developer.mozilla.org/en-US/docs/MDN/About#Copyrights_and_licenses
> 
> Oh good point.  Is there a process that I have to go through to do that?

I'll follow up with in person about this. Lets just land it with the object empty for now.

[Radar WebKit Bug Importer]

<rdar://problem/22281743>

[Timothy Hatcher]

Comment on attachment 258620 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:374
>>> +            advanced: [" Webkit Sticky"]
>> 
>> Why a space prefix? Ditto about localization and Webkit.
> 
> This is sort of hacky, but the property editor superclass replaces all spaces with "-" so in order to get a leading dash, I added a space (which won't show up in an option element by the way).

I see. Hmm, why not just check for a WebKit prefix and then add the prefix "-"?

[Devin Rousso]

> >>> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:374
> >>> +            advanced: [" Webkit Sticky"]
> >> 
> >> Why a space prefix? Ditto about localization and Webkit.
> > 
> > This is sort of hacky, but the property editor superclass replaces all spaces with "-" so in order to get a leading dash, I added a space (which won't show up in an option element by the way).
> 
> I see. Hmm, why not just check for a WebKit prefix and then add the prefix
> "-"?

I felt that adding a check for the "WebKit" prefix would be less future-proof, not to mention making that work with localizations would not be much fun.  Since <option>s don't render prefix/suffix whitespace, I thought that this was fine.

[Devin Rousso]

Created attachment 258990 [details]
Patch

[Timothy Hatcher]

Comment on attachment 258990 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.css:37
> +    -webkit-filter: opacity(0.7);

No prefix. Just use opacity: 0.7;

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.css:55
> +    -webkit-filter: opacity(0.5);

Ditto.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:382
> +        properties.zIndex = new WebInspector.VisualStyleNumberInputBox("z-index", WebInspector.UIString("z-Index"), this._keywords.boxModel, null, true);

Z-Index?

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:421
> +                // COMPATIBILITY (iOS 6): Order of parameters changed in iOS 7.
> +                DOMAgent.highlightNode.invoke({nodeId: this._style.node.id, highlightConfig});

Use DOMTreeManager for these agent calls in this function.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:1045
> +WebInspector.VisualStyleDetailsPanel.propertyReferenceInfo = {};

Add FIXME a comment on why this is empty and what it is for.

[Devin Rousso]

Created attachment 259020 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 259020 [details]
Patch

Rejecting attachment 259020 [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', 259020, '--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 792, <ARGV> line 91.

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/59475

[Devin Rousso]

Created attachment 259050 [details]
Patch

Whoops.  Forgot the --binary.

[WebKit Commit Bot]

Comment on attachment 259050 [details]
Patch

Clearing flags on attachment: 259050

Committed r188503: <http://trac.webkit.org/changeset/188503>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.