192841 - CSS Style Rules lower panel breaks when selected rule is not a style rule

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Alex Vincent [:WeirdAl]]

Testcase:  
(1) Find a document with a stylesheet attached containing an @media rule, and
inspect in DOM Inspector.
(2) Left panel to Stylesheets.  Select the stylesheet.
(3) Right upper panel, select the at-rule.

Warning: reference to undefined property aRule.style
Source File: chrome://inspector/content/viewers/styleRules/styleRules.js
Line: 388

Lower panel doesn't update or go away.

Two issues:
(1) @charset, @namespace, @import, @media don't have any style property at all.
(2) The @media rule can contain an entire CSSRuleList which hasn't been parsed
yet by DOM Inspector.

Suggested solution:
(1) For rules which do not have a style property, set the lower panel to
display:none and do not update.  Otherwise, set the lower panel to display:block
and update.
(2) Fix the parser in the upper frame so the @media rule becomes a <treeitem
container="true"> with a different label specifying the media only, and child
tree items representing its CSSRuleList.

Feedback?

Comment 1

[Alex Vincent [:WeirdAl]]

Reassigning DOM-I bugs which have stagnated in my buglist back to default owner.  Hopefully someone will pick up some of these bugs and work on them.  I'll continue to follow them.

Comment 2

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: fixup viewer to treat @import, @media, and @-moz-document as trees and sanely handle style-less rules

Some notes about the way I handled this:

I disabled the lower panel rather than setting it to display: none. I toyed with various ways including collapsing the lower panel, but found disabling it to be the most reasonable.

The upper panel is now a multi-level treeview as suggested with @media, and I handled @import and @-moz-document similarly. Treating @import this way duplicates the Style Sheets treeview behavior, but I really can't see any rationale to not do so in the face of implementing the others, other than simply to prevent encroaching on the Style Sheets viewer.

I added @font-face awareness to the treeview. The only problem is, the CSS spec is silly and @font-face syntax has been stupidly defined as

> @font-face {
>   ...
>   font-family: "Family Name";
>   ...
> }

rather than

> @font-face "Family Name" {
>   ...
> }

So those looking for a specific rule must check each @font-face rule. I considered changing the cell text to include the family name, but decided against misrepresenting the syntax of the CSS rule, although we've had @import do so for forever.

@import now includes the media-type if specified. A note against using |cssText| has been around since a 2001 checkin, but it works for me today, so it's been changed.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 386843 [details] [diff] [review]
fixup viewer to treat @import, @media, and @-moz-document as trees and sanely handle style-less rules

>+    // for non-style rules
>+    if (dec)
>+      this.mPropsTree.disabled = false;
>+    else
>+      this.mPropsTree.disabled = true;
just do this.mPropsTree.disabled = !!dec;

