Need way to get selector text, in specificity order, for all selectors matching a node

RESOLVED FIXED in mozilla21

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: miker, Assigned: bzbarsky)

Tracking

unspecified
mozilla21
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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
(Assignee)

Comment 1

6 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?
Summary: mozMatchesSelector throws when string contains a namespaced type selector e.g. :not(svg|a) → Need way to get selector text, in specificity order, for all selectors matching a node
(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
(Assignee)

Comment 3

6 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.
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.
(Assignee)

Comment 5

6 years ago
OK, so what do you end up doing when the rule has multiple selectors that match the node?
Created attachment 705800 [details]
Style Inspector's Computed View

(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.
No longer blocks: 832980
(Assignee)

Comment 7

6 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?
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
(Assignee)

Comment 9

6 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".
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.
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.
> 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)
Created attachment 706463 [details] [diff] [review]
part 1.  Inspector code should be IMPL_NS_LAYOUT.
Attachment #706463 - Flags: review?(dbaron)
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?
Attachment #706463 - Flags: review?(dbaron) → review+
Needinfo counts as a line, right?  ;)
Flags: needinfo?(gps)
Created attachment 706494 [details] [diff] [review]
part 2.  Add some utilities for working with selectors to inspector utils.
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?
Attachment #706494 - Flags: feedback?(mratcliffe)
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.
Flags: needinfo?(gps)
> 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.
(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.
(Assignee)

Updated

6 years ago
Attachment #706494 - Flags: feedback?(mratcliffe) → review?(dbaron)
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Whiteboard: [has-patch]
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.)
Attachment #706494 - Flags: review?(dbaron) → review+
Whiteboard: [has-patch] → [land-in-fx-team]
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.
Attachment #706463 - Flags: approval-mozilla-release?
Attachment #706463 - Flags: approval-mozilla-beta?
Attachment #706463 - Flags: approval-mozilla-aurora?
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.
Attachment #706494 - Flags: approval-mozilla-release?
Attachment #706494 - Flags: approval-mozilla-beta?
Attachment #706494 - Flags: approval-mozilla-aurora?
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?
(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.
Attachment #706463 - Flags: approval-mozilla-release?
Attachment #706463 - Flags: approval-mozilla-beta?
Attachment #706494 - Flags: approval-mozilla-release?
Attachment #706494 - Flags: approval-mozilla-beta?
OK.  Are you planning to land these, by the way?  I was going to, but just noticed the whiteboard and ownership...
(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.
Assignee: mratcliffe → nobody
Whiteboard: [land-in-fx-team]
(Assignee)

Updated

6 years ago
Flags: in-testsuite?
Target Milestone: --- → mozilla21
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.
Attachment #706463 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #706494 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.