fix: collapseGroups does not move attributes atomically

[johnkenny54]

In the test case file, before this change, the transform attribute on the group was moved to the child, but the style containing the filter was not moved. This was changing the rendering.

regression.js before fix:

• Mismatched: 214
• Passed: 5308
• Total reduction 764239791 bytes

After fix:

• Mismatched: 201
• Passed: 5321
• Total reduction 764238752 bytes

This removes 13 regression mismatches. Compression of regression files is reduced by 1039 bytes.

Closes #1928

[KTibow]

I somewhat-strongly dislike this test case. It doesn't render in your browser, and it has too much cruft. Use something like this as the input instead.

Suggested change

	<svg xmlns="http:https://www.w3.org/2000/svg" xmlns:xlink="http:https://www.w3.org/1999/xlink">
	<defs>
	<linearGradient xlink:href="#a" id="c" x1="3.97" x2="3.97" y1=".348" y2="4.34" gradientUnits="userSpaceOnUse"/>
	</defs>
	<filter id="b">
	<feGaussianBlur stdDeviation="1"/>
	</filter>
	<symbol viewBox="-9.5 -7 19 14">
	<g style="filter:url(#b)">
	<linearGradient id="a" x1="3.97" x2="3.97" y1="2.425" y2="7.55" gradientUnits="userSpaceOnUse">
	<stop offset="1" style="stop-color:#fff"/>
	</linearGradient>
	</g>
	</symbol>
	<g transform="matrix(0 -1.2285 -1.2285 0 112.337 268.327)" style="filter:url(#b)">
	<path d="M.84.348H7.1V5.8H3.344z" style="fill:url(#c)"/>
	</g>
	</svg>
	<svg xmlns="http:https://www.w3.org/2000/svg" width="60" height="60">
	<defs>
	<filter id="a">
	<feGaussianBlur stdDeviation="1"/>
	</filter>
	</defs>
	<g transform="scale(2)" style="filter:url(#a)">
	<rect width="20" height="20" fill="blue" style=""/>
	</g>
	</svg>

[johnkenny54]

I'd like to keep it as is. The test file is a distillation of the Oxygen icon that was failing. It took me a really long time to isolate the problem, and I know for sure that the original file I submitted was failing before the change.

[KTibow]

Then I hope you will consider at least making it render in a browser and ideally distill it more

[johnkenny54]

Simplified the test case. New test case fails on main branch, passes here.

[KTibow]

The "incorrect" behavior renders the same as the "correct" behavior, but for the purposes of demonstrating the required behavior this test works fine.

[johnkenny54]

Yeah, I think it's pretty hard to build an example where the bug really makes a difference. That's one of the reasons it was so hard to track down. And also why the regression tests with "real" SVGs are important.

[SethFalco]

<svg xmlns="http:https://www.w3.org/2000/svg" viewBox="0 0 88 88">
  <filter id="a">
    <feGaussianBlur stdDeviation="1"/>
  </filter>
  <g transform="matrix(0.6,0,0,0.6,20,66)" style="filter:url(#a)">
    <path d="M33-83L-10-36L-10-32" style="fill-opacity:1"/>
  </g>
</svg>

Here is an example that reproduces the problem and clearly demonstrates the change in rendering caused by the plugin.

It looks like what you describe to be the issue and solution is correct and applies to this example. Could you use this for the test case though, please!

I wonder if we should merge styles in the case that the overlapping attribute is style. Would need to investigate which styles get priority, but we can tackle that later. 🤔

---

PS: The method I took to isolate the problem was:

1. Rename regression-fixtures to .regression-fixtures to ignore it.
2. Create a new regression-fixtures.
3. Copy over one of the failing tests, I took go-home.svg.
4. Delete half the SVG at a time until I find which node was causing the mismatch. (Running node test/regression between iterations.)
5. Strip down the SVG until I've made it as small as I can while still getting a mismatch.
6. Review in a browser that the rendering was actually impacting, i.e. not just antialiasing.

Later, let's make an argument we can pass to test/regression.js to only test a specific file. Then it'll be easier to do this kind of thing to debug a specific mismatch.

[johnkenny54]

I changed the test file.

I did pretty much what you did to isolate this, but this one was difficult to isolate because the file didn't fail if you only ran collapseGroups - you also had to run some other combination of plugins that would reorder the attributes in a way that would cause collapseGroups to trigger the error, and deleting parts of the original file sometimes changed things in such a way that the error didn't show up, so there were a lot of red herrings.

Also, see PR #1932 - this is a start on changing regression.js to make testing/analysis easier. I've been using this version to analyze problems and test changes. Still more to be done obviously, but it's a start.