[rule view] Adding new rules to the current selection in the CSS rule-view

RESOLVED FIXED in Firefox 33

Status

defect
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: pbro, Assigned: gl)

Tracking

(Blocks 1 bug)

unspecified
Firefox 33
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

5 years ago
The rule-view inspector panel allows editing declarations inside of rules that apply to the currently selected element, but does not allow to add new rules.

This would be useful.

There should be either a '+' button somewhere or a contextual menu option, or some other means to trigger the rule addition.
The rule should, by default, be given a selector that matches the currently selected element.
Coming up with a selector that matches the element may be tricky, but we do have a utility that does this iirc (getUniqueCssSelector or something like this).
(Reporter)

Updated

5 years ago
See Also: → 966896
(Reporter)

Comment 1

5 years ago
There is obviously a question about selector validation. Indeed, if the new selector does not match the current element, should the change be reverted? Or committed?

Comment 2

5 years ago
I started right-clicking around the empty space to find such a feature. As I'm used to Firebug, I was expecting a "Add a rule" menu item in the right-click. A [+] button is acceptable as well.

This feature is necessary to attain Firebug parity.

From a user perspective, I don't care so much that the selector isn't accurate. What I find important however is that the possibility to add any rule, just even an empty selector I can write myself would do for the moment. If implementing that in two steps can work (add a rule, then add a rule that makes sense), I'd recommend it.
(Reporter)

Updated

