Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Description • 12 years ago

mozMatchesSelector throws when string contains a namespaced type selector e.g. :not(svg|a)

In the Developer Tools Computed View we list CSS rules that match the currently selected node. We do this using node.mozMatchesSelector("some css selector"). e.g.
  cursor: pointer
      .gbqlca → pointer          inline:11
      :-moz-any-link → pointer   chrome://blah/ua.css:88
  height: 71px
      .gbqlca → 100%             inline:11
  ...

When the selector contains a namespaced type selector e.g. :not(svg|a), mozMatchesSelector() throws "Exception: An invalid or illegal string was specified."

The reason namespaced type selectors have not been added to mozMatchesSelector() is because e.g. if one document is linked to two stylesheets both defining prefix foo but two DIFFERENT URLs how would we know which selector to choose from?

This means that until we have a way to work out which selectors match a node we will have to omit namespaced selectors in the Developer Tools Computed View, which is not desirable.

Any API that will allow us to obtain the following information for a specific node would work:
- The CSS properties contained in the rules affecting the node
- All matching selector texts (ordered by specificity)
- selector stylesheet urls
- selector line numbers

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 12 years ago

As filed, this bug is invalid: the throwing behavior is what the spec calls for.

Resummarizing to cover the actual request here.

So the current state of things is that you can get the set of all matching rules using getCSSStyleRules on the inspector utils.  This gives you the rules in specificity order, but we don't track internally which of the rule's selectors caused it to match at that location in the cascade.  The rules also have a line number and you can get the url of the stylesheet from the rule.

So the only issue is figuring out which exact selector out of the set of selectors the rule has matched at this location in the cascade, right?

How are you handling this right now using the mozMatchesSelector approach?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 2 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #1)
> So the only issue is figuring out which exact selector out of the set of
> selectors the rule has matched at this location in the cascade, right?
> 

Right

> How are you handling this right now using the mozMatchesSelector approach?

Yup

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 12 years ago

> Yup

That wasn't a yes-or-no question; there was no missing comma there.  The question was how you're using mozMatchesSelector to handle this right now for the non-namespaced cases.  Because I'm not quite clear on how one would go about actually doing that, so I'm trying to understand what I'm missing.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 4 • 12 years ago

Sorry, it has been a long day.

We get the cssRules for the node and iterate through each rule's selectors to obtain the selector text. We use node.mozMatchesSelector(selector.text) to check for matches and build the UI from there.

For selected node:
for each cssRule {
  for each cssRule.selector {
    if (node.mozMatchesSelector(selector.text)) {
      Add selector.text, cssRule.url and cssRule.line to the interface.
    }
  }
}

Actually, we break this down for each css property name (e.g. font-size) too. But I think it is the above that you are after.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 12 years ago

OK, so what do you end up doing when the rule has multiple selectors that match the node?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 6 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #5)
> OK, so what do you end up doing when the rule has multiple selectors that
> match the node?

This screenshot will help you see the final output.

With multiple rules and multiple selectors we loop through all selectors for all rules for each supported CSS property and recurse through a nodes ancestors to gather all of the required information.

A more complete view of what we do programatically:
for the selected node and all of it's ancestors {
  for each supported CSSProperty {
    Add CSS Property name to the interface
    for each cssRule {
      if cssRule contains CSSProperty name {
        for each cssRule.selector {
          if (node.mozMatchesSelector(selector.text)) {
            Add selector.text, cssRule.url and cssRule.line to the interface.
          }
        }
      }
    }
  }
}

We are more than aware that this is ugly but this is currently the only way that we can get the information we need.

- The "only user styles" checkbox is misleading, the label should be "only author styles." Notice that html.css is listed in the links to the right of the selectors because this checkbox is cleared.
- The current font-size is 24px due to the following selector:
    html {
      font-size: 100%;
    }
- The next six selectors are crossed out to show that they are overridden by the above selector.
- The next three selectors are inherited from a parent.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 12 years ago

So that doesn't quite answer my question.

My question was what should happen in a case like this:

  <style>
    #foo, div { color: green; }
    .bar { color: red; }
  </style>
  <div class="bar" id="foo">I am a div</div>

and the answer based on experimentation is that right now that first rule is in fact being listed twice, in the right order wrt the second rule (once before and once after). How are you managing that at the moment?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 8 • 12 years ago

Yes, that is what we do. The most important thing here is listing the selectors. Rules can be listed multiple times. 
e.g. based on your above markup:
  color: rgb(0, 128, 0)
    #foo → green  inline:7
    .bar → red    inline:8
    div → green   inline:7

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 12 years ago

Yes.  What I'm asking is how you currently decide that the "#foo" should be listed before the ".bar" and the ".bar" before the "div".

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 10 • 12 years ago

We calculate the specificity for each selector, see:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/CssLogic.jsm#1476

Which is roughly equivalent to:
http://mxr.mozilla.org/mozilla-central/source/layout/style/StyleRule.cpp#479

We then sort the selectors by specificity.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 12 years ago

Aha, I see.

Hmm.

So most simply, we could just expose some API on inspector utils that takes a rule and a selector index 'i' and an element and checks whether the i-th selector in that rule matches the given element.  Basically, keep your code as-is but swap out mozMatchesSelector for something that works right in all cases.

