Get rid of the wrapper div element in Svg component

[mstahv]

Describe your motivation

As the Element API don't support SVG, we are currently adding div element around SVG graphics show with the Svg component (added in https://github.com/vaadin/flow/pull/15540). Would be nice to get rid of this obsolete element.

Describe the solution you'd like

Get rid of the obsolete wrapper div.

Describe alternatives you've considered

Let it be :-)

Additional context

-

[mstahv]

The wrapper div is still there.

[Legioth]

I wonder whether theres's a risk that application logic built using the current implementation might accidentally make assumptions based on the DOM structure, which means that removing the wrapper might cause regressions.

[Legioth]

Seems like all that is needed is to use the createElementNS method to actually create the element with the right namespace, but it's not necessary to use the corresponding setAttributeNS method for setting attributes (probably because the regular setAttribute by default uses the element's own namespace).

This means that it should be relatively easy to add enough namespace functionality to Element. We would just need to add a new optional property to ElementData for the namespace and then change the corresponding logic in SimpleElementBindingStrategy.create to look for the namespace property in the node in the same way that it finds the tag and use createElementNS instead in case a namespace is present. The other uses of getTag (e.g. hasSameTag) should probably also be updated to stay in sync.

[mstahv]

I don't think the regression risk is very high, but would of course be great to get it right from the beginning. And most essentially, it would open a door for far more advanced SVG usage - inside components. I can already see @samie updating his Tetris game to change couple of DOM attributes instead of re-painting the whole game with Canvas API on every move😎

You make it sound like would be rather easy - for somebody who already understands what is happening behind the scenes. If I had ever done anything with the core part of Flow (the Element API and how it is reflected to actual DOM on the browsers side), I'd definitely attack on this.

[Legioth]

I pushed a prototype of the concept to https://github.com/vaadin/flow/tree/element-namespace. What's missing from that prototype is to think about some special cases (noted by comments), adding JavaDocs and creating some proper regression tests.

[samie]

Is this compatible with org.w3c.dom and Batik? https://xmlgraphics.apache.org/batik/using/svg-generator.html

Not tetris, but currently I have a similar wrapper using StreamResource and dynamic SVG generated from using Batik. Here is quick sample which should be replaced with proper namespace element API instead of innerHtml:

protected String generateSvg() {
            DocumentBuilderFactory docBuilderFactory = DocumentBuilderFactory
                    .newInstance();
            DocumentBuilder docBuilder = docBuilderFactory.newDocumentBuilder();
            Document doc = docBuilder.newDocument();
            SVGGraphics2D graphic2d = new SVGGraphics2D(doc);
            graphic2d.setSVGCanvasSize(new Dimension(svgW,svgH));
            drawObjects(graphic2d);
            StringWriter out = new StringWriter();
            graphic2d.stream(graphic2d.getRoot(), out, true, false);
            return out.toString();
}

Another thing is that SVG documents support event listeners which now are registered through executeJs and globally generated method names in window. Doing that properly for the SVG elements and using Vaadin's event mechanisms directly would be really nice.

[mstahv]

Are you asking related to this div element that we wish or in general related to the new Svg component?

In general it should work just fine with the current implementation, but it seems to have the same inconvenience as our Html, Upload and Download that inputs and output are not necessarily in their best order. Might be best for app developer if they could pass an SvgWriter (a lambda taking in an output stream where to write).

After the #2842 is done (that Leif started to work on) we could parse the whole DOM and make those listeners registered without JS haxies through Element API. We could either parse the whole SVG DOM on the server side and create element tree. Or maybe provide a method (xpath 🤓) to map just some elements of a larger SVG DOM (would save quite some resources on large graphics) 🤔

Before getting to that level, I'm waiting for @Legioth also to remove Element APIs the lowercasing of attributes, that will cause some issues with SVG (as discussed in #2842).

So, lot of possibilities, but not there yet.

[Legioth]

Does anyone know what the rules are for preserving the case of attribute names for other namespaces than HTML and SVG? I don't think it's a good idea to remove the automatic lowercasing for attributes on HTML elements, but I'm not sure what to do with elements that have some other namespace.

[mstahv]

Who uses something else than lowercased attribute names? I'm quite sure there wouldn't be many regressions. We could maybe run some code analysis on Flow add-ons🤔

[Legioth]

With the amount of users we have, I'm quite sure that there will be someone who accidentally relies on the way it currently works.

Making it conditional on the namespace is such a small effort that it's not worth taking the risk. Unless someone presents a reason to do otherwise, I would make it so that case is always preserved if a namespace is defined for the element while keeping the existing logic that converts to lower case for elements without an explicitly defined namespace.

[mstahv]

performance? simplicity of API? If we would do it for V24 today, I think we should already get enough data if that (theoretical regressions) becomes a problem. And then re-visit.

[mstahv]

What other namespaces we support currently than html (5)? With html5 it shouldn't matter. The only regression risk is if developer themselves use both lowercased and non-lowercased attribute names in their code 🤷‍♂️

Created this to test: https://github.com/vaadin/flow/pull/15666

At least a lot of code is removed 😎

[samie]

That indeed sounds like small optimization to make. I don't think this affects my use cases, but should test properly.

[Legioth]

The only regression risk is if developer themselves use both lowercased and non-lowercased attribute names in their code

I would say that the main regression risk is if one developer uses one casing and another developer uses something else, rather than the same developer being inconsistent with themselves. One particular thing I've seen is to use camel case for compound words like tabIndex which clashes with tabindex that we use in e.g. Focusable::getTabIndex. Another thing is old-school HTML where everything is upper case.

Then it can also be argued that we should make sure the Element API works in the same way as the corresponding API in the browser. If the browser vendors think it's worthwhile to have the complexity to make htmlElement.getAttribute("foo") return the same value as htmlElement.getAttribute("FOO"), then who are we to argue against that?

[Legioth]

I've updated my prototype branch to stay consistent with how the browser treats attributes for SVG elements (preserves case) and elements without a defined namespace (case insensitive). The logic is extracted to a separate method so that we can make some specific explicitly defined namespace be case insensitive if we find a reason to do that.

[mstahv]

👍 Nice. Bu I still think we could just as well remove that sanity check as my father-in-law want use this product anyways.

Would you @Legioth rename the branch into feature/element-namespace, so that we would start getting feature snapshots and I can create the first ever SVG component as an add-on against it.

[Legioth]

I don't see what you father-in-law has to do with adhering to browser standards. 😒

I pushed the same commits as future/element-namespace as well.

[mstahv]

My point is that Java developers don't need our help to write attribute names in the same way in their code so we could make our core smaller, cheaper to maintain, faster and less buggy.