Allow user to choose stylesheet when creating new rules.
Created attachment 310975 [details] Patch
Created attachment 311238 [details] Patch
Comment on attachment 311238 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311238&action=review > Source/WebCore/inspector/InspectorStyleSheet.cpp:944 > + // Using setText() as this operation changes the stylesheet rule set. Style: Add a newline before this. It deserves its own paragraph. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:286 > + const justSelector = true; > + let selector = this._nodeStyles.node.appropriateSelectorFor(justSelector); The _addNewRule above this uses: let selector = this.currentStyle().selectorText; And this uses: const justSelector = true; let selector = this._nodeStyles.node.appropriateSelectorFor(justSelector); What are the differences? Should these converge all on one common path? > LayoutTests/inspector/css/add-rule.html:10 > + let suite = InspectorTest.createAsyncSuite("CSSAgent.addRule"); Style: Name of this should be "CSS.addRule" not CSSAgent. We should also use this opportunity to test error cases. For instance a bad stylesheet id: CSSAgent.addRule("bad-id", "foo"); Should respond with a reasonable error and not crash. Using sentences for names, like you have here, seems okay. But I'd prefer something I can search for easily. For example: CSS.addRule.InNonExistingInspectorStyleSheet CSS.addRule.InExistingInspectorStyleSheet CSS.addRule.ExternalStyleSheet CSS.addRule.BadStyleSheetId For inspection (name, conventions) check out these: LayoutTests/inspector/dom/highlightNode.html LayoutTests/inspector/page/setEmulatedMedia.html LayoutTests/inspector/worker/worker-create-and-terminate.html
I haven't yet reviewed the entire change. How does this perform if you combine-and-minify the frontend, inspect the inspector, and add rules to Main.css? The reparse might be expensive, I'd like to get a feel for how expensive that is. If its noticeable by the user that would be a concern.
Comment on attachment 311238 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311238&action=review >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:286 >> + let selector = this._nodeStyles.node.appropriateSelectorFor(justSelector); > > The _addNewRule above this uses: > > let selector = this.currentStyle().selectorText; > > And this uses: > > const justSelector = true; > let selector = this._nodeStyles.node.appropriateSelectorFor(justSelector); > > What are the differences? Should these converge all on one common path? I actually forgot that the Visual Styles sidebar does this. When you create a new rule in the Rules sidebar, it sets the selector as the `appropriateSelectorFor` of the node. In the Visual sidebar, however, it will use the selector of whatever style is currently selected (if the inline rule is selected, it falls back to the `appropriateSelectorFor` of the node). IIRC, this was done to avoid having to add a "Duplicate Selector" context menu item. Seeing as this cannot be done in the Rules sidebar (the concept of selecting a style doesn't apply there), I think it's OK to have the Visual and Rules sidebars differ slightly in functionality. I will make `_addNewRuleContextMenu` behave like `addNewRuleClick`.
(In reply to Joseph Pecoraro from comment #4) > I haven't yet reviewed the entire change. > > How does this perform if you combine-and-minify the frontend, inspect the > inspector, and add rules to Main.css? The reparse might be expensive, I'd > like to get a feel for how expensive that is. If its noticeable by the user > that would be a concern. I tried testing this on a few different websites with large minified CSS files and didn't see any lag in WebInspector or style changes on the page.
Created attachment 311359 [details] Patch
Screenshot?
Created attachment 311889 [details] [Image] After Patch is applied
Comment on attachment 311359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311359&action=review > LayoutTests/inspector/css/add-rule-expected.txt:8 > +PASS: The selector is "body" > +PASS: The range is [0:0,0:4] > +PASS: The origin is "inspector" These could all be converted to `should be` sentences. Something like: PASS: Rule selector should be "body". PASS: Rule range should be [0:0,0:4]. PASS: Rule origin should be "inspector". > LayoutTests/inspector/css/add-rule.html:142 > + CSSAgent.addRule("-1", "div", ruleAdded); Same thing about an obviously bad identifier. "DOES_NOT_EXIST" or "BAD_IDENTIFIER" etc.
Created attachment 312252 [details] Patch
Comment on attachment 312252 [details] Patch Clearing flags on attachment: 312252 Committed r217911: <http://trac.webkit.org/changeset/217911>
All reviewed patches have been landed. Closing bug.
The test added in this patch, inspector/css/add-rule.html , seems to be a flaky timeout. It looks to have been since it was added. Linked bugs in See Also.