We could also expose APIs for asking for the specificity of the i-th selector or for its textual representation, so you don't have to manually parse apart selectors into pieces.

Would that help?  I'm not sure what we can do on the style system end that would really make this stuff easy on the JS end... In particular, I don't want to expose css selector objects directly to JS, because the ownership model doesn't really work for that so far.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 12 • 12 years ago

> Would that help?

It would help a lot.

Exposing the API on devtools.utils.inspector would probably make the most sense because there are more APIs for other tools that would be useful for our team in the future.

So we would have:
getSelectorMatchesElement(element, rule, selectorIdx)
getSpecificity(element, rule, selectorIdx)
getSelectorText(rule, selectorIdx)

	David Baron :dbaron:
	Comment 14 • 12 years ago

Comment on attachment 706463 [details] [diff] [review]
part 1.  Inspector code should be IMPL_NS_LAYOUT.

r=dbaron.  I'm not sure what the state of the alert about needing to tell build folks about any Makefile change is.  Maybe drop a line to :gps?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 12 years ago

Needinfo counts as a line, right?  ;)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 12 years ago

Comment on attachment 706494 [details] [diff] [review]
part 2.  Add some utilities for working with selectors to inspector utils.

Mike, want to try this and let me know whether it does what you want?

	Gregory Szorc [:gps]
	Comment 18 • 12 years ago

Comment on attachment 706463 [details] [diff] [review]
part 1.  Inspector code should be IMPL_NS_LAYOUT.

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

::: layout/inspector/src/Makefile.in
@@ +45,5 @@
>  		$(NULL)
>  
>  include $(topsrcdir)/config/rules.mk
> +
> +DEFINES += -D_IMPL_NS_LAYOUT

While this might work, you should generally not put things after the "include rules.mk" line in Makefile.in.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 19 • 12 years ago

> While this might work, you should generally not put things after the "include rules.mk"
> line in Makefile.in.

Hmm.  So I wasn't sure what ordering to use there, in fact, so I looked at other Makefile.ins and copied what they did.  content/base/src/Makefile.in, content/canvas/src/Makefile.in, content/events/src/Makefile.in, and layout/base/Makefile.in all had it after the rules.mk include, so I stopped looking at examples and did that...

I can move it to before, if that's preferred, I guess.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 20 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #17)
> Comment on attachment 706494 [details] [diff] [review]
> part 2.  Add some utilities for working with selectors to inspector utils.
> 
> Mike, want to try this and let me know whether it does what you want?

Yes, it is perfect! Thx.

	David Baron :dbaron:
	Comment 21 • 12 years ago

Comment on attachment 706494 [details] [diff] [review]
part 2.  Add some utilities for working with selectors to inspector utils.

r=dbaron

(And I seem to have missed the introduction of FlushPendingLinkUpdates.)

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 22 • 12 years ago

Comment on attachment 706463 [details] [diff] [review]
part 1.  Inspector code should be IMPL_NS_LAYOUT.

[Approval Request Comment]
Regression caused by (bug #): ?
User impact if declined: Browser styles in devtools computed view throw error and display template content.
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): None, this source code has not changed in a long time.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 23 • 12 years ago

Comment on attachment 706494 [details] [diff] [review]
part 2.  Add some utilities for working with selectors to inspector utils.

[Approval Request Comment]
Regression caused by (bug #): ?
User impact if declined: Browser styles in devtools computed view throw error and display template content.
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): None, this source code has not changed in a long time.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 24 • 12 years ago

This patch changes binary compat.  It's not appropriate for release or beta.  It might be OK on Aurora.

Furthermore, it's not enough on its own, right?  As in, there need to be devtools-side changes to make use of the new APIs, I assume?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 25 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #24)
> This patch changes binary compat.  It's not appropriate for release or beta.
> It might be OK on Aurora.
> 

Okay, I will remove those flags but the computed view is broken all the way up to release version.

> Furthermore, it's not enough on its own, right?  As in, there need to be
> devtools-side changes to make use of the new APIs, I assume?

Hence bug 834187.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 26 • 12 years ago

OK.  Are you planning to land these, by the way?  I was going to, but just noticed the whiteboard and ownership...

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 27 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #26)
> OK.  Are you planning to land these, by the way?  I was going to, but just
> noticed the whiteboard and ownership...

It is late over here so I will leave it to you. I have removed the whiteboard comment and unassigned myself. Thanks.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 28 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/04f0afec7a03
https://hg.mozilla.org/integration/mozilla-inbound/rev/05524e628b83

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 29 • 12 years ago

And https://hg.mozilla.org/integration/mozilla-inbound/rev/dcc017a7cd4d to fix test bits.

	Ed Morley [:emorley]
	Comment 30 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/04f0afec7a03
https://hg.mozilla.org/mozilla-central/rev/05524e628b83
https://hg.mozilla.org/mozilla-central/rev/dcc017a7cd4d

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 31 • 12 years ago

Comment on attachment 706463 [details] [diff] [review]
part 1.  Inspector code should be IMPL_NS_LAYOUT.

As with bug 834187, we don't see the rush to uplift this - let it ride the trains and get proper bake time. At this point we're days away from taking Aurora to Beta and it seems unwise to take this on without more time on Aurora.