Support Level 4 CSS Color (and Level 5 color-mix()) values in sRGB gamut

[ghost]

Replicate

1. Produce a Mermaid diagram having the following input (see Mermaid live or Kroki, or the attached diagram.svg.txt):

   sequenceDiagram
       Alice->>+John: Hello John, how are you?
       Alice->>+John: John, can you hear me?
       John-->>-Alice: Hi Alice, I can hear you!
       John-->>-Alice: I feel great
   

2. Rasterize the SVG diagram using EchoSVG.

Expected

Diagram renders (at least partially). Here's how Inkscape renders the diagram:

Actual

The following exceptions are thrown:

• org.w3c.dom.DOMException: The "stroke" property does not support HSLCOLOR values.
• java.lang.NullPointerException: Cannot invoke "io.sf.carte.echosvg.util.ParsedURL.toString()" because "uri" is null

The first error seems like something that the DOM parser should issue a warning about, but not stop parsing. This is Inkscape's behaviour and it would be useful if there was an API option to indicate that parsing should try to continue until an unrecoverable error. Missing colours aren't the end of the world. See CSSEngine.java, line 1194:

parseStyleSheet(ss, new InputSource(new StringReader(rules)), uri);

That eventually calls AbstractValueFactory.java, lines 59 to 63:

	protected DOMException createInvalidLexicalUnitDOMException(LexicalUnit.LexicalType type) {
		Object[] p = new Object[] { getPropertyName(), type.toString() };
		String s = Messages.formatMessage("invalid.lexical.unit", p);
		return new DOMException(DOMException.NOT_SUPPORTED_ERR, s);
	}

Having a way to reduce all similar exceptions in this class to warnings, rather than an exception, with the ability to register a warning listener (no-op listener by default) would be useful.

Forcing calling classes to register an "exception handler" would be a viable alternative, and could look like:

protected void handleInvalidLexicalUnitDOMException(LexicalUnit.LexicalType type) {
  Object[] p = new Object[] { getPropertyName(), type.toString() };
  String s = Messages.formatMessage("invalid.lexical.unit", p);
  mExceptionHandler.accept( new DOMException(DOMException.NOT_SUPPORTED_ERR, s) );
}

That would leave the choice about whether to throw an exception up to the calling code.

The second error is a bug. When uri is null, calling uri.toString() fails. See line 1202:

String s = Messages.formatMessage("stylesheet.syntax.error", new Object[] { uri.toString(), rules, m });

The root cause is in SVGOMStyleElement.java, lines 133 to 139, where it is assumed that a base URI is set for the document. That's not necessarily true (e.g., an SVG file can be streamed from a generator that has no URI). I traced the null to AbstractNode.java, line 435, which assigns the base URI to doc.getDocumentURI() (for an element that has no attributes). This lets the null pass through.

The simplest fix, of course, is not to call uri.toString() when uri is null. A deeper fix would be to correct the assumption of a URI that always exists and define a fake URI that can be used when no URI is otherwise determined.

[ghost]

See also: https://github.com/DaveJarvis/KeenWrite/discussions/166

[carlosame]

The following exceptions are thrown:

* `org.w3c.dom.DOMException`: The "stroke" property does not support HSLCOLOR values.

Fixed, now EchoSVG supports level 4 CSS colors as well as the Level 5 color-mix() function. The next image shows the before/after the patch, the previous render (left) was identical to Inkscape:

[...] seems like something that the DOM parser should issue a warning about, but not stop parsing. This is Inkscape's behaviour and it would be useful if there was an API option to indicate that parsing should try to continue until an unrecoverable error.
[...]
Having a way to reduce all similar exceptions in this class to warnings, rather than an exception, with the ability to register a warning listener (no-op listener by default) would be useful.

This is a known issue, and EchoSVG already does much better than Batik or Inkscape (unknown rules are just ignored instead of causing the whole stylesheet parsing to fail, which is what Batik does).

Fixing this in full is non-trivial due to the design of Batik's CSSEngine (for example one cannot just proceed with processing after any value error, due to that value potentially being part of a shorthand or a function).

That said, I have figured out a possible clean solution which I may implement in the future.

* `java.lang.NullPointerException`: Cannot invoke "io.sf.carte.echosvg.util.ParsedURL.toString()" because "uri" is null

[...] is a bug. When uri is null, calling uri.toString() fails. See line 1202:

String s = Messages.formatMessage("stylesheet.syntax.error", new Object[] { uri.toString(), rules, m });

The root cause is in SVGOMStyleElement.java, lines 133 to 139, where it is assumed that a base URI is set for the document. That's not necessarily true (e.g., an SVG file can be streamed from a generator that has no URI). I traced the null to AbstractNode.java, line 435, which assigns the base URI to doc.getDocumentURI() (for an element that has no attributes). This lets the null pass through.

The simplest fix, of course, is not to call uri.toString() when uri is null. A deeper fix would be to correct the assumption of a URI that always exists and define a fake URI that can be used when no URI is otherwise determined.

Documents are supposed to always have a documentURI set, and I encourage you to do that. It is used for security purposes, for example the same-origin policy which is used by default (although it can be configured to a less strict policy, CORS is not implemented and I doubt that it will ever be).

However, I have committed a fix so this NPE should never happen again.

[ghost]

Not sure if my comment made it into the PR before the change was committed. FWIW, the conditional around checking for null strings can be simplified using Objects:

import java.util.Objects; 

Object o1 = null;
Object o2 = "aString";
String s;

s = Objects.toString(o1, "isNull"); // returns "isNull"
s = Objects.toString(o2, "isNull"); // returns "aString"

[carlosame]

Not sure if my comment made it into the PR before the change was committed. FWIW, the conditional around checking for null strings can be simplified using Objects:

import java.util.Objects; 

Object o1 = null;
Object o2 = "aString";
String s;

s = Objects.toString(o1, "isNull"); // returns "isNull"
s = Objects.toString(o2, "isNull"); // returns "aString"

Thanks for the suggestion, although it wasn't a PR but a direct commit to the repository.

That said, if using Objects.toString is important for your peace of mind feel free to send a PR. 🙂 But it's the same thing internally and, unless the JVM inlines it, it may have to handle the stack because of entering a new method, which would be slower. So I wouldn't waste time.