1167669 - [rule view] Rules and properties that are newly added or modified should be highlighted if it matches the style filter

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

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

Currently, the filter styles does not apply a highlight to rules and properties that are newly added or modified and matches the filter style search value.

Comment 1

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

Attachment: Part 1: Refactor CssRuleView highlighting of rule selector, property and stylesheet source to RuleEditor [1.0]

Comment 2

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

Attachment: Part 1: Refactor CssRuleView highlighting of rule selector, property and stylesheet source to RuleEditor and TextPropertyEditor [2.0]

Summary of Changes:
- Broke apart CssRuleView.highlightRules and moved the logical components for highlighting selectors and stylesheet source into RuleEditor and highlighting properties and computed properties to TextPropertyEditor
- Removed isValidSearchTerm trim check since we should allow people to search for " " if they wanted.
- Added this.ruleView reference to TextPropertyEditor

Comment 3

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

Attachment: Part 2: Highlight new filter style matches when rules and properties are added or modified [1.0]

Comment 4

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

Attachment: Part 3: Refactor this.ruleEditor.ruleView to this.ruleView in TextPropertyEditor [1.0]

Comment 5

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

Attachment: Part 2: Highlight new filter style matches when rules and properties are added or modified [2.0]

Comment 6

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

Attachment: Part 3: Refactor this.ruleEditor.ruleView to this.ruleView in TextPropertyEditor [2.0]

Comment 7

[Brian Grinstead [:bgrins]]

Looks like this might interact with the work going on in Bug 1164343 - should we mark one as blocking the other?  I'm assuming it'd be easier for you to rebase on top of that when it's all said and done than vice versa?

Comment 8

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

