[rule view] Editing rules' selectors for the current selection in the CSS rule-view

RESOLVED FIXED in Firefox 33

Status

defect
P3
normal
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: pbro, Assigned: gl)

Tracking

unspecified
Firefox 33
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 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 edit those rules' selectors.

This would be useful.

Just like css properties and values which are editable, selectors could become editable on click (or if reached by TABing with the keyboard and ENTERed).

There is obviously a question about selector validation. Indeed, if the new selector does not match anymore the current element, should the change be reverted? Or committed? 
If we decide to let the change be done, the rule should therefore disappear from the panel.
Reporter

Updated

5 years ago
See Also: → 966895
> There is obviously a question about selector validation. Indeed, if the new
> selector does not match anymore the current element, should the change be
> reverted? Or committed? 
> If we decide to let the change be done, the rule should therefore disappear
> from the panel.

This is the biggest UI challenge for this feature we would need to tackle.  Having a bad change make the selector just disappear would be jarring, leaving no way to recover from a mistake.

One option would be to grey out a bad change (while still allowing editing).  Then, if the problem is not resolved, once the selection changes from this node and comes back it would be removed.  We would need to be careful that any rule view refresh does not trigger unmatched selectors from being removed, since this can happen on page resize, inline style changes, etc.
Priority: -- → P3

Comment 2

5 years ago
FWIW, Firebug's main way of dealing with the UI problem is to give every editor a red/yellow/green blurry border, signalling how valid the input is. In this case, green border means the selector matches the element, making it a bit more obvious to the user whether the selector will disappear after editing stops.

The code we have for this is at https://github.com/firebug/firebug/blob/60d330b539ad5c885cc1578468b472a71ff70f44/extension/content/firebug/css/cssRuleEditor.js; it's a bit overcomplicated and bug 37468 would help.
Assignee

Updated

5 years ago
Blocks: 966895
Assignee

Updated

5 years ago
No longer blocks: 966895
Depends on: 37468
Assignee

Updated

5 years ago
Blocks: 966895
Assignee

Updated

5 years ago
Assignee: nobody → gabriel.luong
Assignee

Comment 3

5 years ago
Posted patch 966896.patch (obsolete) — Splinter Review
This patch adds an editable field to allow for editing selectors in the rule view (excludes ua styles and inline element).

Currently, the rule for the edited selector is removed, and the css rule with the new selector is inserted to the front of the stylesheet.
Attachment #8438223 - Flags: feedback?(pbrosset)
Assignee

Updated

5 years ago
Status: NEW → ASSIGNED
Reporter

Comment 4

5 years ago
Comment on attachment 8438223 [details] [diff] [review]
966896.patch

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

You're definitely on the right track, this is looking pretty good.
There are a few important things missing that I've noted below.
I haven't yet really dug into the parentStylesheet while loop reasoning yet, but will do that next.

