546170 - Fix CSS Rules viewer's commands

Status: RESOLVED INCOMPLETE

(Other Applications :: DOM Inspector, defect)

Description

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

Attachment: Some cleanup for the affected files

Bug 536012 broke isCommandEnabled. Again. This time, it breaks non-CSSStyleRules' (e.g., imports') context menu commands (file URI stuff). There are a number of other issues with the CSS rules viewer's commands:
* commands should take focus into account. I.e., when the rule pane is focused, the edit commands should be disabled
* toggling !important (e.g., in chrome://global/skin/toolbar.css) makes the row disappear
* Insert's undo removes the property instead of reverting to the previous value (if any)
* implement paste
* undoing a command from a viewer that is no longer visible throws errors
* unbalanced updateBatch in cmdEditDelete's undoTransaction makes bad things happen
* CSSDeclarations should be CSSPropertys
* bug 443978 nsIDOMCSSStyleDeclaration editing not supported for @font-face

Comment 1

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

Attachment: fix problems

Additionally, the structure used to represent CSS properties internally (e.g., for copy and paste) was called CSSDeclaration. This might have contributed to confusion in 536012. I fixed it.

Comment 2

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

Here's some commentary about what I don't like about my approach, and most of it is due to our not having good MVC:

The transactions are doing all kind of view-related stuff. The only problem is, before my patch, if you switch away from the CSS rules viewer to, say, the JavaScript object viewer after having altered a property in some way, the view no longer exists because |viewer| is destroyed when styleRules.xul gets unloaded. So the following line will cause an error if you try to hit undo:

viewer.mPropsBoxObject.beginUpdateBatch();

The approach I took was register all transactions and make use of the viewerDestroyed method, so after it's destroyed, the transactions don't attempt to mess with the views.

But I think the particular approach to redefine the transaction is less than ideal. It means that we basically have two methods doing the same thing, but one with view related stuff and one without it. This means code duplication in some instances, though not all of them.

What I eventually want to do is for a transaction to broadcast that it's doing something to the object, and it will have registered itself as an interested viewer with the Inspector. So when the transaction occurs, it sends out a message saying that something has happened, then the Inspector invokes a method on the viewer to handle the message, similar to the ObserverManager model that's used. If the viewer is destroyed, it will unregister itself with the Inspector, and whatever messages are being broadcast by the transaction are simply ignored, since there's no view to update.

A plus of better MVC is that multiple views will be handled much better. If, for some reason, I have two CSS rules viewers open showing the same rule, and I change some of the properties in one viewer, the other should reflect those changes. Right now, the second view will only be updated when you move your mouse over the tree row, or something else that forces tree repainting. This is pretty ugly for obvious reasons, but also, if you have a property selected in the second tree view, and delete a property in the first that appeared in the tree before it, the properties get shifted up, and the selection in the second tree is now the property that appears *after* the one you had originally selected.

The reason I didn't do any of this is because I considered "better support for multiple views" to be outside the scope of a "fix the commands" bug. Additionally, an ObserverManager-like approach as I outlined doesn't solve any of the problems that occur if a web page is messing with CSSOM. And that can only really be solved by something akin to event listeners for style, or additional inspector APIs in layout (though I think we need more inspector APIs in Core anyway...).

Something else you may be able to spot in the patch: say you make three changes to the listed properties; three transactions get pushed onto the undo/redo stack. Now hit undo three times to get back to the original state, and edit a property. The first three transactions get expunged from the undo/redo stack, as they should, but mRegisteredCommands is still holding onto them even though they'll never be used, because it's just a dumb list with no concept of the Inspector's undo/redo stack, and these zombie transactions will hang around until the viewer is unloaded.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 426931 [details] [diff] [review]
Some cleanup for the affected files

>-StyleRuleView.prototype.getChildCount = function SRV_GetChildCount(aRow)
>+StyleRuleView.prototype.getChildCount =
>+function SRV_GetChildCount(aRow)
nit: indent second line on these please (at least one other occurrence)

>   undoTransaction: function undoTransaction()
>   {
>     viewer.mPropsBoxObject.beginUpdateBatch();
>-    for (var i = 0; i < this.declarations.length; i++)
>+    for (let i = 0; i < this.declarations.length; i++)
>       this.rule.setProperty(this.declarations[i].property,
>                             this.declarations[i].value,
>                             this.declarations[i].important ? "important" : "");
nit: oh gosh add braces around this loop please

r=sdwilsh

You do not need sr for this change.

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 426932 [details] [diff] [review]
fix problems

>  /**
>   * Returns an array of row objects selected in the tree.
>+  * @param an (optional) array of indices to use, otherwise, the indices from
>+  * the view's selection are used instead
global nit: style should be:
@param aIndexes [optional]
       Desc here

>+  getSelectedRowObjects: function getSelectedRowObjects(aIndices)
s/[iI]ndices/[iI]ndexes]/g
In general, we are moving more to indexes instead of indices (at least with SQL speak, and I think we should try to standardize in general across the code base).

>+++ b/resources/content/utils.js	Sun Feb 14 18:58:26 2010 -0600
> cmdEditCopy.prototype.doTransaction = 
> function doTransaction() {
>   if (this.objects.length == 1)
>     viewer.pane.panelset.setClipboardData(this.objects[0],
>                                           this.objects[0].flavor,
>                                           this.objects.toString());
>   else
>-    viewer.pane.panelset.setClipboardData(this.objects,
>-                                          this.objects[0].flavor + "s",
>-                                          this.objects.join(this.objects[0].delimiter));
>+    viewer.pane.panelset
>+      .setClipboardData(this.objects, this.objects[0].flavors ?
>+                                        this.objects[0].flavors :
>+                                        this.objects[0].flavor + "s",
>+                        this.objects.join(this.objects[0].delimiter));
nit: brace this since these statements take up multiple lines

>+function CSSProperty(aProperty, aValue, aImportant) {
nit: opening brace on new line

global nit: two spaces after a period before starting a new sentence please.

>+++ b/resources/content/viewers/styleRules/styleRules.js	Sun Feb >+//////////////////////////////////////////////////////////////////////////////
>+//// Event Handling
nit: indent two spaces (and drop two slashes on the first line)

>-    finally {
>-      viewer.mPropsBoxObject.endUpdateBatch();
>+    finally { }
just drop the finally now.  You keep this in a few places.

r=sdwilsh

Comment 5

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

(In reply to comment #3)
> nit: indent second line on these please (at least one other occurrence)

Huh? Like:

> StyleRuleView.prototype.getChildCount =
>   function SRV_GetChildCount(aRow)
>   {
>     if (aRow >= 0) {
>       var rule = this.mSheetRules[aRow];
>       if (rule instanceof CSSImportRule) {
>         return rule.styleSheet ? rule.styleSheet.cssRules.length : 0;
>       }
>       if (rule instanceof CSSMediaRule ||
>           rule instanceof CSSMozDocumentRule) {
>         return rule.cssRules.length;
>       }
>     }
>     return 0;
>   }

?

> nit: oh gosh add braces around this loop please

Hm. I wonder why I didn't already do that, given bug 532355, comment 22, and that my own preference is to brace for even single statements.

> You do not need sr for this change.

Is this true for all future "cleanup" patches?

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> Huh? Like:
> 
> > StyleRuleView.prototype.getChildCount =
> >   function SRV_GetChildCount(aRow)
> >   {
No StyleRuleView.prototype.getChildCount =
  function SRV_GetChildCount(aRow)
{
...
}
(I'm not super happy about either version to be honest, but I think this is more consistent with our style)

> Is this true for all future "cleanup" patches?
Yes, but I will still be explicit.

Comment 7

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

(In reply to comment #6)
> No StyleRuleView.prototype.getChildCount =
>   function SRV_GetChildCount(aRow)
> {
> ...
> }
> (I'm not super happy about either version to be honest, but I think this is
> more consistent with our style)

So... this is new. (e.g., attachment 426946 [details] [diff] [review] for bug 501178)

> > Is this true for all future "cleanup" patches?
> Yes, but I will still be explicit.

Thanks.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> So... this is new. (e.g., attachment 426946 [details] [diff] [review] for bug 501178)
Note that sometimes I miss things ;)

Comment 9

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

What about getters defined with __defineGetter__? E.g.,

> StylePropsView.prototype.__defineGetter__("rowCount",
> function()
> {
>   return this.mDec ? this.mDec.length : 0;
> });

Same?

Comment 10

[[e-mail]]

[I always thought there were two sorts of index; the first is the term for an offset into an array, and has the plural indices; the second is the term for a list usually of offsets into an array sorted by a property of the members of the array, and has the plural indexes. Therefore indexes are lists of indices!]

Comment 11

[[e-mail]]

Comment on attachment 426932 [details] [diff] [review]
fix problems

[Just a couple of things I happened to notice on a cursory browse.]

>   getSelectedIndices: function getSelectedIndices()
>   {
>     var indices = [];
>     var rangeCount = this.selection.getRangeCount();
>     for (var i = 0; i < rangeCount; i++) {
>       var start = {};
>       var end = {};
>       this.selection.getRangeAt(i,start,end);
>       for (var c = start.value; c <= end.value; c++) {
>         indices.push(c);
>       }
>     }
>-    return indices;
>+    return indices.sort(function IBTV_compare(a, b)
>+    {
>+      if (a < b) {
>+        return -1;
>+      }
>+      if (b < a) {
>+        return 1;
>+      }
>+      return 0;
>+    });
I could be wrong, but I thought that tree selections guaranteed their ranges were sorted. I also would have thought that the default sort function sufficed.

>+    var indices = aIndices;
>+    if (!indices) {
>+      indices = this.getSelectedIndices();
>+    }
var indices = aIndices || this.getSelectedIndices(); perhaps?

>+    viewer.pane.panelset
>+      .setClipboardData(this.objects, this.objects[0].flavors ?
>+                                        this.objects[0].flavors :
>+                                        this.objects[0].flavor + "s",
>+                        this.objects.join(this.objects[0].delimiter));
this.objects[0].flavors || this.objects[0].flavor + "s"

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> What about getters defined with __defineGetter__? E.g.,
In general you shouldn't need to wrap.

> Same?
In the case you cited, it all fits within the 80 characters, so it shouldn't matter ;)

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> [I always thought there were two sorts of index; the first is the term for an
> offset into an array, and has the plural indices; the second is the term for a
> list usually of offsets into an array sorted by a property of the members of
> the array, and has the plural indexes. Therefore indexes are lists of indices!]
Sadly, the Internet doesn't really have a consensus, so we've been trying to just standardize on something simple (index and indexes).

Comment 14

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

(In reply to comment #11)
> I could be wrong, but I thought that tree selections guaranteed their ranges
> were sorted.

Is that the case? I tried to make it out, but I was under the impression that
even if it would (which I don't think I ever came to a conclusion on), there's
nothing that says it *will* be that way, so I thought it best not to rely on
it.

> I also would have thought that the default sort function sufficed.

Default sort is lexicographic:

js> ([2,1,10,22]).sort()
1,10,2,22

(In reply to comment #12)
> In general you shouldn't need to wrap.
...
> In the case you cited, it all fits within the 80 characters, so it shouldn't
> matter ;)

I was doing a function-expression-always-starts-on-the-next-line thing, not
wrapping, since some of the code elsewhere does this.

So just do wrapping?

Comment 15

[[e-mail]]

Google Fight says indices are 40% more popular, despite not being in the Firefox en-US dictionary.

Ironically sqlite3 seems to use indices when it means indexes.

Also, I forgot that js isn't as clever as Perl. But return a - b; should work.

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> Google Fight says indices are 40% more popular, despite not being in the
> Firefox en-US dictionary.
Right, but are people using it properly in all those instances?

> Ironically sqlite3 seems to use indices when it means indexes.
*shrug*

Comment 17

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

Attachment: Cleanup other touched files

Comment 18

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

Attachment: address review, implement cut, fix selection issue for undo insert, proper use of "declarations" and "rules", reorder Edit menuitems

r?sdwilsh on both new patches since first cleanup was incomplete, and realchanges does some extra changes as well

Comment 19

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

Unfortunately, the problem with the growing mRegisteredCommands isn't fixed. It will keep growing with each (non-transient) command, even if some commands are no longer needed for undo/redo. Native TransactionManager is really inflexible, and doesn't allow any callbacks (e.g., during |clear|) so you could do something like call a destroy method on the command, which would deregister it. If it's a problem, we could always do our own nsITransactionManager implementation that could those things.

Comment 20

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

I posed a question to Shawn in #developers today about DOM Inspector 2.0.5 (bug 530187), and it turned into mention of this bug. The following comments are scattered throughout the channel, so I'm posting them here to make it easier to follow (and for others):

< crussell> sdwilsh: you probably got bugmail about bug 531121, but since bug 530187 isn't marked dependent, I'll ask: any plans to do a DOMi 2.0.5 on AMO?
< crussell> sdwilsh: although I'm kind of hesitant to ask; one of the bugs 2.0.5 depends on is marked fixed (bug 536012), but I broke some other other stuff while doing it
< crussell> sdwilsh: I decided to let it rest and take care of it in bug 546170 since I was attacking commands in the CSS rules viewer, but if we do 2.0.5 without including that fix, isCommandEnabled is still broken
< crussell> sdwilsh: I could pull out the isCommandEnabled stuff from bug 546170, reopen 536012, and fix it there, if that's more agreeable
< crussell> swdilsh: let me know what you want me to do when you get a chance to look it it
< crussell> sdwilsh: the broken behavior is that "Copy URI" and "View URI" are broken when you're using the style sheets viewer in the left pane, and the selected CSS rule is a non-CSSStyleRule (e.g., @import)

Comment 21

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

Comment on attachment 431036 [details] [diff] [review]
address review, implement cut, fix selection issue for undo insert, proper use of "declarations" and "rules", reorder Edit menuitems

Bitrot due to changes for bug 536012.

Comment 22

[Firefox Bug Husbandry Bot]

Bulk close. This component is no longer supported or maintained.

https://bugzilla.mozilla.org/show_bug.cgi?id=1499023

Comment 23

[Firefox Bug Husbandry Bot]

Bulk close. This component is no longer supported or maintained.

https://bugzilla.mozilla.org/show_bug.cgi?id=1499023