Moved classes member to base element #38

JaimeIvanCervantes · 2021-03-27T04:08:56Z

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.

…sses

JaimeIvanCervantes · 2021-03-28T00:41:14Z

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

igagis · 2021-03-28T01:16:12Z

@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 · 2021-03-28T20:42:02Z

@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 😆, 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 · 2021-03-28T22:35:52Z

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 · 2021-03-28T22:37:43Z

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

JaimeIvanCervantes · 2021-03-28T23:49:47Z

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? 😛, 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 · 2021-03-29T00:04:26Z

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 <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.

According to https://www.w3.org/TR/SVG11/styling.html#InterfaceSVGStyleElement the <style> element is not styleable.
Did you find the example where <style> element can be queried by class somewhere on the web? Where is it from? Maybe the example is wrong?

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

Ok, then I think the names should be finder_by_* where * is id or tag or class. It can be written similar to the current finder class, i.e. first it would cache everything into the unordered_map and then it can be queried by id/tag/class multiple times.

Also, a minor optimization would be to change the std::map to std:unordered_map here

Probably yes, it can be changed to unordered_map.

JaimeIvanCervantes · 2021-03-29T04:34:36Z

Ok you're right, it looks like class and tag are part of the spec, I'll close this PR.

JaimeIvanCervantes added 2 commits January 18, 2021 13:08

Minor cloner test cleanup

70adc01

Moved the classes member as part of the base element

9c95a49

igagis self-assigned this Mar 27, 2021

JaimeIvanCervantes added 2 commits March 27, 2021 17:03

Merge remote-tracking branch 'upstream/master' into jaime/element-cla…

3dbb3f6

…sses

Added id and class attributes to text.svg and text.svg.cmp

0b43d93

JaimeIvanCervantes closed this Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved classes member to base element #38

Moved classes member to base element #38

JaimeIvanCervantes commented Mar 27, 2021 •

edited

Loading

JaimeIvanCervantes commented Mar 28, 2021

igagis commented Mar 28, 2021

JaimeIvanCervantes commented Mar 28, 2021

igagis commented Mar 28, 2021

igagis commented Mar 28, 2021

JaimeIvanCervantes commented Mar 28, 2021 •

edited

Loading

igagis commented Mar 29, 2021

JaimeIvanCervantes commented Mar 29, 2021

Moved classes member to base element #38

Moved classes member to base element #38

Conversation

JaimeIvanCervantes commented Mar 27, 2021 • edited Loading

JaimeIvanCervantes commented Mar 28, 2021

igagis commented Mar 28, 2021

JaimeIvanCervantes commented Mar 28, 2021

igagis commented Mar 28, 2021

igagis commented Mar 28, 2021

JaimeIvanCervantes commented Mar 28, 2021 • edited Loading

igagis commented Mar 29, 2021

JaimeIvanCervantes commented Mar 29, 2021

JaimeIvanCervantes commented Mar 27, 2021 •

edited

Loading

JaimeIvanCervantes commented Mar 28, 2021 •

edited

Loading