::: browser/devtools/styleinspector/rule-view.js
@@ +1735,5 @@
> +    if (this.isEditable && this.rule.domRule.type !== ELEMENT_STYLE) {
> +      editableField({
> +        element: this.selectorText,
> +        done: this._onSelectorDone,
> +        destroy: this.update,

I think you should avoid adding a new function to simply do 'this.ruleView.refreshPanel()'.
Especially naming it update could give users of the RuleEditor class the impression that calling it would only update that one rule.
That's something we will probably be doing, in time, to make performance of the rule-view better, but at this stage, since you're only calling refreshPanel, I would just do:

...
  destroy: () => this.ruleView.refreshPanel(),
...

I don't exactly remember how the inplace-editor works, but why do you update on destroy and not on done?

@@ +2028,5 @@
> +   *        True if the change should be applied.
> +   */
> +  _onSelectorDone: function(aValue, aCommit) {
> +    if (aCommit && aValue !== "") {
> +      this.rule.domRule.modifyRuleSelector(aValue);

You're calling a new actor method here which might not actually be available on the server, depending on the target currently being debugged (we support as far back as Firefox 28 right now).

One solution is to add a trait to the root actor: see http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/root.js#101. The list of traits is sent first thing so, in your front-end code (rule-view.js), you can check any of them to decide if you should enable or not some part of the code.
In your case, you could create a styleRuleSelectorEditable trait (or something like that) and use it in the rule-view in the 'if (this.isEditable && this.rule.domRule.type !== ELEMENT_STYLE) {' condition.

Also, the call to modifyRuleSelector is async and we probably want to be blocking any further selector updates for as long as the response hasn't been received.
The method will return a promise, so you should store a flag on the RuleEditor instance that prevents any new modification and define resolve/reject handlers that remove that flag.

::: toolkit/devtools/server/actors/styles.js
@@ +700,5 @@
>    }, {
>      request: { modifications: Arg(0, "array:json") },
>      response: { rule: RetVal("domstylerule") }
> +  }),
> +  

nit: trailing white space (you might want to have something that remove those automatically on save, otherwise it's hard to get rid of them).

@@ +705,5 @@
> +  /**
> +   * Removes the current rule and inserts a new rule with the new selector
> +   * into the parent style sheet.
> +   */
> +  modifyRuleSelector: method(function(selector) {

This method is part of StyleRuleActor, so I think naming this function simply modifySelector would be enough.

@@ +707,5 @@
> +   * into the parent style sheet.
> +   */
> +  modifyRuleSelector: method(function(selector) {
> +    let rule = this.rawRule;
> +    let parentStyleSheet = rule.parentStyleSheet;

Not all rules have a parentStyleSheet property. Some of them may have a parentRule instead. I think you should be testing for 'this.type' here.
Even if the front-end code already does it, the server should validate its inputs.

@@ +709,5 @@
> +  modifyRuleSelector: method(function(selector) {
> +    let rule = this.rawRule;
> +    let parentStyleSheet = rule.parentStyleSheet;
> +    let document;
> +    

nit: trailing white space

@@ +720,5 @@
> +      document = parentStyleSheet.ownerNode;
> +    } else {
> +      document = parentStyleSheet.ownerNode.ownerDocument;
> +    }
> +    

nit: trailing white space

@@ +722,5 @@
> +      document = parentStyleSheet.ownerNode.ownerDocument;
> +    }
> +    
> +    selector = selector.trim();
> +    if (document.querySelector(selector) && rule.selectorText !== selector) {

You have to assume the provided selector may contain invalid characters that would make querySelector throw.
You'll need to try/catch this call.

If the query fails, it probably would be interesting to signify this to the caller, by using a response type. Maybe to start with, the response type could just be boolean and be true if the change succeeded and false otherwise.
This could then be used by the rule-view.js to do things (perhaps avoid refreshing the view if false).

@@ +724,5 @@
> +    
> +    selector = selector.trim();
> +    if (document.querySelector(selector) && rule.selectorText !== selector) {
> +      let cssRules = parentStyleSheet.cssRules;
> +      

nit: trailing white space

@@ +727,5 @@
> +      let cssRules = parentStyleSheet.cssRules;
> +      
> +      // Delete the currently selected rule
> +      let i = 0;
> +      while (i < cssRules.length) {

Why not iterating with a for..of loop here. It would feel more natural I think.

@@ +734,5 @@
> +        if (r === rule) {
> +          parentStyleSheet.deleteRule(i);
> +          break;
> +        }
> +        

nit: trailing white space

@@ +740,5 @@
> +      }
> +
> +      // Inserts the new style rule into the current style sheet
> +      let ruleText = rule.cssText.slice(rule.selectorText.length).trim();
> +      parentStyleSheet.insertRule(selector + " " + ruleText, 0);

Using 0 as the insertion index isn't necessarily correct in all cases. You may be inserting a rule that will now override other rules that it did not before the change. As a user, I expect the rule that I modify to remain where it was before, so that the only change I made is its selector, not its position.

I think you should be using i, but can't be sure.
Attachment #8438223 - Flags: feedback?(pbrosset) → feedback+
Assignee

Comment 5

5 years ago
Posted patch 966896.patch (obsolete) — Splinter Review
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #4)
> Comment on attachment 8438223 [details] [diff] [review]
> 966896.patch
>
> Review of attachment 8438223 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> You're definitely on the right track, this is looking pretty good.
> There are a few important things missing that I've noted below.
> I haven't yet really dug into the parentStylesheet while loop reasoning yet,
> but will do that next.
>
> ::: browser/devtools/styleinspector/rule-view.js
> @@ +1735,5 @@
> > +    if (this.isEditable && this.rule.domRule.type !== ELEMENT_STYLE) {
> > +      editableField({
> > +        element: this.selectorText,
> > +        done: this._onSelectorDone,
> > +        destroy: this.update,
>
> I think you should avoid adding a new function to simply do
> 'this.ruleView.refreshPanel()'.
> Especially naming it update could give users of the RuleEditor class the
> impression that calling it would only update that one rule.
> That's something we will probably be doing, in time, to make performance of
> the rule-view better, but at this stage, since you're only calling
> refreshPanel, I would just do:
>
> ...
>   destroy: () => this.ruleView.refreshPanel(),
> ...
>

Fixed. Removed the update function.

> I don't exactly remember how the inplace-editor works, but why do you update
> on destroy and not on done?
>

This was more to be consistent with the other editable fields, which also updates
on destroy, but I moved it into the promise chain of onSelectorDone now.

> @@ +2028,5 @@
> > +   *        True if the change should be applied.
> > +   */
> > +  _onSelectorDone: function(aValue, aCommit) {
> > +    if (aCommit && aValue !== "") {
> > +      this.rule.domRule.modifyRuleSelector(aValue);
>
> You're calling a new actor method here which might not actually be available
> on the server, depending on the target currently being debugged (we support
> as far back as Firefox 28 right now).
>
> One solution is to add a trait to the root actor: see
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> root.js#101. The list of traits is sent first thing so, in your front-end
> code (rule-view.js), you can check any of them to decide if you should
> enable or not some part of the code.
> In your case, you could create a styleRuleSelectorEditable trait (or
> something like that) and use it in the rule-view in the 'if (this.isEditable
> && this.rule.domRule.type !== ELEMENT_STYLE) {' condition.
>

Fixed. Added selectorEditable to the traits in the root actor, and checks for the
trait.

> Also, the call to modifyRuleSelector is async and we probably want to be
> blocking any further selector updates for as long as the response hasn't
> been received.
> The method will return a promise, so you should store a flag on the
> RuleEditor instance that prevents any new modification and define
> resolve/reject handlers that remove that flag.
>

Fixed. Added an isEditable flag to RuleEditor and handle the new flag in the resolve/reject handlers.

> ::: toolkit/devtools/server/actors/styles.js
> @@ +700,5 @@
> >    }, {
> >      request: { modifications: Arg(0, "array:json") },
> >      response: { rule: RetVal("domstylerule") }
> > +  }),
> > +
>
> nit: trailing white space (you might want to have something that remove
> those automatically on save, otherwise it's hard to get rid of them).
>

Fixed. Turned back on trim whitespace on sublime.

> @@ +705,5 @@
> > +  /**
> > +   * Removes the current rule and inserts a new rule with the new selector
> > +   * into the parent style sheet.
> > +   */
> > +  modifyRuleSelector: method(function(selector) {
>
> This method is part of StyleRuleActor, so I think naming this function
> simply modifySelector would be enough.
>

Fixed. Renamed to modifySelector.

> @@ +707,5 @@
> > +   * into the parent style sheet.
> > +   */
> > +  modifyRuleSelector: method(function(selector) {
> > +    let rule = this.rawRule;
> > +    let parentStyleSheet = rule.parentStyleSheet;
>
> Not all rules have a parentStyleSheet property. Some of them may have a
> parentRule instead. I think you should be testing for 'this.type' here.
> Even if the front-end code already does it, the server should validate its
> inputs.
>

TODO - Handle parentRule edge case?

Added test for this.type to ensure it is not an inline element inside of
modifySelector on the server side.

> @@ +709,5 @@
> > +  modifyRuleSelector: method(function(selector) {
> > +    let rule = this.rawRule;
> > +    let parentStyleSheet = rule.parentStyleSheet;
> > +    let document;
> > +
>
> nit: trailing white space
>

Fixed.

> @@ +720,5 @@
> > +      document = parentStyleSheet.ownerNode;
> > +    } else {
> > +      document = parentStyleSheet.ownerNode.ownerDocument;
> > +    }
> > +
>
> nit: trailing white space
>

Fixed.

> @@ +722,5 @@
> > +      document = parentStyleSheet.ownerNode.ownerDocument;
> > +    }
> > +
> > +    selector = selector.trim();
> > +    if (document.querySelector(selector) && rule.selectorText !== selector) {
>
> You have to assume the provided selector may contain invalid characters that
> would make querySelector throw.
> You'll need to try/catch this call.
>

Fixed. Added try/catch.

> If the query fails, it probably would be interesting to signify this to the
> caller, by using a response type. Maybe to start with, the response type
> could just be boolean and be true if the change succeeded and false
> otherwise.
> This could then be used by the rule-view.js to do things (perhaps avoid
> refreshing the view if false).
>

Fixed. Added a boolean response type to check if the selector was modified,
and refreshes the view if true.

> @@ +724,5 @@
> > +
> > +    selector = selector.trim();
> > +    if (document.querySelector(selector) && rule.selectorText !== selector) {
> > +      let cssRules = parentStyleSheet.cssRules;
> > +
>
> nit: trailing white space
>

Fixed.

> @@ +727,5 @@
> > +      let cssRules = parentStyleSheet.cssRules;
> > +
> > +      // Delete the currently selected rule
> > +      let i = 0;
> > +      while (i < cssRules.length) {
>
> Why not iterating with a for..of loop here. It would feel more natural I
> think.
>

Fixed. Used for loop here.

> @@ +734,5 @@
> > +        if (r === rule) {
> > +          parentStyleSheet.deleteRule(i);
> > +          break;
> > +        }
> > +
>
> nit: trailing white space

Fixed.

>
> @@ +740,5 @@
> > +      }
> > +
> > +      // Inserts the new style rule into the current style sheet
> > +      let ruleText = rule.cssText.slice(rule.selectorText.length).trim();
> > +      parentStyleSheet.insertRule(selector + " " + ruleText, 0);
>
> Using 0 as the insertion index isn't necessarily correct in all cases. You
> may be inserting a rule that will now override other rules that it did not
> before the change. As a user, I expect the rule that I modify to remain
> where it was before, so that the only change I made is its selector, not its
> position.
>
> I think you should be using i, but can't be sure.

Fixed. Inserted the rule to where the old rule was before. In this case, it is indeed i.

Additions:
- Handled promise chain rejections
- Handled pseudo elements and classes
Attachment #8438223 - Attachment is obsolete: true
Assignee

Comment 6

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

Comment 7

5 years ago
Posted patch 966896.patch (obsolete) — Splinter Review
Addressed Heather's comments via IRC:
- Removed traversing up of the stylesheet to get the original stylesheet if the stylesheet was imported. We don't want to do this because we won't be able to find the rule.
- Added a getter to get the document
Attachment #8440143 - Attachment is obsolete: true
Reporter

Comment 8

5 years ago
Comment on attachment 8440531 [details] [diff] [review]
966896.patch

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

Code changes look good. Thanks!
Just a comment regarding the try/catch and a couple of ideas for follow-up bugs.
Otherwise it works great as far as I've been able to test.
Very happy to see this working!
We'll need tests too.

::: browser/devtools/styleinspector/rule-view.js
@@ +2134,5 @@
>      }
> +  },
> +
> +  /**
> +   * Called when the selector name's inplace editor is closed.

s/the selector name's/the selector's

@@ +2144,5 @@
> +   * @param {boolean} aCommit
> +   *        True if the change should be applied.
> +   */
> +  _onSelectorDone: function(aValue, aCommit) {
> +    if (aCommit && !this.isEditing && aValue !== "") {

In general, we try and return early from functions to avoid indenting the whole body unnecessarily:

if (!aCommit || this.isEditing || aValue === "") {
  return;
}

// then the rest, un-indented

@@ +2147,5 @@
> +  _onSelectorDone: function(aValue, aCommit) {
> +    if (aCommit && !this.isEditing && aValue !== "") {
> +      this.isEditing = true;
> +
> +      this.rule.domRule.modifySelector(aValue).then((isModified) => {

nit: no need for parens around the single-param fat arrow function

@@ +2151,5 @@
> +      this.rule.domRule.modifySelector(aValue).then((isModified) => {
> +        this.isEditing = false;
> +
> +        if (isModified) {
> +          this.ruleView.refreshPanel();

I don't exactly know what, but I think we need some sort of user feedback if isModified is false.
It makes sense to clear the incorrect selector and go back to the rule as it was, but then it's hard for the user to know what's going on, especially if the user made a non-obvious mistake. There's nothing right now that will tell the user what's gone wrong.
Since this is a new feature however, we can definitely iterate with follow up bugs, so I'm fine to handle this in a separate bug, and we probably need UX's eyes on this (can you ping Darrin?).

@@ +2153,5 @@
> +
> +        if (isModified) {
> +          this.ruleView.refreshPanel();
> +        }
> +      }).then(null, (err) => {

ditto here

::: toolkit/devtools/server/actors/styles.js
@@ +711,5 @@
> +  }),
> +
> +  /**
> +   * Removes the current rule and inserts a new rule with the new selector
> +   * into the parent style sheet.

nit: please complete this jsdoc comment with @param and @return explaining the signature of this method.

@@ +722,5 @@
> +    let rule = this.rawRule;
> +    let parentStyleSheet = rule.parentStyleSheet;
> +    let document = this.getDocument(parentStyleSheet);
> +
> +    try {

You've enclosed most of the code of this method in the try/catch, therefore silencing any exception. I think you should only use the try/catch around the querySelector part

@@ +728,5 @@
> +      let [selector, pseudoProp] = value.split(/(:{1,2}\w+$)/);
> +
> +      // Check if the selector is valid and not the same as the original
> +      // selector
> +      if (document.querySelector(selector) && rule.selectorText !== selector) {

I was originally about to say that we missed a check here to make sure selector actually matches the current node, but just realized that we can't do that because the user might want to edit a selector of an inherited rule, and we have no easy way of checking if the new selector will lead to an inherited rule too.

This means we need to accept all selectors, as you did, but this also means  there might be cases where the modification is successful but the rule disappears from the rule-view, which, in terms of UX, isn't very easy to deal with.

Fwiw, in these cases, chromedevtools keeps the rule in the rule-view, but greyed out as long as you don't select another node, and allows the user to edit the selector still.
This is nice because it gives the user feedback about the change that was made and an opportunity to correct it.
If we want to also do this, we'll have to store this 'fake' rule on the client, because the actor really doesn't care about this, it only gives you the list of applied rules.
So you could keep the new selector + list of properties/values in the rule-view, until we receive the boolean response. If true, just discard it, if false, insert a new "special" rule in the view and keep it only as long as the current node is selected.
This fake rule will need to be editable too. I haven't looked into this, might be complex. I think you better file a follow-up bug for this and for now, just accept any selector and refresh the view as you're doing now.

Oh and I think you need to compare rule.selectorText to value rather than to selector.
Attachment #8440531 - Flags: feedback+
Assignee

Comment 9

5 years ago
Posted patch 966896.patch (obsolete) — Splinter Review
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #8)
> Comment on attachment 8440531 [details] [diff] [review]
> 966896.patch
> Review of attachment 8440531 [details] [diff] [review]:
> -----------------------------------------------------------------
> Code changes look good. Thanks!
> Just a comment regarding the try/catch and a couple of ideas for follow-up
> bugs.
> Otherwise it works great as far as I've been able to test.
> Very happy to see this working!
> We'll need tests too.
> ::: browser/devtools/styleinspector/rule-view.js
> @@ +2134,5 @@
> >      }
> > +  },
> > +
> > +  /**
> > +   * Called when the selector name's inplace editor is closed.
> s/the selector name's/the selector's
Fixed. s/the selector name's/the selector's

> @@ +2144,5 @@
> > +   * @param {boolean} aCommit
> > +   *        True if the change should be applied.
> > +   */
> > +  _onSelectorDone: function(aValue, aCommit) {
> > +    if (aCommit && !this.isEditing && aValue !== "") {
> In general, we try and return early from functions to avoid indenting the
> whole body unnecessarily:
> if (!aCommit || this.isEditing || aValue === "") {
>   return;
> }
> // then the rest, un-indented
Fixed. Returned early in onSelectorDone and un-indented main body.

> @@ +2147,5 @@
> > +  _onSelectorDone: function(aValue, aCommit) {
> > +    if (aCommit && !this.isEditing && aValue !== "") {
> > +      this.isEditing = true;
> > +
> > +      this.rule.domRule.modifySelector(aValue).then((isModified) => {
> nit: no need for parens around the single-param fat arrow function
Fixed. Removed parens.

> @@ +2151,5 @@
> > +      this.rule.domRule.modifySelector(aValue).then((isModified) => {
> > +        this.isEditing = false;
> > +
> > +        if (isModified) {
> > +          this.ruleView.refreshPanel();
> I don't exactly know what, but I think we need some sort of user feedback if
> isModified is false.
> It makes sense to clear the incorrect selector and go back to the rule as it
> was, but then it's hard for the user to know what's going on, especially if
> the user made a non-obvious mistake. There's nothing right now that will
> tell the user what's gone wrong.
> Since this is a new feature however, we can definitely iterate with follow
> up bugs, so I'm fine to handle this in a separate bug, and we probably need
> UX's eyes on this (can you ping Darrin?).
TODO: Add a follow up bug to provide user feedback when invalid selectors are inputted or something goes wrong

> @@ +2153,5 @@
> > +
> > +        if (isModified) {
> > +          this.ruleView.refreshPanel();
> > +        }
> > +      }).then(null, (err) => {
> ditto here
Same TODO as above.

> ::: toolkit/devtools/server/actors/styles.js
> @@ +711,5 @@
> > +  }),
> > +
> > +  /**
> > +   * Removes the current rule and inserts a new rule with the new selector
> > +   * into the parent style sheet.
> nit: please complete this jsdoc comment with @param and @return explaining
> the signature of this method.
Fixed. Completed jsdoc.

> @@ +722,5 @@
> > +    let rule = this.rawRule;
> > +    let parentStyleSheet = rule.parentStyleSheet;
> > +    let document = this.getDocument(parentStyleSheet);
> > +
> > +    try {
> You've enclosed most of the code of this method in the try/catch, therefore
> silencing any exception. I think you should only use the try/catch around
> the querySelector part
Fixed. I wasn't too sure what was the best way about going with this. I took the
most literal approach that only the querySelector part should be wrapped in the try/catch.

> @@ +728,5 @@
> > +      let [selector, pseudoProp] = value.split(/(:{1,2}\w+$)/);
> > +
> > +      // Check if the selector is valid and not the same as the original
> > +      // selector
> > +      if (document.querySelector(selector) && rule.selectorText !== selector) {
> I was originally about to say that we missed a check here to make sure
> selector actually matches the current node, but just realized that we can't
> do that because the user might want to edit a selector of an inherited rule,
> and we have no easy way of checking if the new selector will lead to an
> inherited rule too.
> This means we need to accept all selectors, as you did, but this also means
> there might be cases where the modification is successful but the rule
> disappears from the rule-view, which, in terms of UX, isn't very easy to
> deal with.
> Fwiw, in these cases, chromedevtools keeps the rule in the rule-view, but
> greyed out as long as you don't select another node, and allows the user to
> edit the selector still.
> This is nice because it gives the user feedback about the change that was
> made and an opportunity to correct it.
> If we want to also do this, we'll have to store this 'fake' rule on the
> client, because the actor really doesn't care about this, it only gives you
> the list of applied rules.
> So you could keep the new selector + list of properties/values in the
> rule-view, until we receive the boolean response. If true, just discard it,
> if false, insert a new "special" rule in the view and keep it only as long
> as the current node is selected.
> This fake rule will need to be editable too. I haven't looked into this,
> might be complex. I think you better file a follow-up bug for this and for
> now, just accept any selector and refresh the view as you're doing now.
> Oh and I think you need to compare rule.selectorText to value rather than to
> selector.

TODO: File a follow up bug for this.
Attachment #8440531 - Attachment is obsolete: true
Assignee

Comment 10

5 years ago
Posted patch 966896.patch (obsolete) — Splinter Review
- Added unit tests that addresses commits, pseudo elements/classes and simple selector edits.
- Added stopOnShiftTab to inplace editor. This is to prevent a timing issue where this.isEditing would still be true because there is an editor still open when trying to refresh the panel.
- Adjusted the regular expression to split selector and pseudo properties in modifySelector
- Adjusted the selectorText check to make sure it is not the same as the current value entered. This check applies to both client and server side.

try: https://tbpl.mozilla.org/?tree=Try&rev=390d65190c8a
Attachment #8440993 - Attachment is obsolete: true
Attachment #8442482 - Flags: review?(pbrosset)
Assignee

Comment 11

5 years ago
Actual try to reflect the latest patch https://tbpl.mozilla.org/?tree=Try&rev=9f9cb6b913b2
Reporter

Comment 12

5 years ago
Comment on attachment 8442482 [details] [diff] [review]
966896.patch

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

Awesome. This is very close to landing.
The code changes look good, try build is green (apart from unrelated xpcshell test failures), and tests you've added look really nice too (nice to see the head.js helper functions being used).

I have added some comments in the code, none of them are a big deal.

I think the only thing that prevents this from landing is a couple of bugs I've found while playing with it:

- Click to edit a selector, then click again in the focused field -> The field is blurred and a new property inplace-editor is added. I guess you need to disable the click-to-add-a-new-property thing when the click is in the selector field.

- Click to edit a selector, change the value to something valid (that matches an element in the parent document), then press TAB -> An exception is thrown:
console.error: 
  Message: TypeError: parentStyleSheet is null
  Stack:
    StyleRuleActor<.modifyProperties<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/styles.js:687:14
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:943:13
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1154:9
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:540:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:7
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:7

And the rule stays in the rule-view.

R+ with these 2 bugs fixed and 2 new test cases added for them.

::: browser/devtools/shared/inplace-editor.js
@@ +170,5 @@
>    this.done = aOptions.done;
>    this.destroy = aOptions.destroy;
>    this.initial = aOptions.initial ? aOptions.initial : this.elt.textContent;
>    this.multiline = aOptions.multiline || false;
> +  this.stopOnShiftTab = !!aOptions.stopOnShiftTab;

Please also add a bit of documentation about this new option, just like stopOnReturn:

 *    {boolean} stopOnReturn:
 *       If true, the return key will not advance the editor to the next
 *       focusable element.

Also, I was looking for tests about this shared utility but couldn't find any. It seems we're only testing the inplace-editor via integration tests on our tools.
So I guess this means we're good as long as one of the tests you added simulates a shift-Tab key.

::: browser/devtools/styleinspector/rule-view.js
@@ +1690,5 @@
>  function RuleEditor(aRuleView, aRule) {
>    this.ruleView = aRuleView;
>    this.doc = this.ruleView.doc;
>    this.rule = aRule;
> +  this.toolbox = this.ruleView.inspector.toolbox;

Since you really only need toolbox in isSelectorEditable, I would avoid exposing it on the instance here. It doesn't have much to do with this class.
So you can remove this line and modify the getter below.

@@ +1695,2 @@
>    this.isEditable = !aRule.isSystem;
> +  // Flag that blocks other selector updates when a selector is being edited

I think this comment is a tiny bit misleading because when I read it I thought this was used to block other selectors (as in other rules' selectors) from being edited. Whereas it really blocks the same selector from being edited again for as long as the server hasn't responded yet.
Maybe rephrase it like this: "Flag that blocks updates of the selector when it is being edited"

@@ +1704,5 @@
>  }
>  
>  RuleEditor.prototype = {
> +  get isSelectorEditable() {
> +    return this.toolbox._target.client.traits.selectorEditable;

Could be rewritten like so:

get isSelectorEditable() {
  let toolbox = this.ruleView.inspector.toolbox;
  return toolbox.target.client.traits.selectorEditable;
}

Note that you can use toolbox.target (a getter) rather than toolbox._target (a private var)

@@ +2062,5 @@
> +
> +      if (isModified) {
> +        this.ruleView.refreshPanel();
> +      }
> +    }).then(null, (err) => {

nit: no need for parens around err

::: browser/devtools/styleinspector/test/browser.ini
@@ +56,5 @@
>  [browser_ruleview_completion-new-property_01.js]
>  [browser_ruleview_completion-new-property_02.js]
>  [browser_ruleview_content_01.js]
>  [browser_ruleview_content_02.js]
> +[browser_ruleview_edit-selector-commit.js]

Please sort test names by alphabetical order in browser.ini (these 3 new tests should come right after browser_ruleview_edit-property_02.js)

@@ +58,5 @@
>  [browser_ruleview_content_01.js]
>  [browser_ruleview_content_02.js]
> +[browser_ruleview_edit-selector-commit.js]
> +[browser_ruleview_edit-selector_01.js]
> +[browser_ruleview_edit-selector_02.js]

Can you do one of 2 things:
- either rename all your new tests with 01/02/03 suffixes
- or rename them all with something more self-explanatory (tests should really be testing one and only one thing, so finding a short description in 2 or 3 words should be doable).
I have no preferences either way, 01/02/03/... sounds fine to me, as long as the first comment in the test files gives enough information about what the test really does.

::: browser/devtools/styleinspector/test/browser_ruleview_edit-selector-commit.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +// Test original selector value is correctly displayed when ESCaping out of the
> +// inplace editor in the style inspector

This test isn't solely about checking that ESC is handled correctly, so this comment should perhaps be rephrased:
// Check selector updates when committing the edit field with ENTER, ESC, TAB, ...
By the way, this test is checking what happens with shift-TAB but not with TAB only, should you be testing this here?

@@ +6,5 @@
> +
> +// Test original selector value is correctly displayed when ESCaping out of the
> +// inplace editor in the style inspector
> +
> +const originalValue = "#testid";

I wouldn't worry about repeating this information in testData instead of defining it here.

@@ +17,5 @@
> +  '</style>',
> +  '<div id="testid" class="testclass">Styled Node</div>',
> +].join("\n");
> +
> +const testData = [

nit: s/testData/TEST_DATA

Also, when you use a data array to iterate of test cases like here, it's usually a good idea to add a 'description' property to each of your test case object that you use with info(description) in runTestData:

const TEST_DATA = [
  {
    description: "Test that pressing ESC after an update doesn't commit the change",
    ...
  },
  ...
]

Then in runTestData:

function* runTestData(inspector, view, data) {
  let {node, value, commitKey, modifiers, expected, description} = data;
  info(description);
  ...
}

This makes it easier to read logs when investigating test failures.

Optionally, you could, in your case, auto-generate a comment instead of adding a description property:
info("Updating " + node + " to " + value + " and committing with " + commitKey + ". Expecting: " + expected);

@@ +23,5 @@
> +    node: "#testid",
> +    value: ".testclass",
> +    commitKey: "VK_ESCAPE",
> +    modifiers: {},
> +    expected: originalValue

Change to "#testid" here

@@ +71,5 @@
> +      "The selector editor got focused");
> +
> +  info("Enter the new selector value: " + value);
> +  for (let ch of value) {
> +    EventUtils.sendChar(ch, view.doc.defaultView);

The selector edit field doesn't live-preview as you type, so there's no need to send characters one by one.
In your case, once the field is focused, you could simply set the value with field.value = value;
And then only synthesize the commit key. This would be enough to simulate user interactions.

@@ +79,5 @@
> +  EventUtils.synthesizeKey(commitKey, modifiers);
> +
> +  if (commitKey === "VK_ESCAPE") {
> +    is(idRuleEditor.rule.selectorText, expected,
> +        "Value is as expected: " + expected);

Isn't idRuleEditor.rule.selectorText always going to be correct in this case?
I think you should also be testing if the RuleEditor isEditing flag isn't set to true. Otherwise, you're not really testing if an unwanted modification isn't being done.

::: browser/devtools/styleinspector/test/browser_ruleview_edit-selector_01.js
@@ +36,5 @@
> +
> +function* testEditSelector(view, name) {
> +  info("Test editing existing selector fields");
> +
> +  let idRuleEditor = view.element.children[1]._ruleEditor;

I think we're starting to have more and more of these in our rule-view tests, perhaps a new simple getRuleViewRuleEditor(index) {} function would be good, to simplify maintenance if this private property comes to change.

@@ +46,5 @@
> +    "The selector editor got focused");
> +
> +  info("Entering a new selector name and committing");
> +  for (let ch of name) {
> +    EventUtils.sendChar(ch, view.doc.defaultView);

Same comment as in the previous test, no need to send the value char by char.
Attachment #8442482 - Flags: review?(pbrosset)
Assignee

Comment 13

5 years ago
Comment on attachment 8442482 [details] [diff] [review]
966896.patch

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

Addressed the previous review feedback.

Changelog:
- Added inplace-editor.stopOnTab to stop advance of the editor from the selector to the properties. This fixes bug #2.
- Added checks for ruleEditor.isEditing when the properties are previewing/committing
- Used the new head.getRuleViewRuleEditor(index) on all test instances
- Added a selector container around the selector text/editor. This is used to fix bug #1 by stopping the event propagation if the click happens within the container.
- Adjusted unit tests.

::: browser/devtools/shared/inplace-editor.js
@@ +170,5 @@
>    this.done = aOptions.done;
>    this.destroy = aOptions.destroy;
>    this.initial = aOptions.initial ? aOptions.initial : this.elt.textContent;
>    this.multiline = aOptions.multiline || false;
> +  this.stopOnShiftTab = !!aOptions.stopOnShiftTab;

Fixed. Added docs for stopOnShiftTab.

There is also a shift-tab test in browser_ruleview_edit-selector-commit.js

::: browser/devtools/styleinspector/rule-view.js
@@ +1690,5 @@
>  function RuleEditor(aRuleView, aRule) {
>    this.ruleView = aRuleView;
>    this.doc = this.ruleView.doc;
>    this.rule = aRule;
> +  this.toolbox = this.ruleView.inspector.toolbox;

Fixed. Removed this.toolbox

@@ +1695,2 @@
>    this.isEditable = !aRule.isSystem;
> +  // Flag that blocks other selector updates when a selector is being edited

Fixed. Rephrase ruleEditor.isEditing comment. Also, included the fact that it blocks preview/commits of properties.

@@ +1704,5 @@
>  }
>  
>  RuleEditor.prototype = {
> +  get isSelectorEditable() {
> +    return this.toolbox._target.client.traits.selectorEditable;

Fixed. Moved toolbox to the getter

@@ +2062,5 @@
> +
> +      if (isModified) {
> +        this.ruleView.refreshPanel();
> +      }
> +    }).then(null, (err) => {

Fixed.Removed parens around err.

::: browser/devtools/styleinspector/test/browser.ini
@@ +56,5 @@
>  [browser_ruleview_completion-new-property_01.js]
>  [browser_ruleview_completion-new-property_02.js]
>  [browser_ruleview_content_01.js]
>  [browser_ruleview_content_02.js]
> +[browser_ruleview_edit-selector-commit.js]

Fixed. Sorted order in browser.ini

@@ +58,5 @@
>  [browser_ruleview_content_01.js]
>  [browser_ruleview_content_02.js]
> +[browser_ruleview_edit-selector-commit.js]
> +[browser_ruleview_edit-selector_01.js]
> +[browser_ruleview_edit-selector_02.js]

It should be okay as-is since the test file names currently follow the same convention as edit-property. Test 01/02 also have comments describing the tests.

::: browser/devtools/styleinspector/test/browser_ruleview_edit-selector-commit.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +// Test original selector value is correctly displayed when ESCaping out of the
> +// inplace editor in the style inspector

Fixed. Edited top comment and included TAB test.

@@ +6,5 @@
> +
> +// Test original selector value is correctly displayed when ESCaping out of the
> +// inplace editor in the style inspector
> +
> +const originalValue = "#testid";

Fixed. Removed originalValue constant.

@@ +17,5 @@
> +  '</style>',
> +  '<div id="testid" class="testclass">Styled Node</div>',
> +].join("\n");
> +
> +const testData = [

Fixed. Added the auto-generated description.

@@ +23,5 @@
> +    node: "#testid",
> +    value: ".testclass",
> +    commitKey: "VK_ESCAPE",
> +    modifiers: {},
> +    expected: originalValue

Fixed. Changed to "#testid"

@@ +71,5 @@
> +      "The selector editor got focused");
> +
> +  info("Enter the new selector value: " + value);
> +  for (let ch of value) {
> +    EventUtils.sendChar(ch, view.doc.defaultView);

Fixed. Used editor.input.value = value to all 3 tests.

@@ +79,5 @@
> +  EventUtils.synthesizeKey(commitKey, modifiers);
> +
> +  if (commitKey === "VK_ESCAPE") {
> +    is(idRuleEditor.rule.selectorText, expected,
> +        "Value is as expected: " + expected);

Fixed. Added test for ruleEditor.isEditing

::: browser/devtools/styleinspector/test/browser_ruleview_edit-selector_01.js
@@ +36,5 @@
> +
> +function* testEditSelector(view, name) {
> +  info("Test editing existing selector fields");
> +
> +  let idRuleEditor = view.element.children[1]._ruleEditor;

Fixed. Added getRuleViewRuleEditor(index)

@@ +46,5 @@
> +    "The selector editor got focused");
> +
> +  info("Entering a new selector name and committing");
> +  for (let ch of name) {
> +    EventUtils.sendChar(ch, view.doc.defaultView);

Fixed. Used editor.input.value = value
Assignee

Comment 14

5 years ago
Posted patch 966896.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=a88d93fcaa1f
Attachment #8442482 - Attachment is obsolete: true
Attachment #8444663 - Flags: review?(pbrosset)
Assignee

Comment 15

5 years ago
Posted patch 966896.patch (obsolete) — Splinter Review
Attachment #8444663 - Attachment is obsolete: true
Attachment #8444663 - Flags: review?(pbrosset)
Assignee

Comment 16

5 years ago
Posted patch 966896.patchSplinter Review
Rebased and removed whitespaces

https://tbpl.mozilla.org/?tree=Try&rev=ac90b98649e7
Attachment #8444665 - Attachment is obsolete: true
Attachment #8444687 - Flags: review?(pbrosset)
Reporter

Comment 17

5 years ago
Comment on attachment 8444687 [details] [diff] [review]
966896.patch

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

- Applied and tested the patch, both of the bugs seem fixed
- All my code review remarks were addressed
- Green try build

Yay! \o/ Congrats for an awesome new feature!
Let's make sure we file those few follow-up bugs we discussed about.
Attachment #8444687 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/mozilla-central/rev/dd0a5c93ab9c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33

Updated

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