[rule view] Filter styles should highlight computed styles

RESOLVED FIXED in Firefox 40

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: gl, Assigned: gl)

Tracking

unspecified
Firefox 40
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(6 attachments, 5 obsolete attachments)

No description provided.
Assignee: nobody → gabriel.luong
Status: NEW → ASSIGNED
Attachment #8596487 - Flags: review?(bgrinstead)
Posted patch 1157293.patch [1.0] (obsolete) — Splinter Review
Attachment #8596487 - Attachment is obsolete: true
Attachment #8596709 - Flags: review?(bgrinstead)
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.
Attachment #8596709 - Flags: review?(bgrinstead)
Posted patch 1157293.patch [2.0] (obsolete) — Splinter Review
Attachment #8596709 - Attachment is obsolete: true
Attachment #8596807 - Flags: review?(bgrinstead)
Posted image Screenshot
Posted image 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?
Posted image alignment-issues.png
Alignment issues with the node.  See how the checkbox is higher than the other elements.  Is this a current problem?
(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
Posted patch 1157293.patch [3.0] (obsolete) — Splinter Review
Only expand the computed list if it doesn't matches the property.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=edc3d1b878d6
Attachment #8596807 - Attachment is obsolete: true
Attachment #8596807 - Flags: review?(bgrinstead)
Attachment #8597628 - Flags: review?(bgrinstead)
Attachment #8597628 - Attachment description: 1157293.patch [3.0 → 1157293.patch [3.0]
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?
Attachment #8597628 - Flags: review?(bgrinstead) → feedback+
Attachment #8597628 - Attachment is obsolete: true
Attachment #8600670 - Flags: review?(bgrinstead)
Attachment #8600671 - Flags: review?(bgrinstead)
Attachment #8600672 - Flags: review?(bgrinstead)
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!
Attachment #8600670 - Flags: review?(bgrinstead) → review+
Attachment #8600671 - Flags: review?(bgrinstead) → review+
Attachment #8600672 - Flags: review?(bgrinstead) → review+
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2953471&repo=fx-team
Flags: needinfo?(gabriel.luong)
(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.
Depends on: 1158506
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.