Moved classes member to base element

JaimeIvanCervantes commented 3 years ago

This PR is moving the classes member to the base element class, since classes are inherently part of the base element and should be part of every element type.

The rationale for this change, is that CSS and styleable, is not the only reason that one would like to access the classes of any element. The element <desc> is not styleable, yet could be accessed through Javascript for example (I know that svgdom does not support <desc>, it's just an example, and I might add it later).

NOTE: I need to do more test to make sure that I didn't break anything, but wanted to open the PR as soon as possible to hear your opinion @igagis. I also want to open a similar PR to move the tag to the base element as well.

JaimeIvanCervantes commented 3 years ago

@igagis I added a test for id and class attributes to text.svg and text.svg.cmp, this PR is ready

igagis commented 3 years ago

@JaimeIvanCervantes Actually, I'm not sure I understand why classes should be part of element instead of styleable.

The element <desc> is not styleable

It actually is styleable, according to SVG spec: https://www.w3.org/TR/SVG11/struct.html#InterfaceSVGDescElement As you can see from there, it inherits SVGStylable

... yet could be accessed through Javascript for example

Could you provide more details on the use case you are trying to achieve? What do you mean by accessing some SVG element from Javascript? I know that elements of html DOM can be accessed by Javascript and this is used quite extensively in frontend development. But I haven't heard about javascript accessing SVG elements. But I think it does not matter whether it is Javascript or C++, I see no problem accessing classes of some SVG element even if it is part of styleable as long the element inherits styleable. So, this means one has to check first that element is stylable and only then cast to styleable and get the classes. For convenience, it is possible to write utility function like vector<string> get_classes(element& e) which would return empty vector for non-styleable elements. A new file util.hpp could be created for this kind of helper functions, I think.

So, could you provide more details on your use case? Why do you need classes in element? Perhaps, what you want to do can be achieved in some other way without doing the change?

I know that svgdom does not support , it's just an example, and I might add it later

You're very welcome to add the element support to svgdom, as it is not supported just because I haven't had time to implement all elements (see #28), not because it is not supposed to support those.

JaimeIvanCervantes commented 3 years ago

@igagis I wanted to use a non-rendered element like <desc>, <title>, or <metadata> as an example, but you are right that <desc> is also SVGStyleable, so I choose a bad example :laughing:, my bad.

My use case is basically accessing elements by id, class, or tag with an API similar to Javascript, by using getElementsByClassName(), getElementById(), or getElementsByTag(), without this being dependent on an element being styleable. In theory, even the <style> element can be accessed by id, class, or tag. Here's an example of a script that would find the <style> element by class, and would display an alert with the corresponding id:

<html>
<body>  
<svg width="500" height="100">
    <style id="id1" class="class1">/* <![CDATA[ */
    #rect1 {
        stroke-width: 5;
    }
    /* ]]> */</style>

  <rect id="rect1" x="10" y="10" width="50" height="80" style="stroke:#000000; fill:none;"/>
</svg>

<input id="button1" type="button" value="Get id of style element" onclick="getIdFromStyle()"/>

<script>
    function getIdFromStyle() {
    alert(document.getElementsByClassName("class1")[0].id);
    }
</script>
</body>
</html>

One should be able to access elements by id, class, or tag for reasons other than setting the style. The most basic use-case would be a simple finder, that would find elements by class or tag (your finder already can access elements by id).

As for svgdom not supporting the <desc> element, that was more of a side note to mention that I might have some time to help adding support for this element in the future 😄, I understand that time is not infinite and you have a great library already!

igagis commented 3 years ago

the existing finder class is very specific thing, and I'm going to rename it to something like styles_cache, because it actually not only caches elements by their IDs, but also caches their style stacks.

For just searching purposes, a simpler visitors can be implemented (without caching style stacks):

For searching by ID we need to add a new finder_by_id class (or is the better name searcher_id? or finder_id?). For searching by class we need to add a new finder_by_class (searcher_class). Same for finder_by_tag (searcher_tag).

So, we can implement 3 new visitors for those cases, and since visitor in its visit() methods knows the exact element type, we don't have to cast. So, in case of styleables we can get classes right a way. So, in this case classes do not have to be part of base element. So, we are confirming to the SVG spec, saying that class attribute should only be at styleable elements.

Could you implement it that way?

The searchers must go to src/svgdom/util directory, which I have created in recent commits.

igagis commented 3 years ago

by the way, what class names for those visitors is better in your opinion?

JaimeIvanCervantes commented 3 years ago

Visitors would work, in fact I was planning of already implementing it this way, but I thought that having classes and tags as part of the base element made more sense. If the SVG spec says that class is part of SVGStyleable though, then this would be the way to go. I can't find any information about class belonging to SVGStyleable though, could you point me to this? I am very curious now.

Also, is <style> styleable? :stuck_out_tongue:, because if you try my example above, you will see that an <style> element can be queried by class as well. This is what I find very confusing.

by the way, what class names for those visitors is better in your opinion?

For the names of the finders/searchers, I would emulate Javascript:

findElementById(): returns nullptr or a single element.
findElementsByClassName(): returns nullptr, or one or more elements.
findElementsByTagName(): returns nullptr, or one or more elements.

In my case performance is very important too, so I was planning on using std::unordered_map to hash elements to id, and hash lists of elements to class and tag, to be able to query elements faster, instead of having to use visitors each time.

the existing finder class is very specific thing, and I'm going to rename it to something like styles_cache, because it actually not only caches elements by their IDs, but also caches their style stacks.

I agree that this would make more sense, I found this confusing at the very beginning. Also, a minor optimization would be to change the std::map to std:unordered_map here, since IDs are meant to be unique: https://github.com/cppfw/svgdom/blob/master/src/svgdom/util/finder.hpp#L37

igagis commented 3 years ago

I can't find any information about class belonging to SVGStyleable though, could you point me to this?

See here: https://www.w3.org/TR/SVG11/types.html#InterfaceSVGStylable the description of className field:

className (readonly SVGAnimatedString) Corresponds to attribute ‘class’ on the given element.

and the class attribute is the list of classes.

Also, is

cppfw / svgdom

Moved classes member to base element #38