1157293 - [rule view] Filter styles should highlight computed styles

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Comment 1

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

Attachment: 1157293.patch

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

Comment 2

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

Attachment: 1157293.patch [1.0]

Comment 3

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

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

Comment 4

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

Just a couple of notes on the changes:
* Adds highlighting for the properties in the computed list (eg, the computed properties for 'margin: 4px 0px')
* A new container (.ruleview-propertycontainer) was needed to separate the main property line from the computed list, otherwise the computed list is highlighted by default if the property is highlighted.
* Renamed .ruleview-propertycontainer to .ruleview-propertyvaluecontainer
* Removed this._highlightedElements array that kept track of all the highlighted elements. Instead we do a query of the doc for .ruleview-highlight
* We need to keep track of the expander and whether or not the computed list is visible. I used a new class .filter-open to keep track of whether or not the computed list was toggled opened by the filter. This is required to maintain the behavior for state #2. There are 2 states to consider:
*** 1) Computed list is not visible and search term matches something in the computed list. Open the computed list and expander. When the search filter is cleared, the computed list should be closed along with the expander.
*** 2) Computed list is already opened by the user. Properties should be highlighted as usual, but when the search filter is cleared, the computed list should remain open.
* Added a helper function _highlightMatches that takes in the element to be highlighted, and the search and property parameters to match on. This was added since the logic is reused for property and computed property matches.

Comment 5

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

Attachment: 1157293.patch [2.0]

Comment 6

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

Attachment: Screenshot

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: back-search.png

My 2 cents - in this case I don't think that we should expand a non-expanded node.  I've searched 'back' which matched the non-"computed" (existing bad choice of terminology) property as well as all the computed properties.  If this happens I think expanding it is actually distracting and makes it harder to scan the results.

If a search term *doesn't* match the non-computed but does match one or more computed values then expand away.  Thoughts?

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: alignment-issues.png

Alignment issues with the node.  See how the checkbox is higher than the other elements.  Is this a current problem?

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> Created attachment 8597497 [details]
> back-search.png
> 
> My 2 cents - in this case I don't think that we should expand a non-expanded
> node.  I've searched 'back' which matched the non-"computed" (existing bad
> choice of terminology) property as well as all the computed properties.  If
> this happens I think expanding it is actually distracting and makes it
> harder to scan the results.
> 
> If a search term *doesn't* match the non-computed but does match one or more
> computed values then expand away.  Thoughts?

This is also disruptive because if I collapse that rule and then type another character in, it re-expands.  That may be a case we want to handle anyway, although it may get messy

Comment 10

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

Attachment: 1157293.patch [3.0]

Only expand the computed list if it doesn't matches the property.

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

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8597628 [details] [diff] [review]
1157293.patch [3.0]

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

This looks really good.  I just have a few notes about the implementation, see below

::: browser/devtools/styleinspector/rule-view.js
@@ +2112,5 @@
> +      // Open the computed list if a computed rule is highlighted and the
> +      // property rule is not highlighted
> +      if (!isPropertyHighlighted && isComputedHighlighted &&
> +          !editor.computed.classList.contains("styleinspector-open")) {
> +        editor.computed.classList.add("filter-open");

This looks like a case where it'd better to keep state outside of the DOM and move specific logic into the TextPropertyEditor instance.  Rather than attaching _textPropertyEditor to the element and reaching up to get to editor and messing with it's DOM, here is what I propose:

First, add functions expandForFilter / collapseForFilter to the TPE right next to _onExpandClicked.  These would be something like this:

expandForFilter: function() {
  if (!this.computed.classList.contains("styleinspector-open")) {
    this.computed.classList.add("filter-open");
    editor.expander.setAttribute("open", "true");
  }
}

collapseForFilter: function() {
  this.computed.classList.remove("filter-open");
  if (!this.computed.classList.contains("styleinspector-open")) {
    editor.expander.removeAttribute("open");
  }
}

Then here you can call:

  editor.expandForFilter();
  this._editorsExpandedForFilter.push(editor);

And later in clearHighlight instead of querySelectorAll(".filter-open") and reaching back up to the _textPropertyEditor you could do:

  for (let editor of this._editorsExpandedForFilter) {
    editor.collapseForFilter();
  }
  this._editorsExpandedForFilter = [];


This moves logic that should be specific to the TPE into that object, and also gets rid of the fragile attachment of the object to it's DOM.

@@ +3021,5 @@
>      aEvent.stopPropagation();
>    },
>  
>    /**
>     * Handles clicks on the computed property expander.

This function could use an additional comment explaining the distinction between filter-open and styleinspector-open and the expected expand/collapse behavior wrt filtering

@@ +3024,5 @@
>    /**
>     * Handles clicks on the computed property expander.
>     */
>    _onExpandClicked: function(aEvent) {
> +    if (this.computed.classList.contains("filter-open") ||

Two observations that I'd like your thoughts on (and would fit in a separate patch here or in a follow up):

1) If we used [styleinspector-open] and [filter-open] as attributes instead of classes, it would make the code here and in the tests a bit more concise.  I know in the browser frontend, they have a convention to use attributes instead of classes when tracking state like this.

2) I styleinspector-open may be better named user-open or something indicating that it was caused by direct interaction.  What do you think?

::: browser/devtools/styleinspector/ruleview.css
@@ +56,4 @@
>    cursor: pointer;
>  }
>  
>  .ruleview-computedlist:not(.styleinspector-open),

Could you just add:

.ruleview-computedlist:not(.filter-open),

to this list instead of adding the new rule below?

Comment 12

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

Attachment: Part 1: Filter styles highlight computed styles [1.0]

Comment 13

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

Attachment: Part 2: Renamed styleinspector-open class to user-open [1.0]

Comment 14

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

Attachment: Part 3: Use filter-open and user-open attributes instead of classes [1.0]

Comment 15

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

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

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8600670 [details] [diff] [review]
Part 1: Filter styles highlight computed styles [1.0]

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

Ship it!

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/47a1d2e67785
https://hg.mozilla.org/integration/fx-team/rev/1ec520b96ede
https://hg.mozilla.org/integration/fx-team/rev/80fc4351eba3

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/47a1d2e67785
https://hg.mozilla.org/integration/fx-team/rev/1ec520b96ede
https://hg.mozilla.org/integration/fx-team/rev/80fc4351eba3

Comment 19

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2953471&repo=fx-team

Comment 20

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/7ef7668d87e2
https://hg.mozilla.org/integration/fx-team/rev/be5b6c988120
https://hg.mozilla.org/integration/fx-team/rev/462159286309

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Carsten Book [:Tomcat] from comment #19)
> sorry had to back this out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=2953471&repo=fx-team

Ah, browser_ruleview_search-filter_escape-keypress.js (Bug 1158506) was landed in the meantime.

Comment 22

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

Attachment: Part 1: Filter styles highlight computed styles [2.0]

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

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9d0bac2dcb79
https://hg.mozilla.org/integration/fx-team/rev/abe7a87f22f5
https://hg.mozilla.org/integration/fx-team/rev/5a2ffe1788ed

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9d0bac2dcb79
https://hg.mozilla.org/mozilla-central/rev/abe7a87f22f5
https://hg.mozilla.org/mozilla-central/rev/5a2ffe1788ed