Closed Bug 1151943 Opened 9 years ago Closed 9 years ago

[Rule View] Add a search button next to overridden properties to filter for similar properties

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox43 fixed, relnote-firefox 43+)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed
relnote-firefox --- 43+

People

(Reporter: janx, Assigned: gl)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [polish-backlog][difficulty=medium])

Attachments

(5 files, 15 obsolete files)

16.00 KB, image/png
Details
105.12 KB, image/png
Details
252.46 KB, image/png
shorlander
: ui-review-
Details
3.04 KB, patch
pbro
: review+
Details | Diff | Splinter Review
10.40 KB, patch
Details | Diff | Splinter Review
A common source of frustration in CSS development is to see that a rule you added is simply ignored, and in the Rules View you see that it was overwritten (shown strike-through).

When hovering on the overwritten rule name, it would be nice to show a tooltip like "Overwritten by div.more-specific-selector in other-file-you-forgot-about.css:55".
See Also: → 1151956
Added concept image.
Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
Whiteboard: [devedition-40] → [devedition-40][difficulty=medium]
Assignee: nobody → gabriel.luong
Attached patch 1151943.patch (obsolete) — Splinter Review
Attached patch 1151943.patch (obsolete) — Splinter Review
Attachment #8625583 - Attachment is obsolete: true
Patrick, do you have any thoughts about how the tooltip should look. Currently, I am thinking we should style the tooltip similar to the rule editor including the source link. We want to think about reusing the tooltip for the box model tooltip. One idea is to have an information badge that opens up the tooltip and would allow you to click on the source link and a search icon inside the tooltip to filter the styles.
Flags: needinfo?(pbrosset)
Attached patch 1151943.patch (obsolete) — Splinter Review
Attachment #8625864 - Attachment is obsolete: true
(In reply to Gabriel Luong [:gl] from comment #5)
> Patrick, do you have any thoughts about how the tooltip should look.
> Currently, I am thinking we should style the tooltip similar to the rule
> editor including the source link. We want to think about reusing the tooltip
> for the box model tooltip. One idea is to have an information badge that
> opens up the tooltip and would allow you to click on the source link and a
> search icon inside the tooltip to filter the styles.
Hmm, I'm wondering if the tooltip is needed at all in fact.
I like the idea of having an icon next to overridden rules that, when clicked, runs a search in the rule-view that ends up highlighting all similar properties.

One of the most important role of the rule-view is to display rules in the right order, and so if you filter out everything else than the given property, you'd end up with an ordered list of rules, from the most specific to the least specific. Wouldn't this be enough?

I'm not too keen on having a tooltip in the rule-view that looks like the rule-view.

I think the same mechanism could be used in the box-model view too.
I find this more powerful too somehow.
Flags: needinfo?(pbrosset)
Attached patch 1151943.patch (obsolete) — Splinter Review
Attachment #8628699 - Attachment is obsolete: true
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #7)
> Created attachment 8629305 [details]
> Using the search box to provide the rule overriding information.png
> 
> (In reply to Gabriel Luong [:gl] from comment #5)
> > Patrick, do you have any thoughts about how the tooltip should look.
> > Currently, I am thinking we should style the tooltip similar to the rule
> > editor including the source link. We want to think about reusing the tooltip
> > for the box model tooltip. One idea is to have an information badge that
> > opens up the tooltip and would allow you to click on the source link and a
> > search icon inside the tooltip to filter the styles.
> Hmm, I'm wondering if the tooltip is needed at all in fact.
> I like the idea of having an icon next to overridden rules that, when
> clicked, runs a search in the rule-view that ends up highlighting all
> similar properties.
> 
> One of the most important role of the rule-view is to display rules in the
> right order, and so if you filter out everything else than the given
> property, you'd end up with an ordered list of rules, from the most specific
> to the least specific. Wouldn't this be enough?
> 
> I'm not too keen on having a tooltip in the rule-view that looks like the
> rule-view.
> 
> I think the same mechanism could be used in the box-model view too.
> I find this more powerful too somehow.

Patrick, that is a great idea. I really liked how it turned out with just filtering on the properties. I have updated the patch to only add the search button next to an overridden property.
Summary: [Rule View] For overwritten rules, show which rule took priority → [Rule View] Add a search button next to overridden properties to filter for similar properties
Attached patch 1151943.patch [1.0] (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a86e399d09fd
Attachment #8629853 - Attachment is obsolete: true
Attachment #8630153 - Flags: review?(pbrosset)
Something was try on the previous try push. New one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3ff842853ab
Something was wrong on try in the previous try push*
Attached image Screenshot
shorlander, would you recommend a better icon for the button or button interaction for the search button you see next to font-size? Possibly adding a hover state similar to the clear button.

The way it works is that the search button appears next to a rule property that is overridden, and you can click on the search button to filter for that property value in the rule view.
Attachment #8630586 - Flags: ui-review?(shorlander)
Attachment #8630153 - Flags: review?(bgrinstead)
Comment on attachment 8630153 [details] [diff] [review]
1151943.patch [1.0]

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

This is only a quick review as I'm not able to do a full one right now, nor test the patch locally. I see Brian is flagged for review too, which is good.
I like it. I only made a few comments.

::: browser/devtools/styleinspector/rule-view.js
@@ +3229,5 @@
> +    this.overriddenSearch.addEventListener("click", event => {
> +      event.stopPropagation();
> +      this.ruleEditor.ruleView.searchField.value = this.prop.name;
> +      this.ruleEditor.ruleView.searchField.focus();
> +      this.ruleEditor.ruleView._onFilterStyles();

It looks like doing a rule-view search programmatically is something that might turn out very useful, could you extract this into a nice public method on CssRuleView so we don't have to repeat this in the future?

::: browser/themes/shared/devtools/ruleview.css
@@ +4,5 @@
>  
>  /* CSS Variables specific to this panel that aren't defined by the themes */
>  .theme-light {
>    --rule-highlight-background-color: #ffee99;
> +  --rule-filter-icon: url(magnifying-glass-light.png);

I think we should only be adding SVG images now. But Brian will know better than me.

::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties
@@ +64,5 @@
>  # the title attribute of the warning icon.
>  rule.warning.title=Invalid property value
>  
> +# LOCALIZATION NOTE (rule.overriddenRule.title): This text is used for
> +# the title attribute of the search overrideden rule icon.

s/overrideden/overridden
Also maybe this comment should be more self-explanatory. For someone who doesn't know the feature, it might not be enough to understand exactly what this string is supposed to mean.

@@ +65,5 @@
>  rule.warning.title=Invalid property value
>  
> +# LOCALIZATION NOTE (rule.overriddenRule.title): This text is used for
> +# the title attribute of the search overrideden rule icon.
> +rule.overriddenRule.title=Find the rule that took priority

Well, the feature doesn't really find the rule, it highlights all occurrences of the property in applied rules. So the English string should say it.
Attachment #8630153 - Flags: review?(pbrosset) → feedback+
Attached patch 1151943.patch [2.0] (obsolete) — Splinter Review
Addressed feedback from pbro. The current icon used is based on the icon in the filter styles searchbar. Currently, waiting for more UI feedback from shorlander and this will probably changed in the future.

Any thoughts about scrolling to the top when we click on this search button?
Attachment #8630153 - Attachment is obsolete: true
Attachment #8630153 - Flags: review?(bgrinstead)
Attachment #8630774 - Flags: review?(bgrinstead)
Attached patch 1151943.patch [3.0] (obsolete) — Splinter Review
Previous patch used the old variable name for the overriddenSearch button in the test before I renamed it. Fixed now
Attachment #8630774 - Attachment is obsolete: true
Attachment #8630774 - Flags: review?(bgrinstead)
Attachment #8631323 - Flags: review?(bgrinstead)
Comment on attachment 8631323 [details] [diff] [review]
1151943.patch [3.0]

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

::: browser/devtools/styleinspector/test/browser_ruleview_search-filter-overridden-property.js
@@ +6,5 @@
> +
> +// Tests that the rule view overriden search filter works properly for
> +// overridden property values.
> +
> +const SEARCH = "00F";

nit : This variable is unused in this test

::: browser/themes/shared/devtools/ruleview.css
@@ +9,5 @@
>  }
>  
>  .theme-dark {
>    --rule-highlight-background-color: #594724;
> +  --rule-filter-icon:url(magnifying-glass.png);

nit : space after ":"

@@ +119,5 @@
>  }
>  
> +.ruleview-overridden-rule-filter {
> +  background-image: var(--rule-filter-icon);
> +  background-size: 13px 12px;

So this is stretching out the image, the background-size should probably be 11px 11px instead.

Also, this should get HDPI treatment

::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties
@@ +66,5 @@
>  
> +# LOCALIZATION NOTE (rule.filterProperty.title): Text displayed in the tooltip
> +# of the search button that is shown next to a property that has been overridden
> +# in the rule view.
> +rule.filterProperty.title=Filter for rules containing this property name

Maybe just "Filter rules" instead of "Filter for rules" ?
Attached patch 1151943.patch [4.0] (obsolete) — Splinter Review
Addressed ntim's comments. Thanks
Attachment #8631323 - Attachment is obsolete: true
Attachment #8631323 - Flags: review?(bgrinstead)
Attachment #8631791 - Flags: review?(bgrinstead)
Attached patch 1151943.patch [5.0] (obsolete) — Splinter Review
Attachment #8631791 - Attachment is obsolete: true
Attachment #8631791 - Flags: review?(bgrinstead)
Attachment #8631804 - Flags: review?(bgrinstead)
Comment on attachment 8630586 [details]
Screenshot

Looks good, I think the magnifying glass icon is good. It looks blurry though. I agree a hover state would be nice. Maybe just darker, or could encapsulate it in a circle to make it look more button-y.
Attachment #8630586 - Flags: ui-review?(shorlander) → ui-review-
(In reply to Stephen Horlander [:shorlander] from comment #21)
> Comment on attachment 8630586 [details]
> Screenshot
> 
> Looks good, I think the magnifying glass icon is good. It looks blurry
> though. I agree a hover state would be nice. Maybe just darker, or could
> encapsulate it in a circle to make it look more button-y.

Can you please convert the magnifying glass icon to an SVG?  This will also let us update the search boxes across the toolbox with it.  Here's what we have already:

mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/images/magnifying-glass-light.png
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/images/magnifying-glass-light@2x.png
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/images/magnifying-glass.png
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/images/magnifying-glass@2x.png

So I'm guessing we would need two SVGs - one for light and one for normal.  Or one SVG with both colors accessible from the URL, whichever you think is best.
Flags: needinfo?(shorlander)
Comment on attachment 8631804 [details] [diff] [review]
1151943.patch [5.0]

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

Cancelling review mostly because of ux stuff but the code looks good.  I'm glad that it reuses our existing search feature, so that if we want to make improvements to things it will also improve the normal search.  For instance, the scroll location is lost when clicking a mag glass and then pressing ESC - we may want to maintain the original position before a search starts and then restore it when the textbox becomes empty, which could be a general improvement for search.

The only issue I see with this feature is that it can include false positives in the 'overridden' results (like if the property that was overridden was 'color' then we will also see results that include 'background-color') but I think that's an OK tradeoff vs building and integrating a whole new UI.

I do think it would be smart to get this in front of a couple of web devs and see if they find it useful and see if it needs any changes.  Maybe also loop Jeff into this?

::: browser/themes/shared/devtools/ruleview.css
@@ +4,5 @@
>  
>  /* CSS Variables specific to this panel that aren't defined by the themes */
>  .theme-light {
>    --rule-highlight-background-color: #ffee99;
> +  --rule-filter-icon: url(magnifying-glass-light.png);

We either need to handle 2x icons using the existing @2x pngs in tree or wait for the SVG asset.  We also need some kind of hover state as shorlander mentions in Comment 21

::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties
@@ +66,5 @@
>  
> +# LOCALIZATION NOTE (rule.filterProperty.title): Text displayed in the tooltip
> +# of the search button that is shown next to a property that has been overridden
> +# in the rule view.
> +rule.filterProperty.title=Filter rules containing this property name

I think the correct name for what we are filtering for is just 'property' (no 'name' added).  Like how things are referred to as "The 'display' property" here: http://www.w3.org/TR/CSS2/

We should also have a note that the terms "rules" and "property" should not be translated since they are technical terms.
Attachment #8631804 - Flags: review?(bgrinstead)
Status: NEW → ASSIGNED
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c037bef15a75

Added a hover state, but it is very subtle unfortunately. The icon could be darker.
Attached patch 1151943.patch [6.0] (obsolete) — Splinter Review
Attachment #8631804 - Attachment is obsolete: true
@jeff: would you be able to show off the try build to some web developers to gather some more feedback? https://treeherder.mozilla.org/#/jobs?repo=try&revision=c037bef15a75

I talked to ally on irc on monday who was also looking for the ability to find the rule that took priority over the overridden property. We removed the tooltip in this iteration, but it seems we might also want to have some sort of tooltip still from the interaction I had with her.
Flags: needinfo?(jgriffiths)
One bug I've seen is that the icon shows for CSS variables which we don't currently handle properly.
(In reply to Gabriel Luong [:gl] from comment #26)
> @jeff: would you be able to show off the try build to some web developers to
> gather some more feedback?
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c037bef15a75
> 
> I talked to ally on irc on monday who was also looking for the ability to
> find the rule that took priority over the overridden property. We removed
> the tooltip in this iteration, but it seems we might also want to have some
> sort of tooltip still from the interaction I had with her.

My biggest concern is the false positive, for properties like 'color' you immediately get results you don't want ( eg background-color ). This is a case where the specific implementation

Stepping back, it feels like we're overloading search. The problem Jan originally stated is - a rule is overridden and the user wants to easily get to the currently applied rule instead. What we need is some way to visually indicate that the user can find out more about the currently applied rule, and some action that takes the user to the currently applied rule.

I don't think I'm in favor of using search in this way - if the user is looking at an overridden rule providing them with a bunch of other search results instead of taking them to the currently applied rule just feels like noise.
Removing needinfo, I think I'd like my concerns from #c28 addressed?
Flags: needinfo?(gabriel.luong)
Flags: needinfo?(jgriffiths)
(In reply to Jeff Griffiths (:canuckistani) from comment #29)
> Removing needinfo, I think I'd like my concerns from #c28 addressed?

I agree with the comments in #c28. Will try to look at this bug again sometime this week.
Flags: needinfo?(gabriel.luong)
Whiteboard: [devedition-40][difficulty=medium] → [polish-backlog][difficulty=medium]
Gabriel and I had a discussion about this feature and we agreed that the following were important:

- we should avoid false positives as Jeff noted,
- we need a better UX,
- searching for all occurrences of the overridden property in the rule-view for the current node is a pretty amazing way to find all inherited properties, and understand cascading and specificity.

So, overall, I'd like to keep using the search filter for this, but make a few changes:
- make sure that clicking on the search icon actually searches for the property in a "strict" mode whereby only properties with that name exactly would be highlighted (color would not match background-color, nor selectors with the word color, nor values with the word color in them),
- and maybe temporarily highlight the one property that did take precedence temporarily, to draw attention.

Gabriel said he would rebase his patch and try this out, it'd be far easier to get a feel for this with a try build.
Attached patch 1151943.patch [7.0] (obsolete) — Splinter Review
This does a strict search for the property name. So, we reduce the false positive on the search.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=915e2d7c1241
Attachment #8632318 - Attachment is obsolete: true
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #31)
> So, overall, I'd like to keep using the search filter for this, but make a
> few changes:
> - make sure that clicking on the search icon actually searches for the
> property in a "strict" mode whereby only properties with that name exactly
> would be highlighted (color would not match background-color, nor selectors
> with the word color, nor values with the word color in them),

Could the "strict" mode maybe be user-accessible as well via some search syntax?  Like "property:color", "`color`", or something.  Then we wouldn't end up with confusing behavior where it prefills the search box with "color", which has a special behavior, but then if you press space and backspace you end up seeing totally different results.
(In reply to Brian Grinstead [:bgrins] from comment #33)
> Could the "strict" mode maybe be user-accessible as well via some search
> syntax?  Like "property:color", "`color`", or something.  Then we wouldn't
> end up with confusing behavior where it prefills the search box with
> "color", which has a special behavior, but then if you press space and
> backspace you end up seeing totally different results.
Agreed, this is actually something we discussed with Gabriel. An idea was using a modifier character like in the debugger.
Attachment #8647969 - Attachment is obsolete: true
Attachment #8649697 - Flags: review?(pbrosset)
Attachment #8649698 - Flags: review?(pbrosset)
Comment on attachment 8649697 [details] [diff] [review]
Part 1: Add setter for filter style search value

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

Simple enough for a quick R+.
Do you think the search code in these 2 views could be put in common somehow (see bug 1164343)?
Attachment #8649697 - Flags: review?(pbrosset) → review+
Comment on attachment 8649698 [details] [diff] [review]
Part 2: Add strict search for filter styles

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

Love the feature!
I think it works really well. Yes, it does not just give you the property that took precedence, but I think it's much more powerful, it shows you all the rules involved in choosing which property took precedence, in the right order, and highlights what you need to know.
I especially like how it expands the long-hand properties if needed: https://dl.dropboxusercontent.com/u/714210/overide-filter.gif
One of the next steps could be showing selector specificity somewhere to explain users even more why a certain property took precedence, but I digress.

I'm less enthusiastic about the fact that you can use combined strict/unstrict searches, like "`strict name`: unstrict value".
I'm not against it, I just think that it's probably not going to be used very often at all. I don't see a use case where there would be so many rules and properties that it would be necessary to use this rather than just a simple search.
I guess another way to phrase this would be: if this adds significant complexity to the code on top of the overridden declaration feature, I think it'd be better to explore it in a separate bug and keep the code as simple as we can for now.

I have one major comment about the tests though. I see a whole lot of code duplications in these tests. Having a lot of small tests is preferable than few huge, un-maintainable, tests, but in this case, since they really just do the same thing over and over again, it's a perfect candidate for having just 1 test that has a big TEST_DATA array at the top that describes all the test cases to be ran, and then just one small test logic at the bottom that loops through them.

::: browser/devtools/styleinspector/rule-view.js
@@ +1614,1 @@
>        this.searchPropertyMatch = FILTER_PROP_RE.exec(this.searchValue);

I know you haven't changed that line in this patch, but I think I'd like Tom to take a look at it. We've removed most (all?) occurrences of regex-based parsing of CSS in the devtools in favor of using the CSS Lexer API. Parsing a property:value; string seems like the perfect job for css-parsing-utils.js:parseDeclarations.
I'm afraid there will be corner cases here that the regex doesn't take into account.
What about a property that has a : sign in it? I know this probably wouldn't happen though (e.g. prop\:erty:value;)

@@ +1647,5 @@
> +        let searchValue = FILTER_STRICT_RE.exec(this.searchValue)[1];
> +        this.strictSearchAllValues = true;
> +        this.searchPropertyName = searchValue;
> +        this.searchPropertyValue = searchValue;
> +        this.strictSearchValue = searchValue;

I'm not a huge fan of adding so many properties on 'this' here. It makes it harder to find things in 'this' when debugging. We should clean things up a bit here. One simple way would be to put all these properties into a single 'this.searchData' object or similar.
Another way would be to instantiate a new RuleViewSearch class in the RuleView constructor to which we would delegate all search-related tasks. This way removing from RuleView a bunch of search and highlight code (could this also help put the computed-view and rule-view search code in common, perhaps?)

@@ +2343,5 @@
>     *         otherwise.
>     */
>    _highlightMatches: function(element, { searchName, searchValue, propertyName,
> +      propertyValue, propertyMatch, strictSearchName, strictSearchValue,
> +      strictSearchAllValues }) {

If I'm not mistaken, _highlightMatches is on the same object than the methods that call it, so why does it need to take all these arguments? Couldn't it just read the property on this.searchData?
Or, if you refactor all this in a separate class, that would simplify it too.
Attachment #8649698 - Flags: review?(pbrosset)
Comment on attachment 8649710 [details] [diff] [review]
Part 3: Add a search button next to overridden properties to filter for similar properties

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

Just of few minor comments on this one.

::: browser/devtools/styleinspector/rule-view.js
@@ +3050,5 @@
>        class: "ruleview-warning",
>        hidden: "",
>        title: CssLogic.l10n("rule.warning.title"),
>      });
> +    // Filter button that filters for the current property name and is

nit: please insert a new line before this comment to separate things out better.

::: browser/devtools/styleinspector/test/browser_ruleview_search-filter-overridden-property.js
@@ +53,5 @@
> +  textPropEditor.filterProperty.click();
> +  yield onRuleViewFiltered;
> +
> +  info("Check that the overridden search is applied");
> +  is(searchField.value, "width", "The search field value is width.");

Shouldn't the value be "`width`" instead?

::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties
@@ +64,5 @@
>  rule.warning.title=Invalid property value
> +# LOCALIZATION NOTE (rule.filterProperty.title): Text displayed in the tooltip
> +# of the search button that is shown next to a property that has been overridden
> +# in the rule view. Note "rules" and "property" should not be translated since
> +# they are technical terms.

Well words like these have already been translated. I have the French version here and the word "Property" is translated to "Propriete" there. So I'm not sure this part of the comment is needed.
Attachment #8649710 - Flags: review?(pbrosset) → review+
Keywords: leave-open
Attachment #8649697 - Flags: checkin+
Depends on: 1197048
Attachment #8649698 - Attachment is obsolete: true
Attachment #8650180 - Attachment is obsolete: true
Attachment #8650822 - Flags: review+
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #38)
> Comment on attachment 8649697 [details] [diff] [review]
> Part 1: Add setter for filter style search value
> 
> Review of attachment 8649697 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Simple enough for a quick R+.
> Do you think the search code in these 2 views could be put in common somehow
> (see bug 1164343)?

There is a bug to create a searchbox widget https://bugzilla.mozilla.org/show_bug.cgi?id=1154789
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #39)
> Comment on attachment 8649698 [details] [diff] [review]
> Part 2: Add strict search for filter styles
> 
> Review of attachment 8649698 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Love the feature!
> I think it works really well. Yes, it does not just give you the property
> that took precedence, but I think it's much more powerful, it shows you all
> the rules involved in choosing which property took precedence, in the right
> order, and highlights what you need to know.
> I especially like how it expands the long-hand properties if needed:
> https://dl.dropboxusercontent.com/u/714210/overide-filter.gif
> One of the next steps could be showing selector specificity somewhere to
> explain users even more why a certain property took precedence, but I
> digress.
> 
> I'm less enthusiastic about the fact that you can use combined
> strict/unstrict searches, like "`strict name`: unstrict value".
> I'm not against it, I just think that it's probably not going to be used
> very often at all. I don't see a use case where there would be so many rules
> and properties that it would be necessary to use this rather than just a
> simple search.
> I guess another way to phrase this would be: if this adds significant
> complexity to the code on top of the overridden declaration feature, I think
> it'd be better to explore it in a separate bug and keep the code as simple
> as we can for now.

I have split up the strict search feature to Bug 1197048. Due to the way the search value is currently parsed, we would get the strict/unstrict searches as a side effect. Strict/unstrict searches probably will not see a lot of uses, but the input for strict/unstrict searches needed to be handled.

> 
> I have one major comment about the tests though. I see a whole lot of code
> duplications in these tests. Having a lot of small tests is preferable than
> few huge, un-maintainable, tests, but in this case, since they really just
> do the same thing over and over again, it's a perfect candidate for having
> just 1 test that has a big TEST_DATA array at the top that describes all the
> test cases to be ran, and then just one small test logic at the bottom that
> loops through them.
I have removed some of the code duplications in Bug 1197048.
> 
> ::: browser/devtools/styleinspector/rule-view.js
> @@ +1614,1 @@
> >        this.searchPropertyMatch = FILTER_PROP_RE.exec(this.searchValue);
> 
> I know you haven't changed that line in this patch, but I think I'd like Tom
> to take a look at it. We've removed most (all?) occurrences of regex-based
> parsing of CSS in the devtools in favor of using the CSS Lexer API. Parsing
> a property:value; string seems like the perfect job for
> css-parsing-utils.js:parseDeclarations.
> I'm afraid there will be corner cases here that the regex doesn't take into
> account.
> What about a property that has a : sign in it? I know this probably wouldn't
> happen though (e.g. prop\:erty:value;)
> 

There was a reason we didn't use parseDeclarations in the original filter styles implementation because it would not handle cases such as ":3px", and tell us the name is empty and the value is 3px.

> @@ +1647,5 @@
> > +        let searchValue = FILTER_STRICT_RE.exec(this.searchValue)[1];
> > +        this.strictSearchAllValues = true;
> > +        this.searchPropertyName = searchValue;
> > +        this.searchPropertyValue = searchValue;
> > +        this.strictSearchValue = searchValue;
> 
> I'm not a huge fan of adding so many properties on 'this' here. It makes it
> harder to find things in 'this' when debugging. We should clean things up a
> bit here. One simple way would be to put all these properties into a single
> 'this.searchData' object or similar.
> Another way would be to instantiate a new RuleViewSearch class in the
> RuleView constructor to which we would delegate all search-related tasks.
> This way removing from RuleView a bunch of search and highlight code (could
> this also help put the computed-view and rule-view search code in common,
> perhaps?)
> 

Fixed with using this.searchData in Bug 1197048

> @@ +2343,5 @@
> >     *         otherwise.
> >     */
> >    _highlightMatches: function(element, { searchName, searchValue, propertyName,
> > +      propertyValue, propertyMatch, strictSearchName, strictSearchValue,
> > +      strictSearchAllValues }) {
> 
> If I'm not mistaken, _highlightMatches is on the same object than the
> methods that call it, so why does it need to take all these arguments?
> Couldn't it just read the property on this.searchData?
> Or, if you refactor all this in a separate class, that would simplify it too.

Fixed in Bug 1197048
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #40)
> Comment on attachment 8649710 [details] [diff] [review]
> Part 3: Add a search button next to overridden properties to filter for
> similar properties
> 
> Review of attachment 8649710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just of few minor comments on this one.
> 
> ::: browser/devtools/styleinspector/rule-view.js
> @@ +3050,5 @@
> >        class: "ruleview-warning",
> >        hidden: "",
> >        title: CssLogic.l10n("rule.warning.title"),
> >      });
> > +    // Filter button that filters for the current property name and is
> 
> nit: please insert a new line before this comment to separate things out
> better.
> 

There is a new line but just not being show on the review page for some reason.

> :::
> browser/devtools/styleinspector/test/browser_ruleview_search-filter-
> overridden-property.js
> @@ +53,5 @@
> > +  textPropEditor.filterProperty.click();
> > +  yield onRuleViewFiltered;
> > +
> > +  info("Check that the overridden search is applied");
> > +  is(searchField.value, "width", "The search field value is width.");
> 
> Shouldn't the value be "`width`" instead?
> 

Fixed.

> ::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties
> @@ +64,5 @@
> >  rule.warning.title=Invalid property value
> > +# LOCALIZATION NOTE (rule.filterProperty.title): Text displayed in the tooltip
> > +# of the search button that is shown next to a property that has been overridden
> > +# in the rule view. Note "rules" and "property" should not be translated since
> > +# they are technical terms.
> 
> Well words like these have already been translated. I have the French
> version here and the word "Property" is translated to "Propriete" there. So
> I'm not sure this part of the comment is needed.

Removed the note.
Keywords: leave-open
Comment on attachment 8650877 [details] [diff] [review]
Part 2: Add a search button next to overridden properties to filter for similar properties [3.0]

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

::: browser/themes/shared/devtools/ruleview.css
@@ +9,5 @@
>  }
>  
>  .theme-dark {
>    --rule-highlight-background-color: #594724;
> +  --rule-filter-icon: url(magnifying-glass.png);

Noticed this a bit late, but this will need HDPI support.
https://hg.mozilla.org/mozilla-central/rev/ecec265ae5f5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Release Note Request (optional, but appreciated)
[Why is this notable]: Easily find the rule that overridden the property
[Suggested wording]:
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Flags: needinfo?(shorlander)
Suggested wording: Search for similar properties button in Dev Tools Inspector

If you have improvements for the release note wording, please comment and needinfo me. Thanks!
I've added some docs on this:

https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#Overridden_declarations

Does it look OK Gabriel?
Flags: needinfo?(gabriel.luong)
Liz, maybe something like: 
"Button to show the rule that overrode a given CSS property." (?)
Flags: needinfo?(lhenry)
(In reply to Will Bamberg [:wbamberg] from comment #56)
> Liz, maybe something like: 
> "Button to show the rule that overrode a given CSS property." (?)

We have something similar in the relnotes already: https://www.mozilla.org/en-US/firefox/43.0a2/auroranotes/
Flags: needinfo?(lhenry)
(In reply to Will Bamberg [:wbamberg] from comment #55)
> I've added some docs on this:
> 
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> Examine_and_edit_CSS#Overridden_declarations
> 
> Does it look OK Gabriel?

Looks good, Thanks!
Flags: needinfo?(gabriel.luong)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.