1520389 - Implement the selector highlighter in the new rules view

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: 1520389.patch [1.0]

Comment 2

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

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

Comment 3

[Razvan Caliman [:rcaliman]]

Comment on attachment 9036822 [details] [diff] [review]
1520389.patch [1.0]

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

::: devtools/client/inspector/rules/components/SelectorHighlighter.js
@@ +24,5 @@
> +  constructor(props) {
> +    super(props);
> +
> +    this.state = {
> +      // An unique selector to the current Rule. This is used to check if the

s/An/A

@@ +25,5 @@
> +    super(props);
> +
> +    this.state = {
> +      // An unique selector to the current Rule. This is used to check if the
> +      // selector highlighter matches the highlighted selector and should be highlighted.

This comment is a bit of a tongue twister :)
Suggestion: A unique selector to the current Rule. This is checked against the value of any existing highlighted selector in order mark the toggled state of the component.

::: devtools/client/inspector/rules/models/rule.js
@@ +189,5 @@
> +  async getUniqueSelector() {
> +    let selector = "";
> +
> +    if (this.domRule.selectors) {
> +      // This is a "normal" rule with a selector.

s/"normal"/style

@@ +197,5 @@
> +      // selector from the node which rule this is inherited from.
> +      selector = await this.inherited.getUniqueSelector();
> +    } else {
> +      // This is an inline style from the current node.
> +      selector = this.elementStyle.ruleView.inspector.selectionCssSelector;

This is making a reference to the old (current) Rule view. It will break when we remove it.

::: devtools/client/inspector/rules/new-rules.js
@@ +127,5 @@
> +   *
> +   * @return {HighlighterOverlay}.
> +   */
> +  get highlighters() {
> +    if (!this._highlighters) {

Why do we need to set `this._highlighters`? 

This logic is similar to what's going on in `inspector.js`:

https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/devtools/client/inspector/inspector.js#192-197

So returning directly `this.inspector.highlighters` would go through the process of getting it lazily from the inspector and caching into `this._highlighters` in `inspector.js`

@@ +151,5 @@
> +      return this._selectorHighlighter;
> +    }
> +
> +    try {
> +      const front = this.inspector.inspector;

I'm not sure how to review this. Yulia did a recent refactoring in the Changes panel with regards to accessing fronts to avoid some of the race condition pitfalls we had:

https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/devtools/client/inspector/changes/ChangesView.js#51-61

I will ask for her review on this particular bit of code to make sure we're following the recommended way to access fronts.

@@ +246,5 @@
> +        selector,
> +      });
> +
> +      this.highlighters.selectorHighlighterShown = selector;
> +      this.emit("ruleview-selectorhighlighter-toggled", true);

This event seems to only be used by tests now. Perhaps add a comment here and on the other `emit()` so we know that, if we refactor the tests to rely on Redux state, this event and the emitter can be removed.

Comment 4

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

Attachment: 1520389.patch [2.0]

Comment 5

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

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

Comment on attachment 9036822 [details] [diff] [review]
1520389.patch [1.0]

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

::: devtools/client/inspector/rules/components/SelectorHighlighter.js
@@ +24,5 @@

• constructor(props) {
• super(props);
• this.state = {

•  // An unique selector to the current Rule. This is used to check if the
  

s/An/A

@@ +25,5 @@

• super(props);
• this.state = {

•  // An unique selector to the current Rule. This is used to check if the
  

•  // selector highlighter matches the highlighted selector and should be highlighted.
  

This comment is a bit of a tongue twister :)
Suggestion: A unique selector to the current Rule. This is checked against
the value of any existing highlighted selector in order mark the toggled
state of the component.

::: devtools/client/inspector/rules/models/rule.js
@@ +189,5 @@

• async getUniqueSelector() {
• let selector = "";
• if (this.domRule.selectors) {

•  // This is a "normal" rule with a selector.
  

s/"normal"/style

@@ +197,5 @@

•  // selector from the node which rule this is inherited from.
  

•  selector = await this.inherited.getUniqueSelector();
  

• } else {

•  // This is an inline style from the current node.
  

•  selector = this.elementStyle.ruleView.inspector.selectionCssSelector;
  

This is making a reference to the old (current) Rule view. It will break
when we remove it.

We should be ok since we pass the reference to the new Rules view in ElementStyle. https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/new-rules.js#162.

We can still mitigate this in the future by ensuring we have a reference to the inspector in ElementStyle as well.

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

• • @return {HighlighterOverlay}.
• */
• get highlighters() {
• if (!this._highlighters) {

Why do we need to set this._highlighters?

This logic is similar to what's going on in inspector.js:

https://searchfox.org/mozilla-central/rev/
bee8cf15c901b9f4b0c074c9977da4bbebc506e3/devtools/client/inspector/inspector.
js#192-197

So returning directly this.inspector.highlighters would go through the
process of getting it lazily from the inspector and caching into
this._highlighters in inspector.js

@@ +151,5 @@

•  return this._selectorHighlighter;
  

• }
• try {

•  const front = this.inspector.inspector;
  

I'm not sure how to review this. Yulia did a recent refactoring in the
Changes panel with regards to accessing fronts to avoid some of the race
condition pitfalls we had:

https://searchfox.org/mozilla-central/rev/
bee8cf15c901b9f4b0c074c9977da4bbebc506e3/devtools/client/inspector/changes/
ChangesView.js#51-61

I will ask for her review on this particular bit of code to make sure we're
following the recommended way to access fronts.

This bit on the SelectorHighlighter was refactored by Yulia in Bug 1508660.
https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/rules.js#265-267

@@ +246,5 @@

•    selector,
  

•  });
  

•  this.highlighters.selectorHighlighterShown = selector;
  

•  this.emit("ruleview-selectorhighlighter-toggled", true);
  

This event seems to only be used by tests now. Perhaps add a comment here
and on the other emit() so we know that, if we refactor the tests to rely
on Redux state, this event and the emitter can be removed.

Comment 6

[Yulia Startsev [:yulia]]

Comment on attachment 9037123 [details] [diff] [review]
1520389.patch [2.0]

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

I took a look. The front access looks fine for now -- we do the same in a few spots in the codebase.

::: devtools/client/inspector/rules/new-rules.js
@@ +147,5 @@
> +      return this._selectorHighlighter;
> +    }
> +
> +    try {
> +      const front = this.inspector.inspector;

this looks fine as a way to get the front. 

If you have a target present, you can get the front via `target.getCachedFront(frontName)` but the inspector is a special case due to its destruction path. At the moment we have a separate function `target.getInspector()`. 

But since no target is in this code, and you are passing the front via the inspector object, its fine to use it. We should at some point rename it to inspectorFront but there hasn't been time to do that yet.

Comment 7

[Razvan Caliman [:rcaliman]]

Comment on attachment 9037123 [details] [diff] [review]
1520389.patch [2.0]

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

Thank you, Yulia, for the extra review! And thank you, Gabriel, for the clarifications & fixes!

::: devtools/client/inspector/rules/new-rules.js
@@ +92,5 @@
>        this.elementStyle.destroy();
>      }
>  
>      this._dummyElement = null;
> +    this._highlighters = null;

`this._highlighters` is no longer used.

Comment 8

[Pulsebot]

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

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/55fc0688ea18