696180 - Add support for inherited properties to the rule view.

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Comment 1

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

Is this for the style inspector?

Comment 2

[Rob Campbell [:rc] (:robcee)]

(In reply to Michael Ratcliffe from comment #1)
> Is this for the style inspector?

nope!

Comment 3

[Dave Camp (:dcamp)]

Attachment: Inherited rules in the rule view 1

Comment 4

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 570883 [details] [diff] [review]
Inherited rules in the rule view 1

+ * TODO: It wouldn't be too hard to expose inherited property
+ * information from the style system, that would be significantly more
+ * maintainable.
+ */
+CssLogic.INHERITED_PROPERTIES = {

this may be a dumb question, but where did you go for this list? CSS spec? Code? Maybe a link to your source in the comment would be helpful for the next person.

I hear your comment about maintainability and wonder how closely we'll have to watch for new inherited properties cropping up in the style system.

+  "-x-system-font" : true,

nit: only line with a space before the colon.

Do we need the : true parts? Wouldn't this also work as an array of strings instead of the object? I don't care either way, but it's a lot of extra typing your way. :)

Algorithm looks nice and the code is pretty.

Comment 5

[Dave Camp (:dcamp)]

(In reply to Rob Campbell [:rc] (robcee) from comment #4)
> Comment on attachment 570883 [details] [diff] [review] [diff] [details] [review]
> Inherited rules in the rule view 1
> 
> + * TODO: It wouldn't be too hard to expose inherited property
> + * information from the style system, that would be significantly more
> + * maintainable.
> + */
> +CssLogic.INHERITED_PROPERTIES = {
> 
> this may be a dumb question, but where did you go for this list? CSS spec?
> Code? Maybe a link to your source in the comment would be helpful for the
> next person.

Yeah, I'll add that comment - it was a careful reading of nsCssPropList.h.

FWIW, that "careful reading of nsCssPropList.h" is essentially what we'll need to do in code to support checking this at runtime.  nsCssPropList.h can be included with different definitions for the CSS_PROP macro - we can include it in a method that builds a hashtable to check against.

Will file a followup for that.  We should do it sooner rather than later.

> 
> I hear your comment about maintainability and wonder how closely we'll have
> to watch for new inherited properties cropping up in the style system.

We'll have to watch very closely unless we implement the platform solution.

> Do we need the : true parts? Wouldn't this also work as an array of strings
> instead of the object? I don't care either way, but it's a lot of extra
> typing your way. :)

Yeah, it was a bit of extra typing.  Just preferred a hash lookup to an array lookup.

Comment 6

[Dave Camp (:dcamp)]

(In reply to Dave Camp (:dcamp) from comment #5)

> FWIW, that "careful reading of nsCssPropList.h" is essentially what we'll
> need to do in code to support checking this at runtime.  nsCssPropList.h can
> be included with different definitions for the CSS_PROP macro - we can
> include it in a method that builds a hashtable to check against.

Ignore that, it's actually much simpler than that if we just want to check if a property is inherited.  Patch incoming.

Comment 7

[Dave Camp (:dcamp)]

Attachment: inIDOMUtils::isInheritedProperty

Still needs tests, comments, and a uuid bump.  But dbaron, is this a reasonable way to ask the style system if a given css property is inherited by default?

Comment 8

[Dave Camp (:dcamp)]

Comment on attachment 571241 [details] [diff] [review]
inIDOMUtils::isInheritedProperty

Moved this patch over to bug 699592, cancelling feedback request.

Comment 9

[Dave Camp (:dcamp)]

Attachment: v2

Patch updated to use new inIDOMUtils::isInheritedProperty().

Comment 10

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 572140 [details] [diff] [review]
v2

-    }
+    var domRules = this.domUtils.getCSSStyleRules(aElement);

can this not throw anymore or do we just want the exception to bubble? 

       // XXX: non-style rules.

do we still need the XXX?

solid.

Comment 11

[Dave Camp (:dcamp)]

(In reply to Rob Campbell [:rc] (robcee) from comment #10)
> Comment on attachment 572140 [details] [diff] [review] [diff] [details] [review]
> v2
> 
> -    }
> +    var domRules = this.domUtils.getCSSStyleRules(aElement);
> 
> can this not throw anymore or do we just want the exception to bubble? 

http://mxr.mozilla.org/mozilla-central/source/layout/inspector/src/inDOMUtils.cpp#187

Only exceptional returns there are invalid arguments and OOM, don't think we want to catch either of those.

> 
>        // XXX: non-style rules.
> 
> do we still need the XXX?

We have a bug on file and it's not clear that the fix belongs near that comment, so I'll pull the comment for now.


> solid.

Comment 12

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/3246d9846071

Comment 13

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/3246d9846071

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 572140 [details] [diff] [review]
v2

>--- a/browser/devtools/styleinspector/CssRuleView.jsm
>+++ b/browser/devtools/styleinspector/CssRuleView.jsm
>+    let element = this.element;
>+    do {
>+      this._addElementRules(element);
>+    } while ((element = element.parentNode) &&

Node.parentElement landed, no?

Comment 15

[Sebastian Zartner [:sebo]]

isInheritedProperty should be documented at https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/inIDOMUtils.

Sebastian