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

In bug 832980 we decided to skip CSS selectors which contain namespaced type selectors e.g. :not(svg|a). This is because element.mozMatchesSelector(selector.text) raises an exception if we pass it one of these selectors.

Once we have an API from platform we need to make these rules visible again.

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

Changes that take advantage of the new API.

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

Fixed failing specificity test

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

Green on try:
https://tbpl.mozilla.org/?tree=Try&rev=a5c14f09658a

	Joe Walker [:jwalker]
	Comment 4 • 12 years ago

Comment on attachment 708146 [details] [diff] [review]
Patch v2

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

::: browser/devtools/styleinspector/CssLogic.jsm
@@ +470,5 @@
>  
>        rule.selectors.forEach(function (aSelector) {
>          if (aSelector._matchId !== this._matchId &&
> +           (aSelector.elementStyle ||
> +            this._selectorMatchesElement(rule._domRule, aSelector.selectorIndex))) {

It feels like we're invading someone else's _privates here. Perhaps this should be public?

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +1354,5 @@
>    this._create();
>  }
>  
>  RuleEditor.prototype = {
> +  domUtils: Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils),

Should this be a member of RuleEditor? Isn't it a jsm local variable declared at the top of the file?
Same for CssLogic.jsm

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

(In reply to Joe Walker [:jwalker] from comment #4)
> Comment on attachment 708146 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 708146 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleinspector/CssLogic.jsm
> @@ +470,5 @@
> >  
> >        rule.selectors.forEach(function (aSelector) {
> >          if (aSelector._matchId !== this._matchId &&
> > +           (aSelector.elementStyle ||
> > +            this._selectorMatchesElement(rule._domRule, aSelector.selectorIndex))) {
> 
> It feels like we're invading someone else's _privates here. Perhaps this
> should be public?
> 

It is fiddling with it's own privates but I have made it public instead.

> ::: browser/devtools/styleinspector/CssRuleView.jsm
> @@ +1354,5 @@
> >    this._create();
> >  }
> >  
> >  RuleEditor.prototype = {
> > +  domUtils: Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils),
> 
> Should this be a member of RuleEditor? Isn't it a jsm local variable
> declared at the top of the file?
> Same for CssLogic.jsm

Agreed, I have made them lazy getters.

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

Need to wait for a blocker.

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

Comment on attachment 709739 [details] [diff] [review]
Patch v3

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

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

[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.

	Paul Rouget [:paul]
	Comment 9 • 12 years ago

Comment on attachment 709739 [details] [diff] [review]
Patch v3

I don't think we want that in release, and merging is soon.

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

Really? Release version is broken but if we are not fixing it I am more than happy.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 11 • 12 years ago

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #10)
> Really? Release version is broken but if we are not fixing it I am more than
> happy.

I don't think shipping a chemspill release for such a bug is on the table.

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

Comment on attachment 709739 [details] [diff] [review]
Patch v3

This is old code, existing issue for many releases - we'll wait and let it ride the trains, there's no clear impetus here for uplift.

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

Will land after the next m-c -> fx-team merge

	Joe Walker [:jwalker]
	Comment 15 • 12 years ago

https://hg.mozilla.org/integration/fx-team/rev/53d22797d694

	Paul Rouget [:paul]
	Comment 16 • 12 years ago

This patch removes `domUtils` definition but uses it in `selectorMatchesElement`.

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

We use a lazy getter instead of the instance member.

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

Fixes domUtils reference in tests

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

https://hg.mozilla.org/integration/fx-team/rev/63f8d59ad8b9

	Tim Taubert [:ttaubert] (inactive)
	Comment 20 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/53d22797d694
https://hg.mozilla.org/mozilla-central/rev/63f8d59ad8b9