>+    for (var i = 0; i < aObject.cssRules.length; i++) {
nit: use let instead of var in the loop please

> StyleRuleView.prototype.getRuleAt =
> function SRV_GetRuleAt(aRow) 
>+  if (aRow >= 0) {
...
> StyleRuleView.prototype.getDecAt =
> function SRV_GetDecAt(aRow) 
>+  if (aRow >= 0) {
When would we get aRow being less than 0?  Also, I'd prefer an early return, in general, as opposed to wrapping the whole method in an if.

>+    if (rule instanceof CSSMozDocumentRule) { //@-moz-document
>+      var i = rule.cssText.indexOf('{');
>+      if (i-- != -1) {
>+        while (rule.cssText.charAt(i).search(/\s/) != -1) {
>+          --i;
>+        }
>+      }
>+      return rule.cssText.substr(0, i + 1);
Could you please add a comment explaining what you are doing here?
Also, no braces for one line while statement please, and use let instead of var for i.

Comment 4

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: correct style issues

(In reply to comment #3)
> (From update of attachment 386843 [details] [diff] [review])
...
> > StyleRuleView.prototype.getRuleAt =
> > function SRV_GetRuleAt(aRow) 
> >+  if (aRow >= 0) {
> ...
> > StyleRuleView.prototype.getDecAt =
> > function SRV_GetDecAt(aRow) 
> >+  if (aRow >= 0) {
> When would we get aRow being less than 0?  Also, I'd prefer an early return,
> in general, as opposed to wrapping the whole method in an if.
It's to be cautious. Tree view painting will result in this being called with a parameter of -1 when no rows are selected.

There are multiple code paths that result in the null return. For getRuleAt, it's -1 is passed or when the code in the try throws. For getDecAt, it's the same plus when the rule is not a style rule.

> >+    if (rule instanceof CSSMozDocumentRule) { //@-moz-document
> >+      var i = rule.cssText.indexOf('{');
> >+      if (i-- != -1) {
> >+        while (rule.cssText.charAt(i).search(/\s/) != -1) {
> >+          --i;
> >+        }
> >+      }
> >+      return rule.cssText.substr(0, i + 1);
> Could you please add a comment explaining what you are doing here?
> Also, no braces for one line while statement please, and use let instead of
> var for i.
Changed it to use a regular expression, so the last two are no longer relevant. Note added.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 389323 [details] [diff] [review]
correct style issues

>+  if (aRow >= 0) {
>+    if (this.mRules) {
>+      var rule = this.mRules.GetElementAt(aRow);
>+      try {
>+        return XPCU.QI(rule, "nsIDOMCSSStyleRule");
>+      } catch (ex) {
>+      }
>+     }
nit: wrong indentation there
nit: catch on new line after } (like how else is done)

>+    else
>+      return this.mSheetRules[aRow];
nit: use braces with the else since you used them for the if please

>+  if (aRow >= 0) {
>+    if (this.mRules) {
>+      if (this.mStyleAttribute && aRow + 1 == this.rowCount) {
>+        return this.mStyleAttribute;
>+      }
>+      var rule = this.mRules.GetElementAt(aRow);
>+      try {
>+        return XPCU.QI(rule, "nsIDOMCSSStyleRule").style;
>+      } catch (ex) {
>+      }
>+     }
ditto

>+StyleRuleView.prototype.isContainer = function SRV_IsContainer(aRow)
>+{
>+  if (this.mSheetRules) {
>+    if (this.mSheetRules[aRow].type == CSSRule.IMPORT_RULE ||
>+        this.mSheetRules[aRow].type == CSSRule.MEDIA_RULE ||
>+        this.mSheetRules[aRow] instanceof CSSMozDocumentRule) {
>+        return true;
nit: return is indented too far.

>+    if (rule.type == CSSRule.IMPORT_RULE)
>+      // @import is tricky, because its styleSheet property is allowed to be
>+      // null if its media-type qualifier isn't supported, among other reasons.
>+      inserts = rule.styleSheet ? rule.styleSheet.cssRules : [];
nit: use braces here - the comments make this hard to see the control flow

r=sdwilsh with those changes

Neil should review this next.

Comment 6

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: further style to meet review

Comment 7

[[e-mail]]

Comment on attachment 393376 [details] [diff] [review]
further style to meet review

>+    // for non-style rules
>+    this.mPropsTree.disabled = !!dec;
Shawn, this is incorrect, you actually just need !dec :-P

>+      }
>+     }
>+    else {
>+      return this.mSheetRules[aRow];
>     }
>+   }
>+  return null;
Indentation got a bit messed up here.

>+      // get rule text up until the block begins, and trim off whitespace
>+      return /@-moz-document(?:\s+[^{\s]+)+/.exec(rule.cssText);
You do know that exec actually returns an array?
Anyway, I'd rather .replace(/\s*{.*/, "");

>+  for (var i = aRow + 1; i < rowCount; ++i) {
Start from aAfter + 1 [and you don't need to check for i > aAfter]

>+    if (this.mSheetRules[aRow].type == CSSRule.IMPORT_RULE ||
>+        this.mSheetRules[aRow].type == CSSRule.MEDIA_RULE ||
>+        this.mSheetRules[aRow] instanceof CSSMozDocumentRule) {
Unfortunately you're inconsistent in your -moz-document rule type checking; here you at least use instanceof but I think it would look nicer to make all the type tests use instanceof (or at least all the ones where -moz-document is one of the types being tested).

Comment 8

[[e-mail]]

Would it be too hard to make the rule column primary only for a stylesheet (rather than the list of CSS rules currently applying to an element)?

Comment 9

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: fix issues from initial superreview

(In reply to comment #7)
> (From update of attachment 393376 [details] [diff] [review])
> >+      // get rule text up until the block begins, and trim off whitespace
> >+      return /@-moz-document(?:\s+[^{\s]+)+/.exec(rule.cssText);
> You do know that exec actually returns an array?
> Anyway, I'd rather .replace(/\s*{.*/, "");

Yes, but its toString will produce the desired result since the array contains only one element, but I suppose we can do it the sane way if that's what you really want. Also dot doesn't match newlines (even with the multiline flag), so I went with [\s\S] per the JavaScript Reference recommendation, and I don't know why I wasn't doing the same before with the @media rules.

> >+  for (var i = aRow + 1; i < rowCount; ++i) {
> Start from aAfter + 1 [and you don't need to check for i > aAfter]
> 
> >+    if (this.mSheetRules[aRow].type == CSSRule.IMPORT_RULE ||
> >+        this.mSheetRules[aRow].type == CSSRule.MEDIA_RULE ||
> >+        this.mSheetRules[aRow] instanceof CSSMozDocumentRule) {
> Unfortunately you're inconsistent in your -moz-document rule type checking;
> here you at least use instanceof but I think it would look nicer to make all
> the type tests use instanceof (or at least all the ones where -moz-document is
> one of the types being tested).

Okay. Which is the proper way to do this, by the way? Components.interfaces.nsIDOMCSSMozDocumentRule or just CSSMozDocumentRule?

Comment 10

[[e-mail]]

I think DOM Inspector already uses the "shorthand" in other places.

Comment 11

[[e-mail]]

Comment on attachment 393484 [details] [diff] [review]
fix issues from initial superreview

One very minor thing that I noticed was that although you can't tab to the lower tree unless a style rule was selected you can still right-click it :-(

>-  if (this.mRules) {
>-    if (this.mStyleAttribute && aRow + 1 == this.rowCount) {
>-      return this.mStyleAttribute;
>+  if (aRow >= 0) {
>+    if (this.mRules) {
>+      if (this.mStyleAttribute && aRow + 1 == this.rowCount) {
[I wonder whether this should say aRow == this.mRules.Count()]

>+function() 
>+{
>+  if (this.mRules)
>+    return this.mRules.Count() + (this.mStyleAttribute ? 1 : 0);
>+  if (this.mSheetRules)
>+    return this.mSheetRules.length;
>+  return 0;
>+});
>+ 
Nit: trailing whitespace on the first and last lines above.

Comment 12

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: correctly disable trees and make context menus work correctly [Checkin: Comment 15]

(In reply to comment #11)
> (From update of attachment 393484 [details] [diff] [review])
> One very minor thing that I noticed was that although you can't tab to the
> lower tree unless a style rule was selected you can still right-click it :-(

You're right. Also, some code that should have been determining the the tree's selection was merely using |currentIndex|, which isn't necessarily a selected row. This meant inconsistency even among the rules view and the properties view.

> [I wonder whether this should say aRow == this.mRules.Count()]

I agree.

> 
> >+function() 
> >+{
> >+  if (this.mRules)
> >+    return this.mRules.Count() + (this.mStyleAttribute ? 1 : 0);
> >+  if (this.mSheetRules)
> >+    return this.mSheetRules.length;
> >+  return 0;
> >+});
> >+ 
> Nit: trailing whitespace on the first and last lines above.

Took care of it, as well as the other occurrences in the files I touched. I also took out some dead code.

Comment 13

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 394541 [details] [diff] [review]
correctly disable trees and make context menus work correctly
[Checkin: Comment 15]

Don't know whether I should carry over sr flag.

Comment 14

[Colby Russell :crussell (no longer Mozilla-ing)]

I'm not the assignee, so I can't tag this with the checkin-needed keyword.

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 394541 [details] [diff] [review]
correctly disable trees and make context menus work correctly
[Checkin: Comment 15]


http://hg.mozilla.org/dom-inspector/rev/ca35d7b7c443

Comment 16

[[e-mail]]

Comment on attachment 394541 [details] [diff] [review]
correctly disable trees and make context menus work correctly
[Checkin: Comment 15]

>   getSelectedDec: function SRVr_GetSelectedDec()
>   {
>     var idx = this.mRuleTree.currentIndex;
>-    return this.mRuleView.getDecAt(idx);
>+    return this.mRuleView.selection.count == 1 ?
>+             this.mRuleView.getDecAt(idx) :
>+             null;
>   },
> 
>   getSelectedProp: function SRVr_GetSelectedProp()
>   {
>+    if (this.mPropsView.selection.count != 1)
>+      return null;
>     var dec = this.getSelectedDec();
>-    var idx = this.mPropsTree.currentIndex;
>-    return dec.item(idx);
>+    // API awkwardness
>+    var min = {}, max = {};
>+    this.mPropsView.selection.getRangeAt(0, min, max);
>+    return dec.item(min);
Whoops, this always returns the first property, because getRangeAt fills in min.value as it's an outparam :-(

Comment 17

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #16)
> (From update of attachment 394541 [details] [diff] [review])
...
> Whoops, this always returns the first property, because getRangeAt fills in
> min.value as it's an outparam :-(

Sorry, what?

Also, from ./.irclogs/mozilla/#developers/#developers-2009-07-28.log:15:57
> < NeilAway> Enn: I happened to notice on DOMi's stylesheets display that the
> twisty updates when it is clicked but not when you use left/right arrow even
> though the tree expands/collapses, is that likely to be a tree issue or a
> DOMi issue?

I copied this behavior into the CSS Rules tree view. nsCertTree neglects to invalidate the row as well: <http://hg.mozilla.org/mozilla-central/file/5fca16b4c17f/security/manager/ssl/src/nsCertTree.cpp#l1331>

Comment 18

[[e-mail]]

(In reply to comment #17)
>(In reply to comment #16)
>>(From update of attachment 394541 [details] [diff] [review])
>>>+    var min = {}, max = {};
>>>+    this.mPropsView.selection.getRangeAt(0, min, max);
>>>+    return dec.item(min);
>>Whoops, this always returns the first property, because getRangeAt fills in
>>min.value as it's an outparam :-(
>Sorry, what?
Because JS only supports pass-by-value, you can't write back to a parameter. Instead, you pass in an object (i.e. var min = {};) and XPConnect writes the out value to the value property of that object. (I believe that a similar trick popular in Java is to pass an array of one element.) In this case, getRangeAt(0, min, max) effectively sets min.value and max.value to the range's extents. Unfortunately you pass min itself to dec.item and XPConnect has no idea what to do (I think it tries converting min to a string, and then to a double, and then to an integer, and ends up with zero).

Comment 19

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: get actual selection and invalidate twisty on toggleOpenState [Checkin: Comment 20]

I'll fix the Stylesheets viewer's twisty for bug 501178.

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 401539 [details] [diff] [review]
get actual selection and invalidate twisty on toggleOpenState
[Checkin: Comment 20]


http://hg.mozilla.org/dom-inspector/rev/f1fc2297e2c5

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 394541 [details] [diff] [review]
correctly disable trees and make context menus work correctly
[Checkin: Comment 15]


Please, don't obsolete pushed patches.