(In reply to Brian Grinstead [:bgrins] from comment #7)
> Looks like this might interact with the work going on in Bug 1164343 -
> should we mark one as blocking the other?  I'm assuming it'd be easier for
> you to rebase on top of that when it's all said and done than vice versa?

I applied both changes, and the overlaps are very little. The only reject that I saw was for the new getter for the searchValue which is just a 4 line rebase. This would be safe to review.

Comment 9

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

Attachment: Part 4: Add unit tests for highlighting new filter style matches when rule properties are added and modified [1.0]

Comment 10

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

ttps://treeherder.mozilla.org/#/jobs?repo=try&revision=4db2a7c48190

Comment 11

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

Attachment: Part 4: Add unit tests for highlighting new filter style matches when rule properties are added and modified [2.0]

Comment 12

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

Attachment: Part 1: Refactor CssRuleView highlighting of rule selector, property and stylesheet source to RuleEditor and TextPropertyEditor [3.0]

Comment 13

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

Attachment: Part 2: Highlight new filter style matches when rules and properties are added or modified [3.0]

Comment 14

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

Attachment: Part 3: Refactor this.ruleEditor.ruleView to this.ruleView in TextPropertyEditor [3.0]

Comment 15

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

Attachment: Part 4: Add unit tests for highlighting new filter style matches when rule properties are added and modified [3.0]

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8634849 [details] [diff] [review]
Part 1: Refactor CssRuleView highlighting of rule selector, property and stylesheet source to RuleEditor and TextPropertyEditor [3.0]

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

This is a general comment about parts 1 and 2.  Although I agree the search matching behavior for a rule 'belongs' to the editors, I'm not a big fan of splitting the one nasty function into these functions across the file.  Mostly because (a) we could at least before point to one place that all search stuff was happening and (b) we end up having to reach up to the rule view from the editors to do things like get the current search value, add highlight classes, clear the 'no-match' class, etc.

Of those, (b) is of more concern.  I think I'd be most happy if we kept the bulk of the logic of _highlightRuleProperty, _highlightComputedProperty in the editor, but renamed it to _getMatchingRuleProperty and _getMatchingComputedProperty, then those would return arrays of elements that matched and the rule view would be in charge of actually adding the class, maintaining the state of the search box, etc.  This would especially help I think when part 2 starts to introduce more ruleview specific functionality into the TPE.  So it would look something like this:

  let matchedRules = textProp.editor._getMatchingRuleProperty(this.searchPropertyName, this.searchPropertyValue, this.searchPropertyMatch);
  let isPropertyHighlighted = matchedRules.length > 0;

  for (let rule of matchedRules) {
    rule.classList.add("ruleview-highlight");
  }

We could do similar things for all of the _highlight* functions introduced in the refactor and maybe even keep one big list of things that need their classList updated and do all the updates at once to limit the amount of style recalc.

Alternatively, I'd be fine with splitting things up as you propose, but keeping _highlightRuleProperty, _highlightComputedProperty, _highlightMatches, and then _updateRulePropertyHighlight, _clearRulePropertyHighlights on the rule view and take a rule in as an argument.  Personally I think it makes more sense to have the ruleview manage the TPE than have the TPE manage the rule view.

Thanks for taking this on - this is touching some complicated parts of the rule view so I just want to make sure we are on the same page before proceeding.  Let's chat if you disagree or want further clarification.

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8634850 [details] [diff] [review]
Part 2: Highlight new filter style matches when rules and properties are added or modified [3.0]

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

See comments from part 1

::: browser/devtools/styleinspector/rule-view.js
@@ +2987,5 @@
> +
> +    // Highlight the new property if it matches the current search value
> +    // and remove the no search match indicator from the search field if
> +    // a property or computed property was highlighted
> +    if (this._updateRulePropertyHighlight() &&

This is a particular case where it'd be nice to say this.ruleView.highlightRule(this) or similar, and then can get any search box logic out of this object.

@@ +3069,5 @@
>    /**
> +   * Clear the rule property highlight for the current TextProperty.
> +   */
> +  _clearRulePropertyHighlights: function() {
> +    for (let element of this.element.querySelectorAll(".ruleview-highlight")) {

This another case where I'd prefer to see the this.ruleView._editorsExpandedForFilter logic handled from within the rule view.

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8634852 [details] [diff] [review]
Part 3: Refactor this.ruleEditor.ruleView to this.ruleView in TextPropertyEditor [3.0]

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

LGTM

Comment 19

[Brian Grinstead [:bgrins]]

Comment on attachment 8634853 [details] [diff] [review]
Part 4: Add unit tests for highlighting new filter style matches when rule properties are added and modified [3.0]

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

Test cases and overall functionality looks good with the patches applied

Comment 20

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

Attachment: Part 1: Refactor the logic for highlighting rule selector, property and stylesheet source [4.0]

Comment 21

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

Attachment: Part 2: Highlight new filter style matches when rules and properties are added or modified [4.0]

Comment 22

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

Attachment: Part 3: Refactor this.ruleEditor.ruleView to this.ruleView in TextPropertyEditor [4.0]

Comment 23

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

Attachment: Part 4: Add unit tests for highlighting new filter style matches when rule properties are added and modified [4.0]

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8636276 [details] [diff] [review]
Part 1: Refactor the logic for highlighting rule selector, property and stylesheet source [4.0]

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

LGTM - r=me with a green try push

Comment 25

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

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

Comment 26

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

Attachment: Part 4: Add unit tests for highlighting new filter style matches when rule properties are added and modified [5.0]

Comment 27

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

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

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/22db49def94f
https://hg.mozilla.org/integration/fx-team/rev/523e9b169dc5
https://hg.mozilla.org/integration/fx-team/rev/efcc530c4364
https://hg.mozilla.org/integration/fx-team/rev/80a6536f6a54

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22db49def94f
https://hg.mozilla.org/mozilla-central/rev/523e9b169dc5
https://hg.mozilla.org/mozilla-central/rev/efcc530c4364
https://hg.mozilla.org/mozilla-central/rev/80a6536f6a54