Bug 18224 – Adding new CSS style properties to HTML nodes using Web Inspector

• Component: Web Inspector (Deprecated)
• Status: RESOLVED WORKSFORME

[Markus Knaup]

Like in Firebug there is currently no function to add new CSS style properties to classes of a node or adding inline styles within the style side bar.

[Markus Knaup]

Created attachment 20216 [details]
Patch for adding new CSS style properties in the Web Inspector style side bar

I was not able to write tests for the changes, please send some info about writing tests for Web Inspector. I could not find useful tutorials.

[Matt Lilek]

We do not currently have support for testing the inspector so one isn't needed.  This patch uses tabs in many places - those will have to be removed before it can be landed (pending a positive review, of course). 

[Markus Knaup]

Created attachment 20219 [details]
Tabs were removed.

I removed the tabs, sorry about that.

[Matt Lilek]

Comment on attachment 20216 [details]
Patch for adding new CSS style properties in the Web Inspector style side bar

Clearing review flag on obsolete patch

[Adam Roben (:aroben)]

Created attachment 20292 [details]
Screenshot with attachment 20219 [details] applied

For those who are curious (like me)

[Adam Roben (:aroben)]

Comment on attachment 20219 [details]
Tabs were removed.

Thanks for the patch! I'm impressed by how closely it matches the style of the existing Inspector code. A couple of comments:

You ChangeLog should explain the changes you made to each function.

+    this.addNewPropertyElement.title = "Add property";

All strings displayed in the Inspector's UI need to be obtained through the WebInspector.UIString function. You'll have to add your new string to WebCore/English.lproj/InspectorLocalizedStrings.js and call UIString here.

Your changes in PropertiesSection.js break the "Properties" pane. PropertiesSection is used by both the Styles pane and the Properties pane. Your changes should move to StylePropertiesSection in StylesSidebarPane.js

I'm confused by the name of the "this.addNewInlineStyle" property added to StylesSidebarPane. The name is confusing because it's a verb phrase, so to me it sounds more like a function name.

I actually think this.addNewInlineStyle doesn't need to be a property at all, and can just be another optional argument to the update function, called something like "alwaysShowInlineStyle".

The "newInlineStyle" function should probably be called "addNewInlineStyle", since that's a verb phrase. I also think it should be bound to the StylesSidebarPane object, instead of to the newInlineStyleElement. So your event listener would change to this.addNewInlineStyle.bind(this).

 220                 if (editable && !(section.styleRule.parentStyleSheet && !section.styleRule.parentStyleSheet.ownerNode && !section.styleRule.parentStyleSheet.href))

It think we need to move "parentStyleSheet && !ownerNode && !href" into a helper function so we're not duplicating the logic so much in this file.

I think it would be better for the new property to start out completely blank, instead of starting out as ": ;". I just find that confusing and it gets in the way.

584          parseElement.setAttribute("style", userInput);
 627         parseElement.setAttribute("style", userInput.replace(/;$/g, "").trimWhitespace() + ";");

What's the purpose of this change? Again, things like this should be mentioned in the ChangeLog.

 690     isPropertyEmpty: function(propertyString)
 691     {
 692         if (!propertyString.match(/([a-zA-Z]+)/g)) {
 693             if (this.treeOutline.section && this.treeOutline.section.pane)
 694                 this.treeOutline.section.pane.update();
 695             this.parent.removeChild(this);
 696         }

"isPropertyEmpty" is not a good name for a function that removes an empty property. Something like "removeIfEmpty" would be better. I'm also not sure what the purpose of the regular expression is. And why is this needed? As far as I can tell we already remove properties if they are empty when you finish editing.

 2204 .section.expanded .hidden {
 2205     display: none;
 2206 }
21812207 \ No newline at end of file

You should put a newline at the end of this file.

I know it sounds like a lot, but this patch is in really good shape! Thanks again!

[Markus Knaup]

Thanks for your input! I will upload a new patch in a few days.

[Zach Brock]

It appear the back end work to support this already exists, there is just no button to make it easy to do.

It is currently possible to do this if you select an existing rule, add a semi-colon to the end, insert a new rule and hit 'enter'.

For example, if inspecting an element, you see
height: 10px;
as the only style rule, you can double click that height rule and change it to:
height: 10px; width: 50px;
and hit enter and the new width rule will appear in addition to the height rule.

Again, all that is missing is a button or menu item to allow the user to do this easily.

[Zach Brock]

It appear the back end work to support this already exists, there is just no button to make it easy to do.

It is currently possible to do this if you select an existing rule, add a semi-colon to the end, insert a new rule and hit 'enter'.

For example, if inspecting an element, you see
height: 10px;
as the only style rule, you can double click that height rule and change it to:
height: 10px; width: 50px;
and hit enter and the new width rule will appear in addition to the height rule.

Again, all that is missing is a button or menu item to allow the user to do this easily.

[Alexander Pavlov (apavlov)]

This works fine, now that a lot of work has been done in the Web Inspector land.

[Alexander Pavlov (apavlov)]

Fix resolution status as advised by AP