672746 - Add an "Only user styles" checkbox to the style inspector

Status: VERIFIED FIXED

(DevTools :: General, defect, P1)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We need to add an "Include default styles" checkbox to the style inspector. This checkbox should toggle the display for css properties that do not have a stylesheet in the stylesheet column on / off.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Wireframe

Comment 2

[Kevin Dangoor]

I'm going to suggest that this is a P1 for minotaur. The Style Inspector is not very usable as it stands now because of information overload. I think we need at least one of this bug or bug 672744 (if not both) and just including bug 672744 would still present the user with an overwhelming amount of information on first open.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP

Storing my WIP for safe-keeping.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Working patch

This appears to work as advertised. It needs to be applied on top of the patches from bugs 683737, 683672, 674856:

$ hg qapplied
mochitest_timeout_683737.patch
incorrect_num_rules_683672.patch
do_not_redraw_ui_674856.patch
styleinspector-defaultstyles

Mike, can you give it a try and tell me if this is what we want?

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Include default styles patch 2

@panos, unfortunately my description of this feature was probably not clear enough and you have implemented a different feature. As part of adding the filter to the style inspector I have had to mostly implement this feature too so I have updated your patch accordingly ... hope you don't mind ;o)

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Include default styles patch 3

Oops, forgot winstripe etc.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 558779 [details] [diff] [review]
Include default styles patch 3

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

OK, that makes more sense actually. I was wondering whether we had another bug filed for not displaying style properties without any selector matches at all, but this solution takes care of that as well.

And it goes without saying that you are always welcome to do my work for me :-)

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ -232,0 +233,6 @@
> > +   * Called when the user changes the value of the "use default styles"
> > +   * checkbox.
> > +   *
> > +   * @param {Event} aEvent the DOM Event object.
NaN more ...

This method is redundant now, refreshPanel can be the event handler.

