Add support for inherited properties to the rule view.

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: dcamp, Assigned: dcamp)

Tracking

9 Branch
Firefox 10
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
Whiteboard: [styleinspector]
Is this for the style inspector?
Whiteboard: [styleinspector]
(In reply to Michael Ratcliffe from comment #1)
> Is this for the style inspector?

nope!
(Assignee)

Updated

6 years ago
Assignee: nobody → dcamp
Status: NEW → ASSIGNED
Priority: -- → P2
(Assignee)

Comment 3

6 years ago
Created attachment 570883 [details] [diff] [review]
Inherited rules in the rule view 1
Attachment #570883 - Flags: review?(rcampbell)
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.
Attachment #570883 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 5

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

Comment 6

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

Comment 7

6 years ago
Created attachment 571241 [details] [diff] [review]
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?
Attachment #571241 - Flags: feedback?(dbaron)
(Assignee)

Comment 8

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

Moved this patch over to bug 699592, cancelling feedback request.
Attachment #571241 - Flags: feedback?(dbaron)
(Assignee)

Updated

6 years ago
Attachment #571241 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
Created attachment 572140 [details] [diff] [review]
v2

Patch updated to use new inIDOMUtils::isInheritedProperty().
Attachment #570883 - Attachment is obsolete: true
Attachment #572140 - Flags: review?(rcampbell)
(Assignee)

Updated

6 years ago
Depends on: 599592
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.
Attachment #572140 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 11

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

Comment 12

6 years ago
https://hg.mozilla.org/integration/fx-team/rev/3246d9846071
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3246d9846071
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
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?
isInheritedProperty should be documented at https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/inIDOMUtils.

Sebastian
You need to log in before you can comment on or make changes to this bug.