feat: renderer/style: Passthrough unknown properties to Svg output

[cmumatt]

• Disallow [,],,^,` in first character of style identifiers (Fixes Invalid characters permitted in style identifier names #767)
• Removed some renderer code made unnecessary by passthrough SVG
• Made Rectangle rotatable, similar to the old shape, square
• Removed 'unknown property' test

Description

Fixes #767

Continued work on passthrough SVG properties (see PR# 749). This PR eliminates Style's check for unknown properties and removes the test associated with the unknown property check.

Further, SVG passthrough properties make some renderer code unnecessary. Where these properties are rendered via the passthrough logic, specific logic for these fields has been eliminated.

Implementation strategy and design decisions

This is a continuation of PR #749, which added support for passthrough properties to the renderer. This PR makes it possible for users to utilize this functionality in a Style program, except in the situation where the property name includes dashes. A future PR will address that issue since allowing dashes in an identifier name conflicts with the desire to allow computations in the form of x1-x2 (no spaces).

As before, there is presently no validation of passthrough SVG properties or their contents. This is left as future work and reflects the current group consensus. Further, there is no guarantee a passthrough property will not interfere with the optimized properties in the diagram. Some care and knowledge of SVG may be necessary on the part of the user.

Examples with steps to reproduce them

Choose any example. Add an unknown property such as "smileandwave" with a string literal value. Run the example and locate the passthrough property and value on the shape within the SVG output.

Checklist

• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• New and existing tests pass locally using yarn test
• I ran yarn docs and there were no errors when generating the HTML site
• My code follows the style guidelines of this project (e.g.: no ESLint warnings)

Open questions

See PR# 749

[codecov]

Codecov Report

Merging #759 (411ba0d) into main (4527bea) will increase coverage by 0.20%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #759      +/-   ##
==========================================
+ Coverage   65.72%   65.93%   +0.20%     
==========================================
  Files          59       59              
  Lines        7980     7920      -60     
  Branches     1405     1402       -3     
==========================================
- Hits         5245     5222      -23     
+ Misses       2726     2689      -37     
  Partials        9        9              

Impacted Files	Coverage Δ
packages/core/src/compiler/Style.ts	86.19% <ø> (-0.07%)	⬇️
packages/core/src/renderer/Circle.ts	100.00% <ø> (ø)
packages/core/src/renderer/Ellipse.ts	27.27% <ø> (+4.19%)	⬆️
packages/core/src/renderer/Polygon.ts	27.27% <ø> (+2.27%)	⬆️
packages/core/src/renderer/Polyline.ts	27.27% <ø> (+2.27%)	⬆️
packages/core/src/renderer/Text.ts	100.00% <ø> (ø)
packages/core/src/shapes/Rectangle.ts	100.00% <ø> (ø)
packages/core/src/engine/BBox.ts	86.59% <100.00%> (+0.07%)	⬆️
packages/core/src/renderer/AttrHelper.ts	89.55% <100.00%> (+11.06%)	⬆️
packages/core/src/renderer/Image.ts	81.48% <100.00%> (ø)
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4527bea...411ba0d. Read the comment docs.

[cloudflare-pages]

Deploying with    Cloudflare Pages

Latest commit:	411ba0d
Status:	✅  Deploy successful!
Preview URL:	https://83af3198.penrose-panes.pages.dev

View logs

[samestep]

Looks good but let's wait until #743 merges.

[maxkrieger]

Out of the loop: does the system warn the user when it's letting stuff pass through? Or, since the goal is to let everything pass through, it'd be too noisy?

as usual i'm just vaguely worried about xss somehow but idk

[cmumatt]

@maxkrieger, it's a good question. Nearly all diagrams will have passthrough properties. An example is r (radius), which is now a passthrough -- the typescript code previously mapping r is no longer necessary.

One of the tradeoffs of the approach we discussed earlier in the week is when a user mistypes a Style property name. There is no error or warning, which can lead to confusion. The consensus was to (eventually) build support in the IDE to help users recognize non-Style properties since we no longer stop compilation upon encountering "unknown" properties -- instead, we pass them down the pipeline.

[maxkrieger]

@cmumatt

Also are we aware about data- attributes, which are the proper way to do custom data?

Also some attr names have colons like xlink:href

[cmumatt]

The desire remains to broaden support for non-alphanumeric property names, but this PR does not implement a solution to this part of the problem. Options discussed include:

• Expanding the definition of style identifier names to include dashes. This breaks x1-x2 subtraction and does not accommodate all possibilities such as greek letters or namespace property qualifiers.
• Escaping with double quotes. Since this looks similar to a literal, its usage needs to be restricted to positions where literals do not occur: i.e., lvalues and the end of path statements.
• Escaping with brackets. This was less popular than double quotes, but might have fewer restrictions than escaping with double-quotes.

[samestep]

Looks great!