Need support for undoing changes to className Attribute

[GoogleCodeExporter]

Because and SVG's className attribute is not a simple string value special 
support is needed from the undo ChangeElementCommand in order for the values to 
applied and unapplied correctly. Attached is a patch which will provide support 
(tests included).

Original issue reported on code.google.com by adamben...@gmail.com on 27 Mar 2012 at 10:15

Attachments:

• 0001-added-support-for-className-in-ChangeElementCommand.patch
• 0002-added-tests-for-new-ChangeElementCommand-behavior-to.patch

[GoogleCodeExporter]

What is the usecase for this? I don't think we support setting classes with the 
UI? If this is changing via the Edit Source then it will be messy to support 
undo's for that. It'll be partial for the most part and undoing attributes you 
can't see is pretty bad isn't it? Heck even changing id which is viewable in 
the UI cannot be undone...

Original comment by asyazwan on 28 Mar 2012 at 1:46

[GoogleCodeExporter]

I admit our use case might be unusual but here it is in a nutshell.

We are using svg-edit as a part of a diagraming tool to create power-systems 
(think electrical substations, not small scale) schematics, some times called 
online diagrams. A key component of these digrams is to color code lines and 
shapes according to the voltages that are present in the real life equipment. 
From a UI perspective this is manifest as a simple 'voltage selector' box which 
applies a choosen voltage to whatever is selected in the canvas. In order to 
color code each line/path/group etc. we make use of stylesheets where 
individual styles correspond to individual voltages. So to set a line to 100Kv 
we apply a class like .voltage-100kv to the line element. This is where my 
issue with jquery support for className arrose in the first place. 

Of course if we can add styles to components it only seemed natural to try and 
support the undo functionality. After taking a look at what was supported by 
the ChangeElementCommand it seemed like a natural fit for the 
applying/unapplying of the 'class' attribute. It was my understanding after 
taking a look at the ChangeElementCommand that in most cases it calls the 
setAttribute function on an element, which of course doesn't quite work in the 
case of the 'class' attribute so I added a special case that looks to be in 
line with the behavior for the #text condition. 

So in summary, we are not going through the source editor, but rather a 
standard UI metaphor which just happens to affect the canvas by setting the 
class attribute instead of directly setting stroke/fill etc.  One advantage of 
the stylesheet-based approach for us is that it makes the voltage colorings 
deploy-time configurable just by changing the stylesheet, which can the also be 
applied to other applications we have which will display the schematics created 
with this tool.

Have I missed something? Is there a better way to manage this kind of change in 
svg-edit?

Adam

Original comment by adamben...@gmail.com on 28 Mar 2012 at 3:31

[GoogleCodeExporter]

Apologies, it was I who completely missed the point. Now I understand that you 
want the option to be able to undo class changes. Somehow I misunderstood. I 
will review and test the patch.

Thanks.

ps: sounds like a cool project, similar to something recently launched, also 
SVG-based, if you want inspiration: https://www.circuitlab.com/editor/

Original comment by asyazwan on 28 Mar 2012 at 5:24

• Changed state: Accepted

[GoogleCodeExporter]

Thanks for taking another look and my apologies if my original description of 
the problem wasn't very cogent. I find that it is sometimes difficult to 
generalize an explanation to a very narrow problem. 

As for the project itself it really is a cool project, and this editor is only 
one component of a larger open-source smart grid management platform. We've 
been working on the whole project for a little over a year now in one form or 
another and things are really starting to come together. We hope to release our 
UI related work into open source in the next few months and Ill be sure to 
mention it on the mailing list here so you all can come take a look.

WRT to circuit lab, I have taken a look and they definitely have something very 
cool going on there. It looks like they have been influenced by Bret Victor and 
his direct manipulation approach. Very cool!

Thanks,
Adam

Original comment by adamben...@gmail.com on 28 Mar 2012 at 7:25

[GoogleCodeExporter]

I just properly read and tested the patch. It seems... unnecessary. I believe 
you can achieve the same effect with this changes to your test (but first undo 
your history.js changes):

        var line = document.createElementNS(svgns,'line');
        line.setAttributeNS('null', 'class', 'newClass');
        change = new svgedit.history.ChangeElementCommand(line,{class:'oldClass'});

        ok(change.unapply);
        ok(change.apply);
        equals(typeof change.unapply, typeof function(){});
        equals(typeof change.apply, typeof function(){});

        change.unapply();
        equals(line.getAttributeNS('null', 'class'), 'oldClass');

        change.apply();
        equals(line.getAttributeNS('null', 'class'), 'newClass');

And the test will pass.

Now you know time and time again why I keep mentioning *AttributeNS(). :P

Please verify this works for you and close the issue if it does.

