1139058 - Rule view should display unmatched rules that are added

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Attachment: pseudo-argh.gif

Invalid selectors input via 'add new rule' ( or selectors made invalid by editing them ) mysteriously disappear from the inspector - we should instead show them and indicate visually that they are invalid.

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1139058.patch

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1139058.patch

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1139058.patch

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

One problem that I am trying to fix is when I enter an invalid selector (eg, "div::::") that will cause the try catch to return a SyntaxError from trying to insertRule in styles.js#modifySelector, and I get some Parser.jsm error and stops me from further modifying the selector.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8591478 [details] [diff] [review]
1139058.patch

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

Thanks Gabriel.
I have 2 general remarks:
- You've moved the modifySelector method between actors: this is backward incompatible, this will make debugging older servers fail. If this is really needed, you'll need to update the front-end code as well to use actorHasMethod to detect which version of the server you're debugging, and have 2 code paths, but:
- Why not keep your original method, but improve it so that:
  - it accepts a new parameter which is the current node selection (which will need backward compatibility handling too),
  - it checks if the new selector still matches that node (i.e does querySelectorAll(selector) contains the node),
  - it still returns the existing isModified boolean, but on top, also returns a isValid, or isMatching boolean that indicates if the new selector matches the node that was passed.
I think this should be enough to have the right code on the client to handle the case gracefully, and limits the number of changes on the server.
On the client, you could then use the isMatching return value:
  - if true and isModified is true too, refresh the the panel, exactly as today,
  - if false, just add the invalid attribute to the rule container, and don't refresh the panel.

This would avoid to refresh the Rule for nothing, and would get rid of isRuleValid.

How does that sound?

