Css improvements - #173
Conversation
75df06a to
70a00b5
Compare
|
Thank you so much for the initiative. I will try to do a thorough review this weekend. |
|
Hmm, textRefTest failed likely because of the sans-serif font used on my system was different from the sans-serif fonts in the 3 CI systems... (JSVG now has Kerning, while the reference Batik rendering doesn't) |
|
Maybe we can make kerning a rendering hint and disable it for those tests. This way at least we test that letter placement doesn't break completely. |
The problem with making kerning a rendering hint is that kerning is used in places where the Output context is not available: GlyphRenderer.layoutGlyphRun and TextMetrics.computeTextMetrics. So SVGDocument.prepareRenderContext would need to put it into the RenderContext as a separate field or so. What do you think? |
|
We could store a boolean indicating that kerning is enabled in the GlyphCursor. The |
| this.sequences = Collections.unmodifiableList(new ArrayList<>(sequences)); | ||
| this.combinators = Collections.unmodifiableList(new ArrayList<>(combinators)); | ||
| this.specificity = computeSpecificity(); | ||
| } |
There was a problem hiding this comment.
Instead of using Collections.unmodifiableList I usually just use List<? extends Type as the return type functions. Even though this still allows someone to modify the collection, it is only possible through unsafe casting. If we document that the constructor takes ownership of the list, we can avoid the copy of the list in here.
This also applies to the other instances of the Collections.unmodifiableList(new ArraList<>(list)) pattern.
|
|
||
| /** Preceding element sibling, or null if first child */ | ||
| public @Nullable ParsedElement previousSibling() { | ||
| ParsedElement parent = parent(); |
There was a problem hiding this comment.
This hides the member variable with the same name. Please also check for other instances of this.
There was a problem hiding this comment.
fixed; found only one more case
| ParsedElement copyAsUseInstance(@NotNull NodeSupplier nodeSupplier) { | ||
| return deepCopy(nodeSupplier, null, 1, 1); | ||
| } | ||
|
|
There was a problem hiding this comment.
The most common case of elements referenced by <use> are ones which have a <defs> element as their parent. I suppose in this case one could simply "detach" the node, as it is never rendered directly (Please correct me if this is an incorrect assumption). If so, we should optimise this codepath.
| @Override | ||
| public @NotNull CssHints cssHints() { | ||
| return cssHints; | ||
| } | ||
|
|
There was a problem hiding this comment.
Mark these as experimental (these = css hints related functions and the class itself). In the long run the CssHints need to be moved into the PlatformSupport (or similar).
There was a problem hiding this comment.
done. But kept CssParser.parseStyleSheet/parseStyleAttribute as-is, not sure what's best there.
| private final static class ParsedElementAnnotated { | ||
| public final @NotNull ParsedElement element; |
There was a problem hiding this comment.
Modifier order should be static final. Also applies in other locations.
There was a problem hiding this comment.
fixed including the other instances
| // Only a declared viewBox may introduce a scaling transform. Synthesizing one from the | ||
| // element's own size would wrongly scale content when the use-site size differs. | ||
| if (info != null) node.renderWithSize(size, node.declaredViewBox(), info.context(), info.output()); |
There was a problem hiding this comment.
Can you add a test case for this? (Maybe you did, but I overlooked it)
There was a problem hiding this comment.
added in UseTest.nestedSvgWithoutViewBoxIsNotScaled
| void toleratesReferenceCycle() { | ||
| // Cyclic references are severed and render nothing instead of failing the document. | ||
| Assertions.assertDoesNotThrow(() -> tryLoad("parser/useCycle.svg")); | ||
| Assertions.assertDoesNotThrow(() -> tryLoad("parser/useCycleSelfReference.svg")); |
There was a problem hiding this comment.
Can we check that these elements are not painted e.g. by comparing to an empty svg?
| Optional<Float> contextFontSize(); | ||
|
|
There was a problem hiding this comment.
Let's deprecate this method instead of removing it.
There was a problem hiding this comment.
done; documented that the method is not used
| if (previousCodepoint != null && cursor.isCurrentGlyphAutoLayout()) { | ||
| cursor.applyKerning(font.kerningAdjustment(previousCodepoint, codepoint)); | ||
| } |
There was a problem hiding this comment.
As mentioned in another comment. Let's create a rendering hint for kerning and store it in the glyph cursor (as the Output is available when the cursor is created). Then check here for cursor.isKerningEnabled().
| // Batik doesn't apply kerning; the tolerance covers the resulting shift (measured 0.45% and 1.22%). | ||
| assertEquals(SUCCESS, compareImages("text/text1.svg", 0.7)); | ||
| assertEquals(SUCCESS, compareImages("text/text2.svg", 1.8)); |
There was a problem hiding this comment.
Render these without kerning enabled.
8853aac to
129d39b
Compare
…e they are now parseable
6bd9e30 to
33f5540
Compare
…on Apple systems. Background: font-family:.some-font-name is ignored in browsers since it's invalid. Previously JSVG supported that, now it doesn't any more due to compliant CSS parsing.
07b597b to
a048e88
Compare
done |
2 participants
It's based top of custom-font-support branch right now (the other PR).
Rushed a bit at the end (some bug fix commits that allowed me to enable more ReSvg test folders were mostly done with AI but I did review/change some things).
I'm on vacation until 25.08 but in general I'm very ready to do major changes/refactoring including coding style if you think they are needed.