834187 - [Computed view] Restore processing of namespaced type selectors e.g. :not(svg|a)

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

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

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.

Comment 1

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

Attachment: Patch

Changes that take advantage of the new API.

Comment 2

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

Attachment: Patch v2

Fixed failing specificity test

Comment 3

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

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

Comment 4

[Joe Walker [:jwalker]]

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

Comment 5

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

Attachment: Patch v3

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

Comment 6

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

Need to wait for a blocker.

Comment 7

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

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:

Comment 8

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

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

Comment 9

[Paul Rouget [:paul]]

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

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

Comment 10

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

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

Comment 11

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

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

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 13

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

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

Comment 14

[Joe Walker [:jwalker]]

Attachment: rebase

Comment 15

[Joe Walker [:jwalker]]

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

Comment 16

[Paul Rouget [:paul]]

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

Comment 17

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

We use a lazy getter instead of the instance member.

Comment 18

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

Attachment: Test fix

Fixes domUtils reference in tests

Comment 19

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

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

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

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