Bug 18224 - Adding new CSS style properties to HTML nodes using Web Inspector
Summary: Adding new CSS style properties to HTML nodes using Web Inspector
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-03-30 12:42 PDT by Markus Knaup
Modified: 2010-03-18 03:51 PDT (History)
5 users (show)

See Also:


Attachments
Patch for adding new CSS style properties in the Web Inspector style side bar (8.39 KB, patch)
2008-03-30 12:46 PDT, Markus Knaup
no flags Details | Formatted Diff | Diff
Tabs were removed. (8.14 KB, patch)
2008-03-30 13:50 PDT, Markus Knaup
aroben: review-
Details | Formatted Diff | Diff
Screenshot with attachment 20219 applied (31.67 KB, image/png)
2008-04-02 08:02 PDT, Adam Roben (:aroben)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Knaup 2008-03-30 12:42:16 PDT
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.
Comment 1 Markus Knaup 2008-03-30 12:46:56 PDT
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.
Comment 2 Matt Lilek 2008-03-30 13:00:16 PDT
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). 
Comment 3 Markus Knaup 2008-03-30 13:50:41 PDT
Created attachment 20219 [details]
Tabs were removed.

I removed the tabs, sorry about that.
Comment 4 Matt Lilek 2008-03-30 13:56:32 PDT
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
Comment 5 Adam Roben (:aroben) 2008-04-02 08:02:34 PDT
Created attachment 20292 [details]
Screenshot with attachment 20219 [details] applied

For those who are curious (like me)
Comment 6 Adam Roben (:aroben) 2008-04-02 08:23:26 PDT
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!
Comment 7 Markus Knaup 2008-04-03 06:49:38 PDT
Thanks for your input! I will upload a new patch in a few days.
Comment 8 Zach Brock 2009-04-15 11:41:16 PDT
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.
Comment 9 Zach Brock 2009-04-15 11:41:57 PDT
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.
Comment 10 Alexander Pavlov (apavlov) 2010-03-17 10:54:46 PDT
This works fine, now that a lot of work has been done in the Web Inspector land.
Comment 11 Alexander Pavlov (apavlov) 2010-03-18 03:51:00 PDT
Fix resolution status as advised by AP