Original comment by asyazwan on 29 Mar 2012 at 3:30

[GoogleCodeExporter]

correction: the 'null' namespace should really be null.

Original comment by asyazwan on 29 Mar 2012 at 3:50

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Btw, once Adam confirms that they don't need the patch, we should still add in 
the unit test :)

Original comment by codedr...@gmail.com on 29 Mar 2012 at 4:28

[GoogleCodeExporter]

This is an interesting result to be sure. The reason I am puzzled by this is 
that jQuery specifically avoids using setAttribute to assign class values to 
objects in favor of the obj.className approach which is why I figured a special 
case would be needed. I agree that if we can get away with setAttribute we 
should cause that gets rid of any special cases. If you don't mind I'd like to 
take a look around see if I can find out why jQuery doesn't also manage classes 
this way. It may be a cross browser thing or some other weirdness. Either way I 
will take a look today and get back to you.

Adam

Original comment by adamben...@gmail.com on 29 Mar 2012 at 3:23

[GoogleCodeExporter]

That was faster than I thought. If you google 'className vs setAttribute' you 
will find a few mentions on the subject, specifically related to IE. Here some 
good stackoverflow questions on on the topic: 

http://stackoverflow.com/questions/2490627/how-can-i-reliably-set-the-class-attr
-w-javascript-on-ie-ff-chrome-etc

http://stackoverflow.com/questions/6465269/ie7-and-setattribute-to-remove-classe
s

Bottom line is it looks like setAttribute is unreliable in IE so going straight 
for className is the preferred approach.

Adam

Original comment by adamben...@gmail.com on 29 Mar 2012 at 3:27

[GoogleCodeExporter]

Hmm I'm a bit torn on this. They claim IE7 and earlier to have unreliable 
setAttribute. Here are some of my arguments
- IE7 and earlier, do we really want to go there?
- is this claim valid for namespace-aware *AttributeNS() too? Can somebody test 
this? My google-fu has failed me
- If we want to use properties for setters/getters, how far must we go? How 
many rewrites need to be done? Just history.js?

I'm a little biased though since I'm spoiled by the option to support 
latest-and-greatest only. :P But I'm fine with applying the patch if you guys 
agree.

@Adam I see that Jeff uses #text notation to index his array. Can you do the 
same? Or am I missing something?

Original comment by asyazwan on 29 Mar 2012 at 5:36

[GoogleCodeExporter]

Interesting.  Here's my two cents:  We should code to the simplest, most 
consistent behavior that works for SVG elements in IE9+, Chrome, Safari, 
Firefox, Opera.  I thought the className DOM property was a HTML DOM thing 
only: http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-95362176 but I had 
forgotten that there is a corresponding attribute in SVG elements: 
http://www.w3.org/TR/2010/WD-SVG11-20100622/types.html#__svg__SVGStylable__class
Name

The thing I'd be worried about with your patch is if someone really did want

  <ellipse className="foo" class="bar" .../>

But perhaps that's not really a valid concern.

Adam, are you saying that setAttributeNS(null, "class", "foo") does not work 
for some of these browsers or are you concerned because of what jQuery does?  I 
would need some time to further dig into this issue, but it might just be a 
difference in behavior between HTML and SVG elements.  In some ways, SVG was an 
opportunity for IE to do things according to spec from the beginning and not 
have to worry about all the legacy content like they do with HTML...

Original comment by codedr...@gmail.com on 29 Mar 2012 at 6:02

[GoogleCodeExporter]

So I like the forward looking approach, I've been down the IE rabbit hole too 
many times for it to be worth it. I would prefer setAttributeNS but I will need 
to test it in the various browsers to validate the behavior - on that note -  
have you looked at jsTestDriver?, it can make running these kinds of cross 
browser tests a snap. As for my jQuery concern, I may have been conflating HTML 
and SVG there, trying to learn all the cross platform quirks jQuery has had to 
deal with, and there maybe no reason to treat SVG they way jQuery treats HTML.

I'll run some tests on setAttributeNS and report back.

Adam

Original comment by adamben...@gmail.com on 29 Mar 2012 at 7:50

[GoogleCodeExporter]

I can verify that by backing out my change to history.js and using 
setAttribute() & setAttributeNS(null,,) I see the correct behavior for 'undo' 
in the following browsers:

Mac OS: Chrome Stable, Firefox 11, Safari 5.1.5
Windows: Chrome Stable, Firefox 11, IE 9 

I believe we can safely close this feature request.

Original comment by adamben...@gmail.com on 29 Mar 2012 at 9:02

[GoogleCodeExporter]

Added your unit test in r2079.

Original comment by asyazwan on 3 Apr 2012 at 3:01

• Changed state: WontFix