::: browser/themes/shared/devtools/ruleview.css
@@ +77,5 @@
>    border-bottom-color: transparent;
>  }
>  
> +/* Display rules that don't match the current selected element differently */
> +.ruleview-rule[valid=false] {

Can you group this with .ruleview-rule[uneditable=true] and edit the comment there ?
Also, it somehow makes more sense to me to have the attribute called "invalid" instead, and I don't think it needs a value:
.ruleview-rule[invalid]

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> Comment on attachment 8591478 [details] [diff] [review]
> 1139058.patch
> 
> Review of attachment 8591478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Gabriel.
> I have 2 general remarks:
> - You've moved the modifySelector method between actors: this is backward
> incompatible, this will make debugging older servers fail. If this is really
> needed, you'll need to update the front-end code as well to use
> actorHasMethod to detect which version of the server you're debugging, and
> have 2 code paths, but:
> - Why not keep your original method, but improve it so that:
>   - it accepts a new parameter which is the current node selection (which
> will need backward compatibility handling too),
>   - it checks if the new selector still matches that node (i.e does
> querySelectorAll(selector) contains the node),
>   - it still returns the existing isModified boolean, but on top, also
> returns a isValid, or isMatching boolean that indicates if the new selector
> matches the node that was passed.
> I think this should be enough to have the right code on the client to handle
> the case gracefully, and limits the number of changes on the server.
> On the client, you could then use the isMatching return value:
>   - if true and isModified is true too, refresh the the panel, exactly as
> today,
>   - if false, just add the invalid attribute to the rule container, and
> don't refresh the panel.
> 
> This would avoid to refresh the Rule for nothing, and would get rid of
> isRuleValid.
> 
> How does that sound?
> 
> ::: browser/themes/shared/devtools/ruleview.css
> @@ +77,5 @@
> >    border-bottom-color: transparent;
> >  }
> >  
> > +/* Display rules that don't match the current selected element differently */
> > +.ruleview-rule[valid=false] {
> 
> Can you group this with .ruleview-rule[uneditable=true] and edit the comment
> there ?
> Also, it somehow makes more sense to me to have the attribute called
> "invalid" instead, and I don't think it needs a value:
> .ruleview-rule[invalid]

Your suggestion would work, but I should've explained my proposed solution. The key idea to fixing this is creating a new RuleEditor similar to how it is done in addNewRule and replace the existing one if the selector is modified successfully. One of the reason I moved it to the Page Actor Style was to access getApplied, and _styleRef, but it seems I can also do that since I have pageStyle in the Style Rule Actor.

Creating a new RuleEditor with the new Rule created by modifySelector if successful means we can edit that new rule as well even if it doesn't match the current selector. See https://cloudup.com/cXXsmO8Ksnn

Rather than getting back isModified we would get the arguments required to setup the new Rule for the RuleEditor, and avoid the entire process of refreshing the panel and just swap the RuleEditors with the new one.

In its current state, we can go with modifying the existing function in the Page Actor Style or Style Rule Actor. Moving it to the Page Actor Style would allow us to refactor some of the shared code between modifySelector and addNewRule, and from a semantic standpoint, I think the functionality should belong in the Page Actor since this is adding new rule that affects the page rather than a Style Rule. The benefit with keeping it in the Style Rule would be backward compatibility.

Do you have any preferences with which direction we should go? I am already swinging towards keeping the function in the Style Rule Actor for backward compatibility.

There are a couple of concerns that will need to be addressed which is if rule overridden is appearing correctly, and how to handle invalid selectors when we are performing stylesheet.insertRule since the try catch doesn't seem to be doing its job as described in comment #5.

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Brian any feedback in terms of direction since Patrick is away

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Gabriel Luong [:gl] from comment #8)
> Brian any feedback in terms of direction since Patrick is away

I looked at this, and I think it'd be best to wait for Patrick's feedback.  But my general advice here is at least for the initial review, favor keeping it in the current actor for backwards compat over semantics *if* doing so doesn't cause extra backwards compat headache for if/when we move it elsewhere.  Then if you two decide the benefit of refactoring / better semantics is worth it, manage that in a second patch that's just focused on doing that.

Comment 10

[Patrick Brosset <:pbro>]

Sorry for the delay Gabriel.

Moving modifySelector to the PageStyleActor sounds fine to me too.
Please do this first as a separate patch, with proper client-side actor method detection.
If needed, you can always create a little facade helper function or class that handles the 2 code paths behind a common interface.
I still think there is value in the method checking if the new selector still matches the node and sending back a boolean flag about this in the response, if only to simplify the client-side logic in rule-view.js a little bit and make the method more useful for later.

(In reply to Gabriel Luong [:gl] from comment #7)
> Your suggestion would work, but I should've explained my proposed solution.
Thanks for the explanation, that helps.

> Creating a new RuleEditor with the new Rule created by modifySelector if
> successful means we can edit that new rule as well even if it doesn't match
> the current selector. See https://cloudup.com/cXXsmO8Ksnn
Ah, yes, good point. But I guess my proposals allowed for this too. With my solution, you'd just keep the same rule editor, somehow greyd out, but it should still work, right? I haven't tried this.

> The key idea to fixing this is creating a new RuleEditor similar to how it
> is done in addNewRule and replace the existing one if the selector is
> modified successfully.
I guess I'm ok with this approach, but I'd like to see some code refactored here. In your patch, the code in _onSelectorDone looks almost exactly like the one in _onAddRule. So we need a new function to handle this.
Refreshing the whole panel as we used to do before this patch sounds bad because we know the performance of the rule-view isn't great, but if it were, I'd much more inclined for a solution like this rather than spreading out the complex new Rule + new RuleEditor logic throughout rule-view.js
So, at least refactor this part so it's all done in one place.

> In its current state, we can go with modifying the existing function in the
> Page Actor Style or Style Rule Actor. Moving it to the Page Actor Style
> would allow us to refactor some of the shared code between modifySelector
> and addNewRule
Ah, cool, so we agree :)

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Allow unmatched rules to be added to the rule view

I was working with bgrins in MV on this and we decided it was best for backward compatibility to keep the method in its current actor. I am wondering what kind of backward checks we need at this point.

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

I have a part 2 which includes test cases that needs to be rebased, but the next issue is to somehow persist the unmatched rules that are added which could be removed by refreshPanel from a layout-change. This will be addressed in part 3.

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb0fa7e952e8

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Unit test for adding unmatched rules to the rule view

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8607356 [details] [diff] [review]
Part 1: Allow unmatched rules to be added to the rule view

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

I haven't had the time to properly look at all the code changes, but I made a few comments anyway.

Now, in terms of backward compatibility:
you're changing an existing protocol method by adding a new parameter and changing the return value.
This means that if you connect a firefox desktop with this patch applied to a firefoxos phone that was built previously, or (simply) to another firefox desktop without this patch (you can use the "connect" screen in the devtools), you will end up having a Front that expects a certain method signature trying to talk to an actor that has a different method signature.
The Front will try to send it an extra argument that the Actor does not expect, and even if that worked, the Actor will send back a serialized packet that the Front doesn't know how to deserialized because it expects another one.

I haven't tested this, but I'm pretty sure this would fail. Can you test this to make sure? Open dev-edition on profile1, open gcli, type in 'listen 6000', open your build with this patch, open the connect screen (tools/web developer/connect...), enter localhost/6000, and then try to modify a selector.

I think the only way to make this work is to introduce a new trait in the root actor (/toolkit/devtools/server/actors/root.js), a boolean that says whether or not the server supports this new feature (just like the addNewRule trait).

::: toolkit/devtools/server/actors/styles.js
@@ +78,5 @@
>    rules: "array:domstylerule",
>    sheets: "array:stylesheet"
>  });
>  
> +types.addDictType("modifiedStylesreturn", {

s/modifiedStylesreturn/modifiedStylesReturn

@@ +1108,4 @@
>     * Removes the current rule and inserts a new rule with the new selector
>     * into the parent style sheet.
>     * @param string value
>     *        The new selector value

Missing jsdoc for the new node parameter.

@@ +1129,3 @@
>  
> +    // Check if the selector is not the same as the original selector
> +    if (selectorText !== value) {

The body of this IF is rather long, so it makes sense to invert the condition and do an early return instead, so the rest doesn't need to be indented.

if (selectorText === value) {
  return { ruleProps: null, isMatching: true };
}

for (let i ....

@@ +1130,5 @@
> +    // Check if the selector is not the same as the original selector
> +    if (selectorText !== value) {
> +      for (let i = 0; i < cssRules.length; i++) {
> +        if (rule === cssRules.item(i)) {
> +          try {

Do you really need 2 nested try/catches?
Please try and only put inside a try/catch what you know can fail and you can handle nicely anyway. All other exceptions should bubble through normally, otherwise they'll be silenced.

@@ +1141,5 @@
> +            // Determine if the new selector value matches the current selected
> +            // element
> +            let matchedElements;
> +            try {
> +              matchedElements = document.querySelectorAll(value);

value is constant in this function, so you could call querySelectorAll outside of the loop.

@@ +1146,5 @@
> +            } catch (e) {}
> +
> +            if (matchedElements) {
> +              for (let element of matchedElements) {
> +                if (element === node.rawNode) {

Also node is constant I believe, so no need to nest this loop inside the top one.

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #15)
> This means that if you connect a firefox desktop with this patch applied to
> a firefoxos phone that was built previously, or (simply) to another firefox
> desktop without this patch (you can use the "connect" screen in the
> devtools), you will end up having a Front that expects a certain method
> signature trying to talk to an actor that has a different method signature.
> The Front will try to send it an extra argument that the Actor does not
> expect, and even if that worked, the Actor will send back a serialized
> packet that the Front doesn't know how to deserialized because it expects
> another one.
> 
> I haven't tested this, but I'm pretty sure this would fail. Can you test
> this to make sure? Open dev-edition on profile1, open gcli, type in 'listen
> 6000', open your build with this patch, open the connect screen (tools/web
> developer/connect...), enter localhost/6000, and then try to modify a
> selector.
> 

I've also tested this type of thing using the WebIDE and connecting to a simulator.  You can pull down this project if you want a simple packaged app to run: [e-mail]:bgrins/devtools-demos.git

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Allow unmatched rules to be added to the rule view [1.0]

Comment 18

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Unit test for adding unmatched rules to the rule view [1.0]

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c53e459ff29e

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Readd unmatched rules to the active element when the panel refreshes [1.0]

So, the panel will refresh when the layout changes. This would get rid of all the unmatched rules. Part 3 introduces an array that will store these unmatched rules and readd them to end of the current list of rules. I am trying to come up with a way where we can keep the refreshed rules more or less in the same order with the unmatched rules, but that could be a follow up (maybe?).

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 4: Unit test for unmatched rules added on panel refresh [1.0]

Comment 22

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Unit test for adding unmatched rules to the rule view [1.0]

Comment 23

[Gabriel Luong [:gl] (ΦωΦ)]

I tested with an old simulater and I got protocol errors. I added the new trait in part 1.

Comment 24

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fc76aa727f9

Comment 25

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Brian Grinstead [:bgrins] from comment #16)
> I've also tested this type of thing using the WebIDE and connecting to a
> simulator.  You can pull down this project if you want a simple packaged app
> to run: [e-mail]:bgrins/devtools-demos.git

WebIDE can also connect to Firefox OS browser tabs, so you can start the simulator, open it's browser, go to some page, and then choose that tab from WebIDE project menu.  Whichever is simplest for you!

Comment 28

[Patrick Brosset <:pbro>]

Comment on attachment 8607818 [details] [diff] [review]
Part 1: Allow unmatched rules to be added to the rule view [1.0]

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

Almost there. The patch seems to work well, but there's a change of behavior when connecting to older debuggees which I think we should correct.

::: browser/devtools/styleinspector/rule-view.js
@@ +2601,5 @@
>     */
>    _onSelectorDone: function(aValue, aCommit) {
>      if (!aCommit || this.isEditing || aValue === "" ||
> +        aValue === this.rule.selectorText ||
> +        !this.target.client.traits.modifySelector) {

Adding a condition for this trait here means that if you connect to an older debuggee that doesn't have this patch, the behavior will change.

- Without your patch (tested on aurora): if I enter an unmatching selector, the view refreshes, the new rule disappears, but if I then select a matching node, it's there.

- With this patch, and connected to an older debuggee (connecting remotely to aurora): if I enter an unmatching selector and press enter, the view doesn't refresh and the new rule is still there, with the default selector, without my modification.

We have to decide whether that's find to live with or if we want to use the trait to preserve the exact same behavior we had before this patch when connecting to older debuggees. My suggestion is let's try and do that.
Neither solutions are perfect, but I'd still prefer to keep the current behavior.

To do this, you'd need to remove the trait condition in this IF here.
And instead, re-add the code you had before inside a new IF, something like:

  _onSelectorDone: function(aValue, aCommit) {
    if (!aCommit || this.isEditing || aValue === "" ||
        aValue === this.rule.selectorText) {
      return;
    }

    let ruleView = this.ruleView;
    let elementStyle = ruleView._elementStyle;
    let element = elementStyle.element;
    let supportsUnmatchedRules = this.target.client.traits.modifySelector;

    this.isEditing = true;

    let modifySelector;
    if (supportsUnmatchedRules) {
      // If the debuggee supports adding unmatched rules (post FF41).
      modifySelector = this.rule.domRule.modifySelector(element, aValue);
    } else {
      modifySelector = this.rule.domRule.modifySelector(aValue);
    }

    modifySelector.then(response => {
      this.isEditing = false;

      if (!supportsUnmatchedRules && response) {
        this.ruleView.refreshPanel();
        return;
      }

      let {ruleProps, isMatching} = response;
      if (!ruleProps) {
        return;
      }

      let newRule = new Rule(elementStyle, ruleProps);
      let editor = new RuleEditor(ruleView, newRule);
      let rules = elementStyle.rules;

      rules.splice(rules.indexOf(this.rule), 1);
      rules.push(newRule);
      elementStyle._changed();

      editor.element.setAttribute("unmatched", !isMatching);
      this.element.parentNode.replaceChild(editor.element, this.element);

      // Remove highlight for modified selector
      if (ruleView.highlightedSelector &&
          ruleView.highlightedSelector == this.rule.selectorText) {
        ruleView.toggleSelectorHighlighter(ruleView.lastSelectorIcon,
          ruleView.highlightedSelector);
      }
    }).then(null, err => {
      this.isEditing = false;
      promiseWarn(err);
    });
  }

::: toolkit/devtools/server/actors/root.js
@@ +138,5 @@
>      // that modifies the rule's selector
>      selectorEditable: true,
> +    // Whether the style rule actor implements the modifySelector method
> +    // that allows for unmatched rule to be added
> +    modifySelector: true,

How about something like modifySelectorUnmatched? Just trying to be a little more self explanatory.

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

Can you add a note here along these lines:
"Note that this method's signature has changed in FF41, see trait modifySelectorUnmatched"

@@ +1164,4 @@
>            break;
>          }
> +      }
> +    }

You can replace the last ~10 lines of code by using el.matches(str), I think you'd still need the try/catch in case the selector is invalid, but:

let isMatching = false;
try {
  isMatching = node.rawNode.matches(value);
} catch (e) {
  // Selector value is invalid, not a problem.
}

@@ +1301,5 @@
>        });
> +  },
> +
> +  modifySelector: protocol.custom(function(node, value) {
> +    return this._modifySelector(node, value).then(response => {

See my comment in rule-view.js.
You'll need to use the new trait here too because the front code is unconditionally loaded on the client, no matter what server version is running.
So you'll need to test the value of the trait here and, if false, revert to using the older signature of _modifySelector.

Comment 29

[Patrick Brosset <:pbro>]

Comment on attachment 8607980 [details] [diff] [review]
Part 2: Unit test for adding unmatched rules to the rule view [1.0]

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

These test changes look good.
Having fixed the existing tests in a separate patch is nice because it ease the review process, but once part 1 is R+, you'll need to merge it with part 2, because with part 1, the existing tests fail,

::: browser/devtools/styleinspector/test/browser_ruleview_edit-selector_04.js
@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test that the selector highlighter is removed when modifying a selector

Good catch adding this test. Can you also add another one that tests that the selectorHighlighter still works after the selector was modified to either a matching selector or unmatching selector?

Comment 30

[Patrick Brosset <:pbro>]

Comment on attachment 8607872 [details] [diff] [review]
Part 3: Readd unmatched rules to the active element when the panel refreshes [1.0]

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

Can you file a new bug for patches part 3 and part 4?
I think the changes you're making in part 1 and 2 are ok to land separately. The layout-change issue isn't great, for sure, but the way the rule-view UI workflow is implemented right now is so confusing that I'd prefer having a dedicated bug to add yet one more way to refresh/populate the view.

You're approach in this patch is pretty simple, but as you said, we need to preserve the sorting of rules, and I'd like to keep some time to think of other potential ways to implement this.
Right now if you trigger a layout-change and the selected node has inherited rules, then the unmatched rules will appear below that, which feels odd somehow.
I wish we didn't even have to refresh the view fully on layout changes, but we do need to handle media-queries and markup changes on resize anyway.

I wish we had a client-side model of the rules to which we could write from the rule-view, and when a server-side update would be needed, we would update the model accordingly and only then refresh the view.
ElementStyle is the closest thing to this, but it's not clear how the model is built/updated.
So I'd like the new bug to be about making ElementStyle more like a model:
- all rules (even unmatched) should be stored in this.rules
- there should be a add method,
- there should be a clear method (unless we do a new ElementStyle everytime, I don't remember),
- populate should not replace this.rules entirely but only update/insert

Comment 31

[Patrick Brosset <:pbro>]

Comment on attachment 8607966 [details] [diff] [review]
Part 4: Unit test for unmatched rules added on panel refresh [1.0]

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

See part 3 review.

Comment 32

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Allow unmatched rules to be added to the rule view [2.0]

Comment 33

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Unit test for adding unmatched rules to the rule view [2.0]

Added new test for adding a new property to an unmatched rule. Also, added the extra test to make sure the selector highlighter works for the unmatched rule in 04.

Comment 34

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd0a72f7fa51

Comment 35

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Allow unmatched rules to be added to the rule view [2.0]

Comment 36

[Patrick Brosset <:pbro>]

Comment on attachment 8610023 [details] [diff] [review]
Part 1: Allow unmatched rules to be added to the rule view [2.0]

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

Nice. r=me.
I have just made a comment about maybe moving the new trait from the rootActor to the StyleRuleActor.
I'd rather this be done now instead of later because otherwise there will yet an extra backward compatibility check to be done.
No need to ping me for review again on this patch though.

::: toolkit/devtools/server/actors/styles.js
@@ +1181,5 @@
>      protocol.Front.prototype.initialize.call(this, client, form, ctx, detail);
> +
> +    this.traits = {
> +      supportsUnmatchedRules: client.traits.modifySelectorUnmatched
> +    }

This change made me realize we could define the new trait on the StyleRuleActor rather than globally on the root actor.
The root actor's list of traits has been the preferred way until some time ago to store all boolean flags used for backward compatibility checks, but it ended up becoming a big list of things that have nothing to do together, and I believe we should try and handle checks as close as possible to the code that's being changed.

So, in order of preference, we should:
- use the target.actorHasMethod(actor, method) method when possible,
- or, next, store a trait on the actor itself when possible,
- or, last, add a global trait on the rootActor.

So, in your case, I'd suggest adding a trait on StyleRuleActor:
- in the function form in StyleRuleActor (around line 990):
  let form = {
    actor: this.actorID,
    ...
    traits: {
      modifySelectorUnmatched: true
    }
  };
- in the StyleRuleFront, add a getter for this trait:
  get supportsModifySelectorUnmatched() {
    return this._form.traits &&
           this._form.traits.modifySelectorUnmatched;
  }
  (checking for this._form.traits first is important because older targets won't have it. We could just not have the traits object at all, and just the property on _form, but introducing this extra object level now may prove useful in the future)
- remove the change in root.js
- remove the this.traits definition you added here in initialize
- just use this.supportsModifySelectorUnmatched in the protocol modifySelector custom method in the front
- in rule-view.js, instead of using 
  this.target.client.traits.modifySelectorUnmatched;
  use this.rule.domRule.supportsModifySelectorUnmatched.

I think this should work, and helps keep the handling of the backward compatibility contained to the files that actually have to deal with it.

Comment 37

[Patrick Brosset <:pbro>]

Comment on attachment 8609102 [details] [diff] [review]
Part 2: Unit test for adding unmatched rules to the rule view [2.0]

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

Just one note about not using _applyingModifications, otherwise looks good.

::: browser/devtools/styleinspector/test/browser_ruleview_edit-selector_05.js
@@ +95,5 @@
> +
> +  info("Entering a value and bluring the field to expect a rule change");
> +  editor.input.value = "center";
> +  let onBlur = once(editor.input, "blur");
> +  onModifications = ruleEditor.rule._applyingModifications;

Try and avoid using applyingModifications, we're in the process of getting rid of this in tests, see bug 1166774.

Comment 38

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Allow unmatched rules to be added to the rule view [3.0]

Addressed feedback. Patrick, I was testing this patch with a simulator and realized it is still not backwards compatible. I believe the problem is because of the return value of modifySelector not being the same type. Any thoughts on what we can do?

Comment 39

[Patrick Brosset <:pbro>]

Comment on attachment 8610320 [details] [diff] [review]
Part 1: Allow unmatched rules to be added to the rule view [3.0]

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

The interdiff from last patch looks great, thanks for making the changes I proposed with the trait.
So the last missing part is, as you said, the backward compatibility problem with the return value of the method you changed.
As discussed, the only solution to this is to actually make a separate method instead of trying to reuse the same.
It's a little bit sad that we have to clutter the code to support older devices but there's no other way I can think of. So, let's introduce a new method and try and share as much code as possible between the 2.
Also, good luck for finding a good name :) !

Comment 40

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Allow unmatched rules to be added to the rule view [4.0]

Comment 41

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Unit test for adding unmatched rules to the rule view [3.0]

Comment 42

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9517bdfb6dc1

Comment 43

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Unit test for adding unmatched rules to the rule view [4.0]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19237470830f

Comment 44

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffe55d0af453

Comment 45

[Patrick Brosset <:pbro>]

Comment on attachment 8610797 [details] [diff] [review]
Part 1: Allow unmatched rules to be added to the rule view [4.0]

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

::: toolkit/devtools/server/actors/styles.js
@@ +80,5 @@
>  });
>  
> +types.addDictType("modifiedStylesReturn", {
> +  isMatching: RetVal("nullable:boolean"),
> +  ruleProps: RetVal("nullable:appliedStylesReturn")

Hmm, I hadn't looked in details at why exactly you needed to have a custom method on the front, but I'm now thinking this retVal isn't what you need and you could probably get rid of the custom front method after all.

What if replaced this with:

types.addDictType("modifiedStylesReturn", {
  isMatching: RetVal("boolean"), // note I removed the nullable:, I don't this this can happen
  ruleProps: RetVal("nullable:appliedstyle")
});

so that modifySelector2 could directly return 'this.pageStyle.getNewAppliedProps(node, newCssRule).entries[0]' and so you could remove the custom modifySelector2 front method?

Can you try if that would work.
It's not a big deal if it doesn't, I haven't looked at if this would cause problems, but it's better if the actor method can directly return what's needed.
(Maybe this won't work if the appliedStylesReturn is in fact needed by protocol.js to register the new rule actor as it gets sent down the wire).

@@ +1005,5 @@
>        type: this.type,
>        line: this.line || undefined,
> +      column: this.column,
> +      traits: {
> +        // Whether the style rule actor implements the modifySelectorUnmatched

s/modifySelectorUnmatched/modifySelector2 : the feature is modifySelectorUnmatched but the method is named modifySelector2

@@ +1196,5 @@
> +
> +  /**
> +   * Modify the current rule's selector by inserting a new rule with the new
> +   * selector value and removing the current rule. This method supports adding
> +   * new rules with unmatched selectors.

Can you rephrase this comment to something like:

"Modify the current rule's selector by inserting a new rule with the new selector value and removing the current rule.

In contrast with the modifySelector method which was used before, this method also returns information about the new rule and applied style so that consumers can immediately display the new rule, whether or not the selector matches the current element, without having to refresh the whole list".

Comment 46

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Allow unmatched rules to be added to the rule view [5.0]

Comment 47

[Brian Grinstead [:bgrins]]

Comment on attachment 8611104 [details] [diff] [review]
Part 1: Allow unmatched rules to be added to the rule view [5.0]

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

::: toolkit/devtools/server/actors/styles.js
@@ +1374,5 @@
>          return location;
>        });
> +  },
> +
> +  modifySelector2: protocol.custom(function(node, value) {

My 2 cents: it'd be nice if we instead had a modifySelector function on the front that managed the `if (this.supportsModifySelectorUnmatched)` logic and called modifySelector / modifySelector2.  It feels like that bit of logic belongs on the Front rather than the tool frontend(s).  Up to you two if you want to go with the suggestion in Comment 45, something like this, or leave it.  Backwards compat is hard..

Comment 48

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Allow unmatched rules to be added to the rule view [6.0]

Added check for supportsModifySelectorUnmatched at the front and removed it from the client

Comment 49

[Brian Grinstead [:bgrins]]

Attachment: ruleview-unmatched-front-proposal.patch

This is how I did it in the WIP search patch for Bug 835896.  Using an impl property on a protocol.custom function for the front you can preserve the original name as far as the client is concerned

Comment 50

[Brian Grinstead [:bgrins]]

Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8611306&action=interdiff&newid=8611320&headers=1

Comment 51

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Allow unmatched rules to be added to the rule view [7.0]

Comment 52

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Unit test for adding unmatched rules to the rule view [5.0]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=730f28367f82

Comment 53

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Unit test for adding unmatched rules to the rule view [6.0]

Reverted edit-selector_05 to use applyingModifications and the changes from rebasing on top of bug 1166774.

Comment 54

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f1b7286ef3f

Comment 55

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e56dd3f52961
https://hg.mozilla.org/integration/fx-team/rev/f30178f41b1f

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e56dd3f52961
https://hg.mozilla.org/mozilla-central/rev/f30178f41b1f