::: browser/devtools/styleinspector/test/browser/browser_styleinspector_bug_672746_default_styles.js
@@ +49,5 @@
> +{
> +  Services.obs.removeObserver(SI_check, "StyleInspector-populated", false);
> +  SI_CheckPropertiesForSpan();
> +  Services.obs.addObserver(finishUp, "StyleInspector-closed", false);
> +  stylePanel.hidePopup();

I think it is too early to hide the popup here, the StyleInspector-closed notification is racing with StyleInspector-populated in SI_toggleDefaultStyles. It would make more sense as the last statement in SI_checkDefaultStyles.

@@ +87,5 @@
> +  let propertyViews = stylePanel.cssHtmlTree.propertyViews;
> +  let className = propertyViews[aName].className;
> +  if (className == "property-view") {
> +    return true;
> +  } else if (className == "property-view-hidden") {

The else clause here is redundant and causes a warning from the JS engine.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Panos Astithas [:past] from comment #7)
> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ -232,0 +233,6 @@
> > > +   * Called when the user changes the value of the "use default styles"
> > > +   * checkbox.
> > > +   *
> > > +   * @param {Event} aEvent the DOM Event object.
> NaN more ...
> 
> This method is redundant now, refreshPanel can be the event handler.

NaN more? Thank you Splinter for borking my first review. I was referring to toggleDefaultStyles here.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Include default styles patch 3

(In reply to Panos Astithas [:past] from comment #7)
> Comment on attachment 558779 [details] [diff] [review]
> Include default styles patch 3
> 
> Review of attachment 558779 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, that makes more sense actually. I was wondering whether we had another
> bug filed for not displaying style properties without any selector matches
> at all, but this solution takes care of that as well.
> 
> And it goes without saying that you are always welcome to do my work for me
> :-)
> 
> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ -232,0 +233,6 @@
> > > +   * Called when the user changes the value of the "use default styles"
> > > +   * checkbox.
> > > +   *
> > > +   * @param {Event} aEvent the DOM Event object.
> NaN more ...
> 
> This method is redundant now, refreshPanel can be the event handler.
> 

Done

> :::
> browser/devtools/styleinspector/test/browser/
> browser_styleinspector_bug_672746_default_styles.js
> @@ +49,5 @@
> > +{
> > +  Services.obs.removeObserver(SI_check, "StyleInspector-populated", false);
> > +  SI_CheckPropertiesForSpan();
> > +  Services.obs.addObserver(finishUp, "StyleInspector-closed", false);
> > +  stylePanel.hidePopup();
> 
> I think it is too early to hide the popup here, the StyleInspector-closed
> notification is racing with StyleInspector-populated in
> SI_toggleDefaultStyles. It would make more sense as the last statement in
> SI_checkDefaultStyles.
> 

Maybe, done.

> @@ +87,5 @@
> > +  let propertyViews = stylePanel.cssHtmlTree.propertyViews;
> > +  let className = propertyViews[aName].className;
> > +  if (className == "property-view") {
> > +    return true;
> > +  } else if (className == "property-view-hidden") {
> 
> The else clause here is redundant and causes a warning from the JS engine.

Done

Comment 10

[Mihai Sucan [:msucan]]

Kevin and Dave: Is this supposed to show user agent rules or not? Selectors and values coming from the default styles of the browser.

The current behavior, implemented in the patch, is confusing for me. It hides properties that have no matched selectors, and that's all. Far different from what I would've expected from the label of the new check box, and from the title of the bug. Is this the actual purpose of the bug?

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

I didn't catch it previously, but what I think is missing is the two lines in my first patch that change the filter from ALL to UA when toggling the checkbox. This way a checked state shows both the properties without selectors and the rules from the user agent. Unless of course we never want to show the UA style rules.

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I would say that this and show UA rules are two totally different features. I logged this ticket after discussing what we needed with Kevin and my implementation is what we had agreed on. I agree that the label "Show default styles" is misleading though but we couldn't think of a better name.

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Include default styles patch 4

Rebased.

BTW: The point of this patch is to avoid information overload.

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

From the styleinspector features page:

Initial Display

Only user-provided, non-default styles are displayed (F3) in order to minimize the number of items the user needs to wade through in order to find what they're looking for. These styles are displayed in alphabetical order by property name. 

Include Default Styles

The user can choose to include the default styles (F14) and not just the overridden ones. These styles are sorted alphabetically by property name.

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Include default styles patch 5

Rebased

Comment 16

[Kevin Dangoor]

My thinking on this is that the top level that we're providing is not a list of stylesheet rules but rather computed properties. If the UA default color is "black" and no user stylesheet overrides it, we don't show the "color" property at all.

A gray area would be a case where there are unmatched rules for "color". I would argue that, for the sake of making the view as useful as possible, "color" should not be displayed in that case. After all, the value for color is just the default style provided by the UA.

In the case where the "color" happens to be the same as the default style (the user set it to black in their stylesheet), then we should still show the color property because a user stylesheet matches.

Hopefully that is clear and doesn't muddy the waters more!

Comment 17

[Mihai Sucan [:msucan]]

(In reply to Kevin Dangoor from comment #16)
> My thinking on this is that the top level that we're providing is not a list
> of stylesheet rules but rather computed properties. If the UA default color
> is "black" and no user stylesheet overrides it, we don't show the "color"
> property at all.

Thanks for your explanation Kevin!

I understand the reasoning, however there's one "catch" here, and perhaps this is why I was most confused. In CssLogic we determine as "matched selectors" any selector from any rule that matches the highlighted element OR *any* of its parent elements. Note the parents.

What that means is simple: if you have an H2 element inside a DIV with border:1px solid red; you'll see all the border properties listed when you highlight the H2 element. So, there all the benefit intended with this patch is "blown away".

If you take a small-medium sized layout (in terms of CSS) like robodesign.ro ... there's enough styling to make this patch have almost no benefits. If you go to big sites with complicated layouts, like github.com, you'll see what I mean. Almost any element you highlight has *many* of the properties listed, because there's almost always a rule with selectors that match one of the highlighted element parents.

In CssHtmlTree we cannot determine if there are only parent matches, to hide such cases. Such endeavor would entail changes to CssLogic - a far more involved patch.

This is why I believe there's too little benefit for this new check box, in the current implementation.

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Mihai Sucan [:msucan] from comment #17)
> (In reply to Kevin Dangoor from comment #16)
> > My thinking on this is that the top level that we're providing is not a list
> > of stylesheet rules but rather computed properties. If the UA default color
> > is "black" and no user stylesheet overrides it, we don't show the "color"
> > property at all.
> 
> Thanks for your explanation Kevin!
> 
> I understand the reasoning, however there's one "catch" here, and perhaps
> this is why I was most confused. In CssLogic we determine as "matched
> selectors" any selector from any rule that matches the highlighted element
> OR *any* of its parent elements. Note the parents.
> 
> What that means is simple: if you have an H2 element inside a DIV with
> border:1px solid red; you'll see all the border properties listed when you
> highlight the H2 element. So, there all the benefit intended with this patch
> is "blown away".
> 
> If you take a small-medium sized layout (in terms of CSS) like robodesign.ro
> ... there's enough styling to make this patch have almost no benefits. If
> you go to big sites with complicated layouts, like github.com, you'll see
> what I mean. Almost any element you highlight has *many* of the properties
> listed, because there's almost always a rule with selectors that match one
> of the highlighted element parents.
> 
> In CssHtmlTree we cannot determine if there are only parent matches, to hide
> such cases. Such endeavor would entail changes to CssLogic - a far more
> involved patch.
> 
> This is why I believe there's too little benefit for this new check box, in
> the current implementation.

I don't understand why you say all the benefit is blown away ... a match from a parent rule is displayed. The filter box is also there to further refine things if necessary.

Comment 19

[Mihai Sucan [:msucan]]

(In reply to Michael Ratcliffe from comment #18)
> I don't understand why you say all the benefit is blown away ... a match
> from a parent rule is displayed. The filter box is also there to further
> refine things if necessary.

Because we are back to numerous properties being listed. Where the purpose of this patch would be the ease that, to have fewer properties listed when they are not needed. And it seems to me that's faaar more often that most props show because there's a parent match for the given prop + element combo.

Comment 20

[Mihai Sucan [:msucan]]

Comment on attachment 559435 [details] [diff] [review]
Include default styles patch 5

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

Patch looks good. All tests pass. Please address the comments below.

Giving the patch r+ with a grain of salt: this code hides property views using a class name which is conditionally set, based on the number of matchedSelectorCount (which comes from CssLogic). Why I do not like this much:

- it's a "hack". Just change a class name to do display:none.
- it has no positive perf impact, just hiding stuff from the UI. I would like to see better performance when properties are hidden. I guess this code needs to be revisited when bug 685902 is fixed.
  - I would like to see no further work being done on the property view when it's hidden. To keep things minimal.

Going to ask for additional review from Rob. What do you think?

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +373,5 @@
> +    if (!this.defaultStylesCheckbox.checked && this.matchedSelectorCount == 0) {
> +      return false;
> +    }
> +
> +    return visible;

Simpler:
return this.defaultStylesCheckbox.checked || this.matchedSelectorCount > 0;

@@ +385,5 @@
> +    if (this.visible) {
> +      return "property-view";
> +    } else {
> +      return "property-view-hidden";
> +    }

Simpler:
return this.visible ? "property-view" : "property-view-hidden";

@@ +425,3 @@
>      this.valueNode.innerHTML = this.propertyInfo.value;
>  
>      if (this.prevViewedElement != this.tree.viewedElement) {

Can you please move this block of code before the if (!this.tree.viewedElement) ? I forgot to ask for this in bug 674856. We need this block of code to always execute, even when viewedElement is null.

Thanks!

::: browser/devtools/styleinspector/csshtmltree.xhtml
@@ +84,3 @@
>    -->
>    <div id="templateRoot">
> +    <div class="path">

This element doesn't hold the path. I suggest you use a different class name, with the same styling, if needed.

@@ +84,5 @@
>    -->
>    <div id="templateRoot">
> +    <div class="path">
> +      <input id="defaultStyles" type="checkbox" onchange="${refreshPanel}"/>
> +      <label id="defaultStylesLabel" dir="${getRTLAttr}">&defaultStylesLabel;</label>

I cannot click the label to toggle the checkbox state. It is missing the for="defaultStyles" attribute.

Also please note that this is buggy. The two elements are cloned by the templater and appended into #root, which means there will be elements in the DOM with the same ID. Please do not use id attributes here.

Why does the label have an ID? Is it used?

Lastly, you could wrap the input into the label:

<label><input></label>

(no need to have the for="" attribute)

::: browser/devtools/styleinspector/test/browser/browser_styleinspector_bug_672746_default_styles.js
@@ +1,5 @@
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Tests that the checkbox to include the UA stylesheet works properly.

This is confusing. :) Please change the test description to better match what this bug fixes/implements.

@@ +91,5 @@
> +    return true;
> +  }
> +  if (className == "property-view-hidden") {
> +    return false;
> +  }

Please do:

return !propertyViews[aName].classList.contains("property-view-hidden");

::: browser/locales/en-US/chrome/browser/styleinspector.dtd
@@ +1,3 @@
> +<!-- LOCALIZATION NOTE (defaultStylesLabel): This is the label for the checkbox
> +  -  that specifies whether the default styles (the ones in the system
> +  -  stylesheet) should be displayed or not. -->

This is a confusing description. It makes me believe this checkbox will display the user agent style sheets and their rules, selectors, properties, etc.

Please update.

::: browser/themes/gnomestripe/browser/devtools/csshtmltree.css
@@ +171,5 @@
>  }
> +
> +#defaultStylesLabel {
> +  position:relative;
> +  bottom: 3px;

I believe you don't need position: relative; bottom: 3px; here.

You can change this to margin top/bottoms as you see fit.

(I am asking for this change because AFAIK for rendering it's more expensive to do relative positioning rather than a simple margin. The same visual effect *here*. Leave position:relative for more advanced purposes.)

Comment 21

[Mihai Sucan [:msucan]]

Comment on attachment 559435 [details] [diff] [review]
Include default styles patch 5

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

Something I forgot to ask for. See the comment below.

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +420,5 @@
>        this.unmatchedExpander.removeAttribute("open");
>        return;
>      }
>  
> +    this.element.className = this.className;

Here you can do an early return, no need to do further updates.

Put this.element.className = this.className; before if (!this.tree.viewedElement).

Then change the if condition to: if (!this.tree.viewedElement || !this.visible), so it bails out earlier when the property view is not visible.

Comment 22

[Mihai Sucan [:msucan]]

Comment on attachment 559435 [details] [diff] [review]
Include default styles patch 5

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

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +363,5 @@
>      return this.tree.cssLogic.getPropertyInfo(this.name);
>    },
>  
>    /**
> +   * should this property be visible?

Nit: please use a capital letter for the start of the comment.

Comment 23

[Mihai Sucan [:msucan]]

Comment on attachment 559435 [details] [diff] [review]
Include default styles patch 5

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

::: browser/devtools/styleinspector/test/browser/browser_styleinspector_bug_672746_default_styles.js
@@ +66,5 @@
> +  // Click on the checkbox.
> +  let iframe = stylePanel.querySelector("iframe");
> +  let checkbox = iframe.contentDocument.querySelector("#defaultStyles");
> +  Services.obs.addObserver(SI_checkDefaultStyles, "StyleInspector-populated", false);
> +  EventUtils.sendMouseEvent({ type: "click" }, checkbox);

Please use the synthesizeMouse() method here.

Comment 24

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Include default styles patch 6

(In reply to Mihai Sucan [:msucan] from comment #20)
> Comment on attachment 559435 [details] [diff] [review]
> Include default styles patch 5
> 
> Review of attachment 559435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good. All tests pass. Please address the comments below.
> 
> Giving the patch r+ with a grain of salt: this code hides property views
> using a class name which is conditionally set, based on the number of
> matchedSelectorCount (which comes from CssLogic). Why I do not like this
> much:
> 
> - it's a "hack". Just change a class name to do display:none.
> - it has no positive perf impact, just hiding stuff from the UI. I would
> like to see better performance when properties are hidden. I guess this code
> needs to be revisited when bug 685902 is fixed.
>   - I would like to see no further work being done on the property view when
> it's hidden. To keep things minimal.
> 
> Going to ask for additional review from Rob. What do you think?
> 
> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ +373,5 @@
> > +    if (!this.defaultStylesCheckbox.checked && this.matchedSelectorCount == 0) {
> > +      return false;
> > +    }
> > +
> > +    return visible;
> 
> Simpler:
> return this.defaultStylesCheckbox.checked || this.matchedSelectorCount > 0;
> 

It would be simpler but far less expandable ... we don't always want to return straight away. Adding things like filters will also return false here. I have still simplified slightly.

> @@ +385,5 @@
> > +    if (this.visible) {
> > +      return "property-view";
> > +    } else {
> > +      return "property-view-hidden";
> > +    }
> 
> Simpler:
> return this.visible ? "property-view" : "property-view-hidden";
> 

Done

> @@ +425,3 @@
> >      this.valueNode.innerHTML = this.propertyInfo.value;
> >  
> >      if (this.prevViewedElement != this.tree.viewedElement) {
> 
> Can you please move this block of code before the if
> (!this.tree.viewedElement) ? I forgot to ask for this in bug 674856. We need
> this block of code to always execute, even when viewedElement is null.
> 
> Thanks!
> 

Done

> ::: browser/devtools/styleinspector/csshtmltree.xhtml
> @@ +84,3 @@
> >    -->
> >    <div id="templateRoot">
> > +    <div class="path">
> 
> This element doesn't hold the path. I suggest you use a different class
> name, with the same styling, if needed.
> 

Changed the name to filters

> @@ +84,5 @@
> >    -->
> >    <div id="templateRoot">
> > +    <div class="path">
> > +      <input id="defaultStyles" type="checkbox" onchange="${refreshPanel}"/>
> > +      <label id="defaultStylesLabel" dir="${getRTLAttr}">&defaultStylesLabel;</label>
> 
> I cannot click the label to toggle the checkbox state. It is missing the
> for="defaultStyles" attribute.
> 
> Also please note that this is buggy. The two elements are cloned by the
> templater and appended into #root, which means there will be elements in the
> DOM with the same ID. Please do not use id attributes here.
> 
> Why does the label have an ID? Is it used?
> 
> Lastly, you could wrap the input into the label:
> 
> <label><input></label>
> 
> (no need to have the for="" attribute)
> 

Done

> :::
> browser/devtools/styleinspector/test/browser/
> browser_styleinspector_bug_672746_default_styles.js
> @@ +1,5 @@
> > +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> > +/* Any copyright is dedicated to the Public Domain.
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +// Tests that the checkbox to include the UA stylesheet works properly.
> 
> This is confusing. :) Please change the test description to better match
> what this bug fixes/implements.
> 

Done

> @@ +91,5 @@
> > +    return true;
> > +  }
> > +  if (className == "property-view-hidden") {
> > +    return false;
> > +  }
> 
> Please do:
> 
> return !propertyViews[aName].classList.contains("property-view-hidden");
> 

There is no classList property here (it is a propertyView, not a node). I have still optimized this check though.

> ::: browser/locales/en-US/chrome/browser/styleinspector.dtd
> @@ +1,3 @@
> > +<!-- LOCALIZATION NOTE (defaultStylesLabel): This is the label for the checkbox
> > +  -  that specifies whether the default styles (the ones in the system
> > +  -  stylesheet) should be displayed or not. -->
> 
> This is a confusing description. It makes me believe this checkbox will
> display the user agent style sheets and their rules, selectors, properties,
> etc.
> 
> Please update.
> 

Done

> ::: browser/themes/gnomestripe/browser/devtools/csshtmltree.css
> @@ +171,5 @@
> >  }
> > +
> > +#defaultStylesLabel {
> > +  position:relative;
> > +  bottom: 3px;
> 
> I believe you don't need position: relative; bottom: 3px; here.
> 
> You can change this to margin top/bottoms as you see fit.
> 
> (I am asking for this change because AFAIK for rendering it's more expensive
> to do relative positioning rather than a simple margin. The same visual
> effect *here*. Leave position:relative for more advanced purposes.)

I had tried margin first but it moves both the checkbox and the label. The default positioning is just not good so we just don't have much choice except to use position:relative here.

(In reply to Mihai Sucan [:msucan] from comment #21)
> Comment on attachment 559435 [details] [diff] [review]
> Include default styles patch 5
> 
> Review of attachment 559435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Something I forgot to ask for. See the comment below.
> 
> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ +420,5 @@
> >        this.unmatchedExpander.removeAttribute("open");
> >        return;
> >      }
> >  
> > +    this.element.className = this.className;
> 
> Here you can do an early return, no need to do further updates.
> 
> Put this.element.className = this.className; before if
> (!this.tree.viewedElement).
> 
> Then change the if condition to: if (!this.tree.viewedElement ||
> !this.visible), so it bails out earlier when the property view is not
> visible.

Done

(In reply to Mihai Sucan [:msucan] from comment #22)
> Comment on attachment 559435 [details] [diff] [review]
> Include default styles patch 5
> 
> Review of attachment 559435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ +363,5 @@
> >      return this.tree.cssLogic.getPropertyInfo(this.name);
> >    },
> >  
> >    /**
> > +   * should this property be visible?
> 
> Nit: please use a capital letter for the start of the comment.

Done

(In reply to Mihai Sucan [:msucan] from comment #23)
> Comment on attachment 559435 [details] [diff] [review]
> Include default styles patch 5
> 
> Review of attachment 559435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> browser/devtools/styleinspector/test/browser/
> browser_styleinspector_bug_672746_default_styles.js
> @@ +66,5 @@
> > +  // Click on the checkbox.
> > +  let iframe = stylePanel.querySelector("iframe");
> > +  let checkbox = iframe.contentDocument.querySelector("#defaultStyles");
> > +  Services.obs.addObserver(SI_checkDefaultStyles, "StyleInspector-populated", false);
> > +  EventUtils.sendMouseEvent({ type: "click" }, checkbox);
> 
> Please use the synthesizeMouse() method here.

Done

Comment 25

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Quick note to point out that this is now a "Only user styles" as discussed with Kevin & dcamp.

Comment 26

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Include default styles patch 7

Updated stylesheets

Comment 27

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Include default styles patch 8

More team discussion ... we now also list UA stylesheet properties when "Only user styles" is cleared.

Mihai, it is ready for review again now ... sorry about the rapid updates.

Comment 28

[Mihai Sucan [:msucan]]

Comment on attachment 560203 [details] [diff] [review]
Include default styles patch 8

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

Great to see that the decision was made to add more usefulness to this checkbox. :)

Thanks for the updated patch.

General comment: please update the patch message, since the bug title changed.

Giving the patch r- because some points from comment 20 were not addressed, and I'd like to re-review the updated patch. Please address the comments below. Thank you!

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +236,5 @@
>    },
>  
>    /**
> +   * Toggle UA styles and toggle the display of css properties without rules
> +   * that originate in a user stylesheet.

s/css/CSS/

This description is confusing. It says it toggles UA styles and the display of properties without rules coming from user styles - properties that come from rules outside those of user-provided styles ... are exactly UA styles.

I think you mean:

"The change event handler for the onlyUserStyles checkbox. When onlyUserStyles.checked is true we do not display properties that have no matched selectors, and we do not display UA styles. If .checked is false we do display even properties with no matched selectors, and we include the UA styles."

@@ +240,5 @@
> +   * that originate in a user stylesheet.
> +   *
> +   * @param {Event} aEvent the DOM Event object.
> +   */
> +  onlyUserStylesClicked: function CssHtmltree_onlyUserStylesClicked(aEvent)

s/Click/Changed/g

@@ +246,5 @@
> +    if (aEvent.target.checked) {
> +      this.cssLogic.sourceFilter = CssLogic.FILTER.ALL;
> +    } else {
> +      this.cssLogic.sourceFilter = CssLogic.FILTER.UA;
> +    }

this.cssLogic.sourceFilter = aEvent.target.checked ?
                             CssLogic.FILTER.ALL :
                             CssLogic.FILTER.UA;

@@ +311,5 @@
>    this.link = "https://developer.mozilla.org/en/CSS/" + aName;
>  
>    this.templateMatchedSelectors = aTree.styleDocument.getElementById("templateMatchedSelectors");
>    this.templateUnmatchedSelectors = aTree.styleDocument.getElementById("templateUnmatchedSelectors");
> +  this.onlyUserStylesCheckbox = aTree.styleDocument.getElementById("userStyles");

Put this in CssHtmlTree somewhere. Don't do getElementById() inside each new PropertyView. It's faster to only do this once in CssHtmlTree and just access the checkbox from this.tree.onlyUserStylesCheckbox.

In CssHtmlTree you could also add a getter showOnlyUserStyles that returns this.onlyUserStylesCheckbox.checked. From the PropertyView.visible getter you can do this.tree.showOnlyUserStyles.

@@ +423,5 @@
>    {
> +    this.element.className = this.className;
> +    this.valueNode.innerHTML = this.propertyInfo.value;
> +
> +    if (!this.tree.viewedElement || !this.visible) {

In comment 20 I asked to move the if (this.prevViewedElement != this.tree.viewedElement) { ... } block before the if (!this.tree.viewedElement || !this.visible) { ... }.

For less confusion, here's how the refresh() method needs to look:

refresh: function PropertyView_refresh()
{
  this.element.className = this.className;

  if (this.prevViewedElement != this.tree.viewedElement) {
    this._matchedSelectorViews = null;
    this._unmatchedSelectorViews = null;
    this.prevViewedElement = this.tree.viewedElement;
  }

  if (!this.tree.viewedElement || !this.visible) {
    this.valueNode.innerHTML = "";
    this.matchedSelectorsContainer.hidden = true;
    this.unmatchedSelectorsContainer.hidden = true;
    this.matchedSelectorTable.innerHTML = "";
    this.unmatchedSelectorTable.innerHTML = "";
    this.matchedExpander.removeAttribute("open");
    this.unmatchedExpander.removeAttribute("open");
    return;
  }

  this.valueNode.innerHTML = this.value;

  this.refreshMatchedSelectors();
  this.refreshUnmatchedSelectors();
}

::: browser/devtools/styleinspector/csshtmltree.xhtml
@@ +86,5 @@
> +    <div class="filters">
> +      <label id="userStylesLabel" dir="${getRTLAttr}">
> +        &userStylesLabel;
> +        <input id="userStyles" type="checkbox" onchange="${onlyUserStylesClicked}"
> +          checked=""/>

In comment 20 I also asked for removing the IDs. Please use classes.

::: browser/devtools/styleinspector/test/browser/browser_styleinspector_bug_672746_default_styles.js
@@ +32,5 @@
> +}
> +
> +function SI_inspectNode()
> +{
> +  var span = doc.querySelector("#matches");

s/var/let/

@@ +50,5 @@
> +  Services.obs.removeObserver(SI_check, "StyleInspector-populated", false);
> +  SI_CheckPropertiesForSpan();
> +}
> +
> +function SI_CheckPropertiesForSpan()

I think you can merge this function into SI_check().

::: browser/locales/en-US/chrome/browser/styleinspector.dtd
@@ +1,3 @@
> +<!-- LOCALIZATION NOTE (userStylesLabel): This is the label for the checkbox
> +  -  that specifies whether the styles that are not from the user's stylesheet
> +  should be displayed or not. -->

-  should be displayed or not. -->

Comment 29

[Mihai Sucan [:msucan]]

(In reply to Michael Ratcliffe from comment #24)
> > @@ +91,5 @@
> > > +    return true;
> > > +  }
> > > +  if (className == "property-view-hidden") {
> > > +    return false;
> > > +  }
> > 
> > Please do:
> > 
> > return !propertyViews[aName].classList.contains("property-view-hidden");
> > 
> 
> There is no classList property here (it is a propertyView, not a node). I
> have still optimized this check though.

Ah, yes, my bad. I meant propertyViews[aName].element.classList.contains("property-view-hidden")

Nonetheless, your solution in the updated patch is fine. Thank you!

Comment 30

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: [in-fx-team] Only user styles patch

> General comment: please update the patch message, since the bug title
> changed.
> 

Done

> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ +236,5 @@
> >    },
> >  
> >    /**
> > +   * Toggle UA styles and toggle the display of css properties without rules
> > +   * that originate in a user stylesheet.
> 
> s/css/CSS/
> 
> This description is confusing. It says it toggles UA styles and the display
> of properties without rules coming from user styles - properties that come
> from rules outside those of user-provided styles ... are exactly UA styles.
> 
> I think you mean:
> 
> "The change event handler for the onlyUserStyles checkbox. When
> onlyUserStyles.checked is true we do not display properties that have no
> matched selectors, and we do not display UA styles. If .checked is false we
> do display even properties with no matched selectors, and we include the UA
> styles."
> 

I can see the ambiguity, thanks for the concise comment ... done.

> @@ +240,5 @@
> > +   * that originate in a user stylesheet.
> > +   *
> > +   * @param {Event} aEvent the DOM Event object.
> > +   */
> > +  onlyUserStylesClicked: function CssHtmltree_onlyUserStylesClicked(aEvent)
> 
> s/Click/Changed/g
> 

Done

> @@ +246,5 @@
> > +    if (aEvent.target.checked) {
> > +      this.cssLogic.sourceFilter = CssLogic.FILTER.ALL;
> > +    } else {
> > +      this.cssLogic.sourceFilter = CssLogic.FILTER.UA;
> > +    }
> 
> this.cssLogic.sourceFilter = aEvent.target.checked ?
>                              CssLogic.FILTER.ALL :
>                              CssLogic.FILTER.UA;
> 

The old ternary operator eh? Personally I prefer if statements but ... done.

> @@ +311,5 @@
> >    this.link = "https://developer.mozilla.org/en/CSS/" + aName;
> >  
> >    this.templateMatchedSelectors = aTree.styleDocument.getElementById("templateMatchedSelectors");
> >    this.templateUnmatchedSelectors = aTree.styleDocument.getElementById("templateUnmatchedSelectors");
> > +  this.onlyUserStylesCheckbox = aTree.styleDocument.getElementById("userStyles");
> 
> Put this in CssHtmlTree somewhere. Don't do getElementById() inside each new
> PropertyView. It's faster to only do this once in CssHtmlTree and just
> access the checkbox from this.tree.onlyUserStylesCheckbox.
> 

We now save it to cssHtmlTree from the templater.

> In CssHtmlTree you could also add a getter showOnlyUserStyles that returns
> this.onlyUserStylesCheckbox.checked. From the PropertyView.visible getter
> you can do this.tree.showOnlyUserStyles.
> 

Done

> @@ +423,5 @@
> >    {
> > +    this.element.className = this.className;
> > +    this.valueNode.innerHTML = this.propertyInfo.value;
> > +
> > +    if (!this.tree.viewedElement || !this.visible) {
> 
> In comment 20 I asked to move the if (this.prevViewedElement !=
> this.tree.viewedElement) { ... } block before the if
> (!this.tree.viewedElement || !this.visible) { ... }.
> 
> For less confusion, here's how the refresh() method needs to look:
> 
> refresh: function PropertyView_refresh()
> {
>   this.element.className = this.className;
> 
>   if (this.prevViewedElement != this.tree.viewedElement) {
>     this._matchedSelectorViews = null;
>     this._unmatchedSelectorViews = null;
>     this.prevViewedElement = this.tree.viewedElement;
>   }
> 
>   if (!this.tree.viewedElement || !this.visible) {
>     this.valueNode.innerHTML = "";
>     this.matchedSelectorsContainer.hidden = true;
>     this.unmatchedSelectorsContainer.hidden = true;
>     this.matchedSelectorTable.innerHTML = "";
>     this.unmatchedSelectorTable.innerHTML = "";
>     this.matchedExpander.removeAttribute("open");
>     this.unmatchedExpander.removeAttribute("open");
>     return;
>   }
> 
>   this.valueNode.innerHTML = this.value;
> 
>   this.refreshMatchedSelectors();
>   this.refreshUnmatchedSelectors();
> }
> 

Aha, you meant that block ... done.

> ::: browser/devtools/styleinspector/csshtmltree.xhtml
> @@ +86,5 @@
> > +    <div class="filters">
> > +      <label id="userStylesLabel" dir="${getRTLAttr}">
> > +        &userStylesLabel;
> > +        <input id="userStyles" type="checkbox" onchange="${onlyUserStylesClicked}"
> > +          checked=""/>
> 
> In comment 20 I also asked for removing the IDs. Please use classes.
> 

Done

> :::
> browser/devtools/styleinspector/test/browser/
> browser_styleinspector_bug_672746_default_styles.js
> @@ +32,5 @@
> > +}
> > +
> > +function SI_inspectNode()
> > +{
> > +  var span = doc.querySelector("#matches");
> 
> s/var/let/
> 

Done

> @@ +50,5 @@
> > +  Services.obs.removeObserver(SI_check, "StyleInspector-populated", false);
> > +  SI_CheckPropertiesForSpan();
> > +}
> > +
> > +function SI_CheckPropertiesForSpan()
> 
> I think you can merge this function into SI_check().
> 

Done

> ::: browser/locales/en-US/chrome/browser/styleinspector.dtd
> @@ +1,3 @@
> > +<!-- LOCALIZATION NOTE (userStylesLabel): This is the label for the checkbox
> > +  -  that specifies whether the styles that are not from the user's stylesheet
> > +  should be displayed or not. -->
> 
> -  should be displayed or not. -->

Wow, well spotted ... done

Comment 31

[Mihai Sucan [:msucan]]

Comment on attachment 561148 [details] [diff] [review]
[in-fx-team] Only user styles patch

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

Thank you for addressing the review comments, including the nits. ;)

General comment: the patch has some trailing white spaces. Please remove it.

r+, this patch can land! Great work!

::: browser/devtools/styleinspector/csshtmltree.xhtml
@@ +79,5 @@
>  -->
>  <div style="display:none;">
>    <!--
> +  templateRoot sits at the top of the window and contains the "include default
> +  styles" checkbox. For data it needs an instance of CssHtmlTree.

Nit: change "include default styles" to "only user styles".

Comment 32

[Rob Campbell [:rc] (:robcee)]

can this land?

Comment 33

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Only user styles patch 2

Rob, yes ... all green

Comment 34

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 561148 [details] [diff] [review]
[in-fx-team] Only user styles patch

https://hg.mozilla.org/integration/fx-team/rev/1314532cfdb9

Comment 35

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/1314532cfdb9

Comment 36

[Florin Strugariu [:Bebe]]

verified on
Mozilla/5.0 (Windows NT 6.1; rv:9.0a2) Gecko/20111010 Firefox/9.0a2