705707 - Style Inspector doesn't take into account chrome:// stylesheets

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

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

Apparently using the rule view or the property view of our style inspector does not work for XUL documents. I've tried both chrome:// and file:// URLs, without any observable difference. Try opening one of the following:

chrome://browser/content/styleeditor.xul
file:///path/to/your/repo/browser/devtools/styleeditor/styleeditor.xul

For the latter you need to set dom.allow_XUL_XBL_for_file to true. Highlight the search box and observe that the rule view is empty. The property view is empty as well, but if you uncheck the "only user styles" option, you can see there is a matching max-width rule from splitview.css. There is also a -moz-margin-start in that rule that is not displayed at all.

It would make working in browser/extension code much easier if these tools could properly display such information.

Comment 1

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

Bug triage, filter on PEGASUS.

Comment 2

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

This is because we block chrome stylesheets when the "only user styles" checkbox is checked (that is the meaning of the option).

If I remember correctly, there was more to this bug ... I will need to discuss it with Panos.

Comment 3

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

(In reply to Michael Ratcliffe from comment #2)
> This is because we block chrome stylesheets when the "only user styles"
> checkbox is checked (that is the meaning of the option).

Unchecking the option is not a workaround that can be used in the rules view, unfortunately. Furthermore, from the POV of a XUL application/add-on developer, styles included in a XUL file *are* the user styles. The label is misleading in that particular use case.

Comment 4

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

The meaning of "only user styles" in normal docs is "only show properties that have been changed from their defaults and to hide chrome CSS." The meaning in XUL docs will be "only show properties that have been changed from their defaults."

-moz-*-start and -moz-*-end are simply aliases to -moz-*-left and -moz-*-right (depending on the locale) so they would be shown in computed styles as margin-(left|right). All that we need to do here is include chrome css when inspecting XUL documents.

Comment 5

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

Sorry, I obviously meant:
-moz-*-start and -moz-*-end are simply aliases to *-left and *-right (depending on the locale) so they would be shown in computed styles as margin-(left|right). All that we need to do here is include chrome css when inspecting XUL documents.

Comment 6

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

Attachment: Patch

This patch allows the computed and rule views to work with XUL documents.

Comment 7

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

msucan pointed out that using the file extension to determine if we have a xul doc is probably not the best way. I will change this to use the root namespace instead.

Comment 8

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

Attachment: Patch now uses root namespace to detect XUL docs

Obviously using the file extension to check for a xul doc was not ideal so now we use the root element's namespace instead.

Comment 9

[Dão Gottwald [:dao]]

Why does CssLogic.isSystemStyleSheet return true for chrome: URIs? This seems bogus.

Comment 10

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

chrome == system for the sake of this method. The implementation of this is basically straight out of firebug.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Rob Campbell [:rc] (robcee) from comment #10)
> chrome == system for the sake of this method.

What chrome stylesheets affect web pages?

> The implementation of this is basically straight out of firebug.

Sure, but this doesn't make it right.

Comment 12

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

it does for about 99% of our content inspection needs. When we poll the style system for a list of stylesheets, it gives us everything on the page. That includes a good number of chrome: stylesheets that come from the system.

It might be worth somehow separating these from the stylesheets lookup itself, but that'll require some extra smarts from the layout machinery.

Comment 13

[Dão Gottwald [:dao]]

Does DOM Inspector use the same approach to solve this?

Comment 14

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

(In reply to Dão Gottwald [:dao] from comment #13)
> Does DOM Inspector use the same approach to solve this?

I expect so since they were both written by the same guy.

http://hg.mozilla.org/dom-inspector/

Comment 15

[Dão Gottwald [:dao]]

But does DOMi contain the patch proposed here? DOMi doesn't suffer from this bug, so presumably it doesn't have exactly the same code as Firebug.

Comment 16

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

Attachment: Patch without chrome check

Dão is correct here, we do not need to filter chrome url's in CssLogic.isSystemStyleSheet because they are only included when the UA filter is enabled and then we need to see them anyway.

In effect this check did nothing except waste cycles. Removing the check causes no issues so ... done.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 591421 [details] [diff] [review]
Patch without chrome check

Why do you need to detect XUL documents at this point?

Note that XML documents can mix different namespaces, which makes "XUL document" kind of meaningless...

Comment 18

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

After discussing this as a group we didn't like the idea of removing the chrome check even though it does not affect CSSLogic. Our reasoning is that CssLogic is a utility that could be used by other tools (it is used by the rule view). And these other tools should be able to make use of this method.

We are struggling to understand exactly what your issue is with this method, to all intents and purposes chrome stylesheets are system stylesheets so all of these checks are valid and there is no bug.

Can you explain your issue with CssLogic.isSystemStyleSheet? Is it just the name of the method or is it the definition of what constitutes a system stylesheet?

Comment 19

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

Attachment: chrome check added again

Comment 20

[Dão Gottwald [:dao]]

(In reply to Michael Ratcliffe from comment #18)
> After discussing this as a group we didn't like the idea of removing the
> chrome check even though it does not affect CSSLogic. Our reasoning is that
> CssLogic is a utility that could be used by other tools (it is used by the
> rule view). And these other tools should be able to make use of this method.

I'm not sure what this means. What exactly is the impact on the rule view?

> to all intents and purposes chrome stylesheets are system stylesheets so all
> of these checks are valid and there is no bug.

Chrome stylesheets styling chrome documents aren't system stylesheets. This seems to be what this bug is about.

> Can you explain your issue with CssLogic.isSystemStyleSheet? Is it just the name of
> the method or is it the definition of what constitutes a system stylesheet?

The latter.

Has somebody checked by now what DOM Inspector is doing?

Comment 21

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

Attachment: isSystemStyleSheet renamed to isNotContentStyleSheet

Robcee has already looked at this patch so he may as well be the one to review it.

(In reply to Dão Gottwald [:dao] from comment #20)
> (In reply to Michael Ratcliffe from comment #18)
> > After discussing this as a group we didn't like the idea of removing the
> > chrome check even though it does not affect CSSLogic. Our reasoning is that
> > CssLogic is a utility that could be used by other tools (it is used by the
> > rule view). And these other tools should be able to make use of this method.
> 
> I'm not sure what this means. What exactly is the impact on the rule view?
> 

Probably none actually, but the intent of the method should not be changed.

> > to all intents and purposes chrome stylesheets are system stylesheets so all
> > of these checks are valid and there is no bug.
> 
> Chrome stylesheets styling chrome documents aren't system stylesheets. This
> seems to be what this bug is about.
> 

True

> Has somebody checked by now what DOM Inspector is doing?

Yes, but it works completely differently because it has no "only user styles" "only user styles" checkbox.

> > Can you explain your issue with CssLogic.isSystemStyleSheet? Is it just the name of
> > the method or is it the definition of what constitutes a system stylesheet?
> 
> The latter.
> 

Ah, okay then ... method renamed to isNotContentStyleSheet as that is actually the intent of this method.

Comment 22

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

Comment on attachment 592108 [details] [diff] [review]
isSystemStyleSheet renamed to isNotContentStyleSheet

-CssLogic.isSystemStyleSheet = function CssLogic_isSystemStyleSheet(aSheet)
+CssLogic.isNotContentStyleSheet = function CssLogic_isNotContentStyleSheet(aSheet)

I honestly didn't have a problem with the original name, I find this one uglier, but in the interests of appeasing everyone, let's go with it.

Comment 23

[Dão Gottwald [:dao]]

(In reply to Michael Ratcliffe from comment #21)
> > Has somebody checked by now what DOM Inspector is doing?
> 
> Yes, but it works completely differently because it has no "only user
> styles" "only user styles" checkbox.

Nor does it list chrome: stylesheets for web content. What problem is the exclusion of chrome: trying to solve?

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 592108 [details] [diff] [review]
isSystemStyleSheet renamed to isNotContentStyleSheet

As mentioned earlier, the XUL document check is bogus since XUL and other languages such as HTML can be mixed.

Comment 25

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

Comment on attachment 592108 [details] [diff] [review]
isSystemStyleSheet renamed to isNotContentStyleSheet

+      // Refresh source filter in case the element being inspected is inside a
+      // XUL document.
+      this.refreshSourceFilter();

...

 
+      // Refresh source filter in case the element being inspected is inside a
+      // XUL document. This must be done after templateRoot has been processed.
+      this.refreshSourceFilter();

...

+    let xul =
+      this.viewedElement.ownerDocument.documentElement.namespaceURI == XUL_NS;

Dão's right about this being overly-specific.

What about using a check similar to the isNotContentStyleSheet (ugh) method to check if the page we're inspecting is one of about: resource: chrome: URLs? That might get us a little closer to being accurate.

I really do wish we had a better name for these classes of pages. "System" feels about right.

Comment 26

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

... though the check does conform with the intent of this bug as described in the Summary.

dcamp, what say you?

Comment 27

[Dão Gottwald [:dao]]

(In reply to Rob Campbell [:rc] (robcee) from comment #26)
> ... though the check does conform with the intent of this bug as described
> in the Summary.

Rather than modelling the patch after some inaccurate summary, let's fix the summary? ;)

Comment 28

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

this is acceptable! :)

Comment 29

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

So, let's rethink this ... if a user has only added one line of CSS to an element they do not expect to see hundreds of rules (we want to show only non-default values by default). We have been doing this by filtering out chrome & system stylesheets.

The problem is that theoretically any chrome: or resource: stylesheet can be used by any page. What we really need is a simple way to tell if a rule comes from a stylesheet on the page or whether it comes from outside the page.

We could just use styleSheets.ownerNode.baseURI to check if the sheet is from outside the document.

Comment 30

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

Attachment: Complete rewrite

I have totally rewritten the isSystemStylesheet method and changed it to isContentStylesheet (reversing the logic). We no longer rely on URL prefixes to detect non-content stylesheets as this approach works fine in all situations.

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 593867 [details] [diff] [review]
Complete rewrite

>+      // Refresh source filter in case the element being inspected is inside a
>+      // XUL document.

>+      // Refresh source filter in case the element being inspected is inside a
>+      // XUL document. This must be done after templateRoot has been processed.

>+  /**
>+   * When onlyUserStyles.checked is true we do not display properties that have
>+   * no matched selectors, and we do not display UA styles unless we are
>+   * inspecting a XUL document. If .checked is false we display properties with
>+   * and without matched selectors, and we include the UA styles.
>+   */

Are these comments about XUL still accurate? Your code doesn't seem to care for XUL at all (which is good).

Comment 32

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

Attachment: Fixed bad comments

You are right, we don't care about the document type any longer.

I have fixed all comments now.

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 593914 [details] [diff] [review]
Fixed bad comments

Looks solid to me overall.

Comment 34

[Mihai Sucan [:msucan]]

Comment on attachment 593914 [details] [diff] [review]
Fixed bad comments

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

This looks really good! Thanks Mike!

::: browser/devtools/styleinspector/CssLogic.jsm
@@ +878,5 @@
>      return true;
>    }
>  
> +  // If the sheet has a CSSImportRule we need to check the parent stylesheet.
> +  if (aSheet.ownerRule instanceof Ci.nsIDOMCSSImportRule) {

Can ownerRule be something different than null or Ci.nsIDOMCSSImportRule?

Comment 35

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

Nope, just null or importRule ... see http://www.w3.org/TR/2000/REC-DOM-Level-2-Style-20001113/css.html#CSS-CSSStyleSheet-ownerRule

I still think we should keep the instance check in case of future expansion.

Comment 36

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

https://hg.mozilla.org/integration/fx-team/rev/17a1f5c47b25

Comment 37

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

backed out in rev 4a07e7 due to bug 726101

Comment 38

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

Attachment: [in-fx-team] Whitelist xul iri for the test

A workaround for bug 726101

Comment 39

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

Green on try now so this should now be landable.

Comment 40

[Mihai Sucan [:msucan]]

Comment on attachment 596676 [details] [diff] [review]
[in-fx-team] Whitelist xul iri for the test

Landed:
https://hg.mozilla.org/integration/fx-team/rev/30cb97299656

Comment 41

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/30cb97299656