User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b5) Gecko/20091204 Firefox/3.6b5 (.NET CLR 3.5.30729) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b5) Gecko/20091204 Firefox/3.6b5 (.NET CLR 3.5.30729) The most recent version of DOM inspector no longer allows users to edit the CSS attributes individually through the CSS Rules view (object menu-top right window pane). Users are still able to edit the CSS properties under the DOM Node view, however this is limiting as you must edit the entire string of CSS attributes instead of a single property. Reproducible: Sometimes Steps to Reproduce: 1.Navigate to any page with CSS applied to various elements 2.Open DOM Inspector 3.Select a node in the DOM (Div, iframe, Span..etc) 4.in the top right hand pane, click the drop down and choose "CSS Rules" 5.in the bottom right pane select CSS attribute property 6.Right click to edit 7.menu is grayed out Actual Results: Unable to edit/insert CSS attributes. However, if you edit the CSS string under the menu option " DOM Node", you can now apply/edit the styles. Expected Results: In previous versions we were able to edit these attributes direction and individually under the CSS Rules menu option It would be very helpful to be able to use this tool as we could with previous versions or to understand what exactly changed and why.

And you're not trying to edit resource stylesheets (bug 343508)? You said this is sometimes reproducible; what's an example of a page on which this occurs?

No, definitely not trying to edit resource style sheets. The easiest way to reproduce this is to open a page and grab a div that a flash ad unit is served into. http://www.flickr.com/photos/45820419@N02/4205847925/sizes/l/ I hope this helps, let me know if you need more information.

Caused by my use of the newly-introduced getSelectedRule in isCommandEnabled in bug 212754, neglecting to take into account mStyleAttribute.

Doesn't this break Insert?

In what way?

Insert needs an editable rule, but it doesn't need an existing declaration.

For which rules should Insert be enabled where it's possible for no declaration to exist? The only rules I can think of that don't have corresponding declarations are @charset, @media, and @import (and CSSUnknownRule), but insertion doesn't make sense here. (Rules with no properties in the declaration block still have CSSStyleDeclarations.)

Comment on attachment 418926 [details] [diff] [review] fix brokenness in isCommandEnabled introduced in fix to 212754 [Checkin: Comment 10] Sorry, I had confused declarations with properties. So, each rule in the upper pane may or may not have a declaration, which then may have any number of properties. And the problem you introduced was with the style="" fake row, which isn't in the array of rules, because it is only a declaration. And anyway, all you're really doing is reverting changes from bug 212754. > return this.mPropsTree.view.selection.count > 0; >@@ -152,17 +155,17 @@ StyleRulesViewer.prototype = > return isEditable && this.mPropsTree.view.selection.count > 0; (Unfortunate break in context; it would have been nice to see the lines!) > return isEditable && this.mRuleTree.view.selection.count == 1; (Looks like bug 353006 made the count check redundant.)

Comment on attachment 418926 [details] [diff] [review] fix brokenness in isCommandEnabled introduced in fix to 212754 [Checkin: Comment 10] r=sdwilsh

Comment on attachment 418926 [details] [diff] [review] fix brokenness in isCommandEnabled introduced in fix to 212754 [Checkin: Comment 10] http://hg.mozilla.org/dom-inspector/rev/b851732cc610

