Closed
Bug 833808
Opened 12 years ago
Closed 12 years ago
Need way to get selector text, in specificity order, for all selectors matching a node
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: miker, Assigned: bzbarsky)
References
Details
Attachments
(3 files)
79.74 KB,
image/png
|
Details | |
1.85 KB,
patch
|
dbaron
:
review+
lsblakk
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
8.69 KB,
patch
|
dbaron
:
review+
lsblakk
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
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•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?
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
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
![]() |
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.
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.
![]() |
Assignee | |
Comment 5•12 years ago
|
||
OK, so what do you end up doing when the rule has multiple selectors that match the node?
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.
![]() |
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?
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
![]() |
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".
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.
![]() |
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.
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)
![]() |
Assignee | |
Comment 13•12 years ago
|
||
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+
![]() |
Assignee | |
Comment 16•12 years ago
|
||
![]() |
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?
Attachment #706494 -
Flags: feedback?(mratcliffe)
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.
Flags: needinfo?(gps)
![]() |
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.
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.
![]() |
Assignee | |
Updated•12 years ago
|
Attachment #706494 -
Flags: feedback?(mratcliffe) → review?(dbaron)
Reporter | ||
Updated•12 years ago
|
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+
Reporter | ||
Updated•12 years ago
|
Whiteboard: [has-patch] → [land-in-fx-team]
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.
Attachment #706463 -
Flags: approval-mozilla-release?
Attachment #706463 -
Flags: approval-mozilla-beta?
Attachment #706463 -
Flags: approval-mozilla-aurora?
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.
Attachment #706494 -
Flags: approval-mozilla-release?
Attachment #706494 -
Flags: approval-mozilla-beta?
Attachment #706494 -
Flags: approval-mozilla-aurora?
![]() |
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?
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.
Reporter | ||
Updated•12 years ago
|
Attachment #706463 -
Flags: approval-mozilla-release?
Attachment #706463 -
Flags: approval-mozilla-beta?
Reporter | ||
Updated•12 years ago
|
Attachment #706494 -
Flags: approval-mozilla-release?
Attachment #706494 -
Flags: approval-mozilla-beta?
![]() |
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...
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.
Assignee: mratcliffe → nobody
Whiteboard: [land-in-fx-team]
![]() |
Assignee | |
Comment 28•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/04f0afec7a03 https://hg.mozilla.org/integration/mozilla-inbound/rev/05524e628b83
Assignee: nobody → bzbarsky
![]() |
Assignee | |
Updated•12 years ago
|
Flags: in-testsuite?
Target Milestone: --- → mozilla21
![]() |
Assignee | |
Comment 29•12 years ago
|
||
And https://hg.mozilla.org/integration/mozilla-inbound/rev/dcc017a7cd4d to fix test bits.
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Attachment #706463 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Updated•12 years ago
|
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.
Description
•