1521013 - Implement selector editing in the new rules view

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, enhancement, P3)

Description

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

Main logic https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/views/rule-editor.js#653-723

Comment 1

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

Attachment: 1521013.patch [1.0]

You can also see another example of how we used the InplaceEditor in the box model. https://searchfox.org/mozilla-central/source/devtools/client/inspector/boxmodel/box-model.js#273

Comment 2

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

Attachment: 1521013.patch [2.0]

Comment 3

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

Attachment: 1521013.patch [3.0]

Comment 4

[Razvan Caliman [:rcaliman]]

Comment on attachment 9038039 [details] [diff] [review]
1521013.patch [3.0]

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

::: devtools/client/inspector/rules/components/Rules.js
@@ +10,5 @@
>  const Rule = createFactory(require("./Rule"));
>  
>  const Types = require("../types");
>  
>  class Rules extends PureComponent {

Do you think we still need the standalone Rules component? It seems to only iterate through the given array to return a list of individual Rule components, forwarding them the callbacks as props.

Both the Rules (wrapper) and Rule (individual) components are used in RulesApp. It looks like that's the only place where they're used.

If you don't foresee any logic that needs to live in the Rules component, it would simplify the code quite a bit if we drop this wrapper and just iterate the array and generate individual Rule components right in the RulesApp. 

Moving this logic up the stack reduces some of the prop drilling too.

::: devtools/client/inspector/rules/models/element-style.js
@@ +363,5 @@
> +      });
> +
> +      // Recompute the list of applied styles because editing a
> +      // selector might cause this rule's position to change.
> +      const appliedStyles = await this.pageStyle.getApplied(this.element, {

I was very confused for a while why `getApplied()` would return an array when the actor implementation, which passes down the result of getAppliedProps(), returns an object with multiple arrays:

```
    return {
      entries: entries,
      rules: [...rules],
      sheets: [...sheets],
    };
```
https://searchfox.org/mozilla-central/rev/330daedbeac2bba296d663668e0e0cf248bc6823/devtools/server/actors/styles.js#762-766

This was especially confusing since the spec defines the same structure:
https://searchfox.org/mozilla-central/rev/330daedbeac2bba296d663668e0e0cf248bc6823/devtools/shared/specs/styles.js#45-49

It turns out that the front overwrites the `getApplied()` implementation and discards everything in that object aside from the `entries` array which it returns directly:
https://searchfox.org/mozilla-central/rev/330daedbeac2bba296d663668e0e0cf248bc6823/devtools/shared/fronts/styles.js#66-76

This change was done way back in 2016. 

There don't seem to be any other consumers of the method on the server. On the client, the only consumers, via the overwritten front, are the Rule view, Computed view and a few tests:
https://searchfox.org/mozilla-central/search?q=getApplied%28&path=


Given this information, what do you think about refactoring the server method to return consistently and perahps avoid doing needless extra work to get that data that's not actually used? 

This should be a separate bug, of course, but I'd like to get your insight on this. Perhaps there are other use cases I'm not familiar with. But having the actor, spec and front respect the same return signature would definitely help others in the future.

@@ +394,5 @@
> +      }
> +
> +      this._changed();
> +    } catch (err) {
> +      promiseWarn(err);

Shouldn't we return this rejected promise?

::: devtools/client/inspector/rules/new-rules.js
@@ +197,5 @@
> +   *         The selector's span element to show the inplace editor.
> +   * @param  {String} ruleId
> +   *         The id of the Rule to be modified.
> +   */
> +  onShowSelectorEditor(element, ruleId) {

This method name is slightly misleading and it's passed as a prop through many layers of components.

On a cursory read, it seems to be a callback _in response_ to showing the selector editor when, in fact, it's a request to set it up. Perhaps showSelectorEditor or setupSelectorEditor is less ambiguous.

Comment 5

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

(In reply to Razvan Caliman [:rcaliman] from comment #4)

Comment on attachment 9038039 [details] [diff] [review]
1521013.patch [3.0]

Review of attachment 9038039 [details] [diff] [review]:

::: devtools/client/inspector/rules/components/Rules.js
@@ +10,5 @@

const Rule = createFactory(require("./Rule"));

const Types = require("../types");

class Rules extends PureComponent {

Do you think we still need the standalone Rules component? It seems to only
iterate through the given array to return a list of individual Rule
components, forwarding them the callbacks as props.

Both the Rules (wrapper) and Rule (individual) components are used in
RulesApp. It looks like that's the only place where they're used.

If you don't foresee any logic that needs to live in the Rules component, it
would simplify the code quite a bit if we drop this wrapper and just iterate
the array and generate individual Rule components right in the RulesApp.

Moving this logic up the stack reduces some of the prop drilling too.

I wrote the Rules component initially because we needed to pass a component to the Accordion items. I added a new getRuleProps method to help with the duplicate prop declarations in RulesApp component. I will file a new bug to investigate whether or not we can remove the Rules component somehow.

::: devtools/client/inspector/rules/models/element-style.js
@@ +363,5 @@

•  });
  

•  // Recompute the list of applied styles because editing a
  

•  // selector might cause this rule's position to change.
  

•  const appliedStyles = await this.pageStyle.getApplied(this.element, {
  

I was very confused for a while why getApplied() would return an array
when the actor implementation, which passes down the result of
getAppliedProps(), returns an object with multiple arrays:

    return {
      entries: entries,
      rules: [...rules],
      sheets: [...sheets],
    };

https://searchfox.org/mozilla-central/rev/
330daedbeac2bba296d663668e0e0cf248bc6823/devtools/server/actors/styles.
js#762-766

This was especially confusing since the spec defines the same structure:
https://searchfox.org/mozilla-central/rev/
330daedbeac2bba296d663668e0e0cf248bc6823/devtools/shared/specs/styles.js#45-
49

It turns out that the front overwrites the getApplied() implementation and
discards everything in that object aside from the entries array which it
returns directly:
https://searchfox.org/mozilla-central/rev/
330daedbeac2bba296d663668e0e0cf248bc6823/devtools/shared/fronts/styles.js#66-
76

This change was done way back in 2016.

There don't seem to be any other consumers of the method on the server. On
the client, the only consumers, via the overwritten front, are the Rule
view, Computed view and a few tests:
https://searchfox.org/mozilla-central/search?q=getApplied%28&path=

Given this information, what do you think about refactoring the server
method to return consistently and perahps avoid doing needless extra work to
get that data that's not actually used?

This should be a separate bug, of course, but I'd like to get your insight
on this. Perhaps there are other use cases I'm not familiar with. But having
the actor, spec and front respect the same return signature would definitely
help others in the future.

Yes, we should definitely refactor this. It's also confusing to me too. I will file a new bug for this.

@@ +394,5 @@

•  }
  

•  this._changed();
  

• } catch (err) {

•  promiseWarn(err);
  

Shouldn't we return this rejected promise?

Since an async function returns a promise already, we don't need to return promiseWarn(err). It's also an eslint error to do this. We could await promiseWarn(err), but looking at what promiseWarn did. I simplified it to simply console.error(err) since await Promise.reject(new Error("") is equivalent to throw new Error(""). https://javascript.info/async-await#error-handling

::: devtools/client/inspector/rules/new-rules.js
@@ +197,5 @@

• •     The selector's span element to show the inplace editor.
    

• • @param {String} ruleId

• •     The id of the Rule to be modified.
    

• */
• onShowSelectorEditor(element, ruleId) {

This method name is slightly misleading and it's passed as a prop through
many layers of components.

On a cursory read, it seems to be a callback in response to showing the
selector editor when, in fact, it's a request to set it up. Perhaps
showSelectorEditor or setupSelectorEditor is less ambiguous.

Renamed to showSelectorEditor.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a721c78e648
Implement selector editing in the new rules view. r=rcaliman

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2a721c78e648