(In reply to comment #8) > (Looks like bug 353006 made the count check redundant.) (You may want to file a bug about that...)

I think I should note here (since my other mentions of it are in other places, so you wouldn't know it from following this bug), this patch broke isCommandEnabled in other ways. I started to take care of it in bug 546170, you can see bug 546170, comment 0 and bug 546170, comment 20 for some discussion of it. (Turns out there's a reason I did what I did in bug 212754, which caused this bug. The real solution is a mixture of the approach there and the patch here.)

(In reply to comment #8) > (From update of attachment 418926 [details] [diff] [review]) > Sorry, I had confused declarations with properties. So, each rule in the upper > pane may or may not have a declaration, which then may have any number of > properties. And the problem you introduced was with the style="" fake row, > which isn't in the array of rules, because it is only a declaration. Yeah, it probably doesn't help the confusion (and might've actually caused it) that that elsewhere in inspector code it uses incorrect terminology to refer to properties as declarations. I've since mentioned this in bug 546170. > > return isEditable && this.mRuleTree.view.selection.count == 1; > (Looks like bug 353006 made the count check redundant.) I don't follow you, but I do know it has been redundant at least as far back as bug 212754, when I changed the rule tree to seltype="single", and the properties pane is disabled when the selection isn't a CSSStyleRule.

Comment on attachment 435252 [details] [diff] [review] isCommandEnabled fixery imported from 546170 r=sdwilsh

Comment on attachment 435252 [details] [diff] [review] isCommandEnabled fixery imported from 546170 >+ var isEditable = !(/^resource:/.test(fileURI) || >+ rule instanceof CSSFontFaceRule); So, this is relying on the context menu being unavailable when no declaration is selected? Is that desirable?

Comment on attachment 435252 [details] [diff] [review] isCommandEnabled fixery imported from 546170 (In reply to comment #16) > (From update of attachment 435252 [details] [diff] [review]) > >+ var isEditable = !(/^resource:/.test(fileURI) || > >+ rule instanceof CSSFontFaceRule); > So, this is relying on the context menu being unavailable when no declaration > is selected? Is that desirable? ? There are two menus: a context menu for the rules pane, and a context menu for the properties pane. The latter *is* supposed to be inaccessible when a rule with no declaration is selected, because the tree in that pane is disabled. (But I'm not sure about the quoted lines' significance here.) However, the "standard" commands in that menu (copy, insert, delete) are also present in the Edit menu in the menubar. I did spot an issue that arises with these lines, though. It's due to the tree focus stuff I was relying on in bug 546170 that I didn't bring along. Is this what you were talking about?

The whole point of the early return was a shortcut for a set of conditions that only occur when the viewer is first loaded and no rule is selected. It returns the correct result without the early return, it wasn't really much of an optimization anyway, and taking it out means shorter, more understandable code, so I just took it out. (I found the magic check to use to correct it, but I'd rather just take it out, considering readability and how many times I've messed this up before.)

Comment on attachment 443653 [details] [diff] [review] just drop the early return r=sdwilsh Sorry this took so long.

Comment on attachment 443653 [details] [diff] [review] just drop the early return I'm still marking this sr?neil. If someone feels like this should be different (based on gavin's comments on IRC), feel free to change it to addl-review? or what-have-you.

(In reply to comment #17) > However, the "standard" commands in that menu (copy, insert, delete) are also > present in the Edit menu in the menubar. Whoa, I never noticed that. Now I can insert CSS more quickly :-)

(In reply to comment #21) > (In reply to comment #17) > > However, the "standard" commands in that menu (copy, insert, delete) are also > > present in the Edit menu in the menubar. > Whoa, I never noticed that. Now I can insert CSS more quickly :-) Well, for now, anyway, until this patch forces me to tab to the declarations.

(In reply to comment #22) > declarations. "properties"? (In reply to comment #22) > Well, for now, anyway, until this patch forces me to tab to the declarations. We need to get keyboard shortcuts on these someday.

(In reply to comment #23) > (In reply to comment #22) > > declarations. > "properties"? Whatever. I'll have to reread comment #14 a few times ;-) (In reply to comment #16) >(From update of attachment 435252 [details] [diff] [review]) >>+ var isEditable = !(/^resource:/.test(fileURI) || >>+ rule instanceof CSSFontFaceRule); >So, this is relying on the context menu being unavailable when no declaration >is selected? Is that desirable? The latest patch "fixes" this by requiring that the properties tree have focus?

Comment on attachment 443653 [details] [diff] [review] just drop the early return >+ var propFocus = this.mFocusedTree && this.mFocusedTree == this.mPropsTree; Nit: don't need to null-check mFocusedTree to compare it to mPropsTree. >+ // The first three of these are context-sensitive; until they are >+ // supported for the rule pane, they are meaningless when it has focus. > case "cmdEditCopy": >- return this.mPropsTree.view.selection.count > 0; >+ return propFocus && propCount > 0; > case "cmdEditDelete": > case "cmdTogglePriority": >- return isEditable && this.mPropsTree.view.selection.count > 0; >+ return isEditable && propCount > 0; Except you forgot the && propFocus && on this one ;-) sr=me with this fixed. [I wrote && propFocus && because comparison with the previous rule suggests it should be tested before propCount and comparison with the following rule suggests it should be tested after isEditable.]

> (In reply to comment #16) > >(From update of attachment 435252 [details] [diff] [review]) > >>+ var isEditable = !(/^resource:/.test(fileURI) || > >>+ rule instanceof CSSFontFaceRule); > >So, this is relying on the context menu being unavailable when no declaration > >is selected? Is that desirable? > The latest patch "fixes" this by requiring that the properties tree have focus? What is "this"? The bug? The patch doesn't necessarily fix the bug as filed. The bug as filed was fixed with the first patch that got checked in (attachment 418926 [details] [diff] [review]). The first patch broke isCommandEnabled for non-style rules, though. (Try the Style Sheets viewer in the left pane and the CSS Rules viewer in the right pane. Bring the context menu up on an @import rule, and notice that the commands are disabled. This is because the first patch returned false if the selected rule doesn't have a declaration. @import and the other non-style rules that can appear if you're using the Style Sheets viewer in the left pane do not have declarations.) The bug as filed was caused by the patch for bug 212754. The most recent iterations of this bug's patches just ensure that the buggy cases in that the patch for 212754 and attachment 418926 [details] [diff] [review] now work correctly. The whole propFocus thing is pretty auxiliary, but it's something I was doing in bug 546170 anyway, and when I didn't carry it over, I noticed something in the last patch I did (attachment 435252 [details] [diff] [review]) that made resulted in a buggy case because I didn't carry over the focus stuff. I've since forgotten what those set of conditions were. But I just carried over the focus stuff rather than work around it since the focus stuff is pretty small/simple, and I was already doing it anyway. (In reply to comment #25) > (From update of attachment 443653 [details] [diff] [review]) > >+ var propFocus = this.mFocusedTree && this.mFocusedTree == this.mPropsTree; > Nit: don't need to null-check mFocusedTree to compare it to mPropsTree. > > >+ // The first three of these are context-sensitive; until they are > >+ // supported for the rule pane, they are meaningless when it has focus. > > case "cmdEditCopy": > >- return this.mPropsTree.view.selection.count > 0; > >+ return propFocus && propCount > 0; > > case "cmdEditDelete": > > case "cmdTogglePriority": > >- return isEditable && this.mPropsTree.view.selection.count > 0; > >+ return isEditable && propCount > 0; > Except you forgot the && propFocus && on this one ;-) sr=me with this fixed. > [I wrote && propFocus && because comparison with the previous rule suggests it > should be tested before propCount and comparison with the following rule > suggests it should be tested after isEditable.] You're right. But I split off cmdTogglePriority from cmdEditDelete in that case, since cmdTogglePriority doesn't require propFocus (in the same way cmdEditEdit, cmdCopySelectedFileURI, and cmdViewSelectedFileURI don't need propFocus). Geez, this would all be a lot simpler if I'd gotten this right the first time.

Comment on attachment 444914 [details] [diff] [review] with focus, drop the early return [r=sdwilsh: Comment 19, sr=neil: Comment 25, Checkin: Comment 27] http://hg.mozilla.org/dom-inspector/rev/e1b6bb30710d