5 years ago
Blocks: firebug-gaps
By rule, do you mean a selector { name:value} combination, or a name:value combination ?
(In reply to Girish Sharma [:Optimizer] from comment #3)
> By rule, do you mean a selector { name:value} combination, or a name:value
> combination ?

The feature in Firebug lets you add a new selector (right click in Firebug rule view and say 'Add Rule').  This opens up a textbox where you can enter a selector.  Once you do that, you can add name:value pairs.  If the selector was invalid, it seems to just disappear after you are done with the process.
(Reporter)

Comment 5

5 years ago
Yeah, by rule I meant a selector{name:value;} rule. name:value only would be a declaration.
I like how Firebug handles this because, even if the rule disappears when the selector doesn't match the currently selected node, the rule is still added, and if then switch to an element that does match the selector, the rule then appears.
So it's kind of like having a mini styleeditor UI inside of the rule-view, and once done, the rule-view is simply refreshed.

Comment 6

5 years ago
Firebug's implementation of this is here, feel free to copy: https://github.com/firebug/firebug/blob/60d330b539ad5c885cc1578468b472a71ff70f44/extension/content/firebug/css/cssPanel.js#L750-L815

The selector it generates is intentionally not unique - for such a case the user might just as well edit element.style. Instead it tries to come up with some reasonable guess from classes/ids of the element and its ancestors and lets the user fix it up as needed.

The rule is added to a temporary stylesheet injected into the page. (Chrome does the same and gives it a pseudo-URL of "inspector-stylesheet".)
(Assignee)

Updated

5 years ago
Assignee: nobody → gabriel.luong
(Assignee)

Updated

5 years ago
Depends on: 966896
(Assignee)

Updated

5 years ago
No longer depends on: 966896
(Assignee)

Updated

5 years ago
Depends on: 966896
(Assignee)

Comment 7

5 years ago
Posted patch 966895.patch [WIP] (obsolete) — Splinter Review
This patch added a new menu item when you right click and you can select to add a new rule. Currently, styles._styleRef would not construct a new StyleRuleActor since we are passing in the inline element again, and an actor already exists for the inline element. As a result, we get a new inline element rule. Ideally, we would create a new CSSStyleRule prior to constructing a new StyleRuleActor. However, that might depend on platform work. Alternatively, we could find a way around the current implementation. 

There are still considerations on how to get the new selectors to work. I believe finishing 966896 would be the first step towards completing this bug.
(Assignee)

Comment 8

5 years ago
Posted patch 996895.patch (obsolete) — Splinter Review
Attachment #8429304 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 9

5 years ago
Working implementation of 'Add Rule'. 

- Adds a context menu for 'Add Rule'
- A new style tag is added to the document and referenced in the PageStyleActor. New rules are appended to this new style element. After adding the new css rule, we grab the StyleRuleActor to create a new rule editor instead of refreshing the panel.
- New rule appears after the inline element rule and appears with an editable selector that is focused
(Assignee)

Comment 10

5 years ago
Posted patch addrule.patch (obsolete) — Splinter Review
- Fixed addNewRule to use styles#getApplied. Previously, the StyleRuleActor that we got back does not actually match the object that would be returned from rule-view#refreshPanel, and expected for the rule option in new Rule. The StyleRuleActor would be missing properties such as parentStyleSheet, which resulted in the user not being able to add new properties to the new rule until the panel refreshed.

The way refreshPanel seems to work is that it gets a list of rules that applies to the given element by calling styles#getApplied. In getApplied, the rule entries (StyleRuleActor) and stylesheet actor seems to get squashed together when it is returned. I modified styles#getApplied for my use case, so, that the returned StyleRuleActor from styles#addNewRule will return the same StyleRuleActor as expected for the rule-view#Rule object.
Attachment #8443293 - Attachment is obsolete: true
Attachment #8445353 - Flags: feedback?(pbrosset)
Attachment #8445353 - Flags: feedback?(fayearthur)
(Assignee)

Comment 11

5 years ago
Posted patch addrule.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=702a161d5e84
Attachment #8445353 - Attachment is obsolete: true
Attachment #8445353 - Flags: feedback?(pbrosset)
Attachment #8445353 - Flags: feedback?(fayearthur)
(Assignee)

Comment 12

5 years ago
- Added unit tests:
 #1 - Tests for adding new properties to the newly added rule
 #2 - Tests for ability to edit the selector for the newly added rule
(Assignee)

Updated

5 years ago
Attachment #8446178 - Flags: review?(fayearthur)
Comment on attachment 8446178 [details] [diff] [review]
addrule.patch

Review of attachment 8446178 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good, a few notes though. Do you know why it displays the new rule's line number as -1? We should fix that.

::: browser/devtools/styleinspector/rule-view.js
@@ +1363,5 @@
> +  /**
> +   * Add a new rule to the current element.
> +   */
> +  _onAddRule: function() {
> +    let pageStyle = this.pageStyle;

you don't use this var, so take it out.

@@ +1369,5 @@
> +    let element = elementStyle.element;
> +    let rules = elementStyle.rules;
> +    let client = this.inspector.toolbox._target.client;
> +
> +    if (client.traits.addNewRule) {

If you do the inverse of this by returning if !client.traits.addNewRule, then you can avoid indenting the next block.

::: toolkit/devtools/server/actors/styles.js
@@ +297,5 @@
>     *   `inherited`: Include styles inherited from parent nodes.
>     *   `matchedSeletors`: Include an array of specific selectors that
>     *     caused this rule to match its node.
> +   * @param array entries
> +   *   List of appliedstyle objects that lists the rules that apply to the node

You explained the need for "entries" earlier. Just from looking at the code it's confusing. I'd either make the use case more obvious in this comment, explaining when you'd want to pass in "entries".

Another option is to make two new functions here, one is an general helper function that contains all the logic that you want to share between getApplied and your new use case, and another appropriately named one that is specific for the new use case that calls this helper function.

@@ +540,5 @@
> +  /**
> +   * Helper function to addNewRule to construct a new style tag in the document.
> +   * @returns DOMElement of the style tag
> +   */
> +  getStyleElement: function() {

Can make this a getter with "get styleElement()" and use this._styleElement to back it.

::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties
@@ +104,5 @@
>  # the rule view context menu "Show CSS sources" entry.
>  ruleView.contextmenu.showCSSSources.accessKey=S
>  
> +# LOCALIZATION NOTE (ruleView.contextmenu.addRule): Text displayed in the
> +# rule view context menu.

add a bit more info e.g "displayed in the rule view context menu for adding a new rule to the element"
(Assignee)

Comment 14

5 years ago
Comment on attachment 8446178 [details] [diff] [review]
addrule.patch

Review of attachment 8446178 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the review! Patch incoming.

In addition to the fixes listed below, the following was changed:
- Source link for newly added rules are rendered unselectable
- Negative line numbers are not displayed

::: browser/devtools/styleinspector/rule-view.js
@@ +1363,5 @@
> +  /**
> +   * Add a new rule to the current element.
> +   */
> +  _onAddRule: function() {
> +    let pageStyle = this.pageStyle;

Fixed. Removed unused pageStyle var.

@@ +1369,5 @@
> +    let element = elementStyle.element;
> +    let rules = elementStyle.rules;
> +    let client = this.inspector.toolbox._target.client;
> +
> +    if (client.traits.addNewRule) {

Fixed. Did the inverse.

::: toolkit/devtools/server/actors/styles.js
@@ +297,5 @@
>     *   `inherited`: Include styles inherited from parent nodes.
>     *   `matchedSeletors`: Include an array of specific selectors that
>     *     caused this rule to match its node.
> +   * @param array entries
> +   *   List of appliedstyle objects that lists the rules that apply to the node

Fixed. Separated the main logic behind getApplied to a helper function (getAppliedProps) that is used by getApplied and addNewRule.

@@ +540,5 @@
> +  /**
> +   * Helper function to addNewRule to construct a new style tag in the document.
> +   * @returns DOMElement of the style tag
> +   */
> +  getStyleElement: function() {

Fixed. Used "get styleElement()"

::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties
@@ +104,5 @@
>  # the rule view context menu "Show CSS sources" entry.
>  ruleView.contextmenu.showCSSSources.accessKey=S
>  
> +# LOCALIZATION NOTE (ruleView.contextmenu.addRule): Text displayed in the
> +# rule view context menu.

Fixed. Added more details to the localization note.
(Assignee)

Comment 15

5 years ago
Posted patch addrule.patch (obsolete) — Splinter Review
try https://tbpl.mozilla.org/?tree=Try&rev=ec85e0d623f5
Attachment #8446178 - Attachment is obsolete: true
Attachment #8446178 - Flags: review?(fayearthur)
Attachment #8449034 - Flags: review?(fayearthur)
Comment on attachment 8449034 [details] [diff] [review]
addrule.patch

Review of attachment 8449034 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!
Attachment #8449034 - Flags: review?(fayearthur) → review+
(Assignee)

Comment 17

5 years ago
Posted patch addrule.patch (obsolete) — Splinter Review
Added commit message
Attachment #8449034 - Attachment is obsolete: true
Attachment #8450003 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
sorry had to back this out for test failures like in bug 1034015 - https://tbpl.mozilla.org/php/getParsedLog.php?id=43027597&tree=Fx-Team
Whiteboard: [fixed-in-fx-team]
Depends on: 1034015
(Assignee)

Comment 20

5 years ago
Posted patch addrule.patchSplinter Review
From the last back-out, I suspect it was a timing issue with Window 7 when it tries to fetch ruleText in styles.js:846 after deleting the rule.

The following dump illustrates what the variables are after deleting the currently selected rule in styles.js:844
16:49:31     INFO -  styles.modifySelector:842 - value = span
16:49:31     INFO -  styles.modifySelector:843 - value.length = 4
16:49:31     INFO -  styles.modifySelector:844 - i = 0
16:49:31     INFO -  styles.modifySelector:844 - rule.cssText =
16:49:31     INFO -  styles.modifySelector:844 - rule.selectorText.length = 0
16:49:31     INFO -  styles.modifySelector:844 - slice =
16:49:31     INFO -  styles.modifySelector:845 - ruleText=

rule.cssText should return the actual string value of the entire style rule, but it is empty in this case, and when it is slice it should remove the selector from the text.

To address this, I stored the cssText and selectorText prior to deleting the rule.

In addition, I added an additional test that tests adding a new rule and property followed by editing the selector.

https://tbpl.mozilla.org/?tree=Try&rev=bf6a8a61bf69
Attachment #8450003 - Attachment is obsolete: true
Attachment #8451704 - Flags: review?(fayearthur)
Attachment #8451704 - Flags: review?(fayearthur) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a7e7bafd6de9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33

Comment 23

5 years ago
I am really sorry I am new here (created an account 5 secs ago). I would i get this new feature installed on my inspector?
(Assignee)

Comment 24

5 years ago
(In reply to Davey McNight from comment #23)
> I am really sorry I am new here (created an account 5 secs ago). I would i
> get this new feature installed on my inspector?

Hi Davey, this feature is currently available on the Firefox Nightly builds (http://nightly.mozilla.org/). It should be available in Firefox Aurora sometime this week (once the version bumps up to Firefox 33).

The stable release of Firefox 33 does not happen until Oct 14, which will contain this feature. If you have any additional questions, let me know how I can help either here or on IRC at #devtools.

Comment 25

5 years ago
Added a rule and the class names were not prefixed with a dot "."

Should get ".col .whitewash .ground .communities"

Instead got ".col whitewash ground communities"
(Reporter)

Comment 26

5 years ago
(In reply to thinsoldier from comment #25)
> Added a rule and the class names were not prefixed with a dot "."
> 
> Should get ".col .whitewash .ground .communities"
> 
> Instead got ".col whitewash ground communities"
This got fixed by bug 1069980 in Firefox 35.

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.