Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(applyTransformsShapes): new plugin #1854

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

KTibow
Copy link
Contributor

@KTibow KTibow commented Nov 26, 2023

Right now transforms can only be applied to paths. This plugin applies them to circle, ellipse, rect, and image. These transforms often come from graphical editors.
Has 99% line test coverage
Closes #989
Closes #594

Results

While this plugin doesn't help with any of the common test cases, it does help with some other real situations.

  • https://accelerator.github.com/images/invertocat.svg
    -82b compared to current SVGO
  • Lawnicons, a collection of SVG icons
    While I didn't do a full size comparison, I tested it on a number of SVGs, and it successfully applied the transformations. For example, this SVG
    <svg xmlns="http://www.w3.org/2000/svg" width="192" height="192" fill="none" stroke="#000" stroke-linecap="round" stroke-linejoin="round" stroke-width="12"><circle cx="5" cy="5" r="5" stroke-width="10" transform="matrix(1 0 0 -1 65 81)"/><circle cx="96" cy="96" r="74"/><path d="M133 77a13.998 13.998 0 0 0-28 0m-34 46s8 15 24 15 24-15 24-15m14-46c0 11-14 24-14 24s-14-14-14-24"/></svg>
    was converted to
    <svg xmlns="http://www.w3.org/2000/svg" width="192" height="192" fill="none" stroke="#000" stroke-linecap="round" stroke-linejoin="round" stroke-width="12"><circle cx="70" cy="76" r="5" stroke-width="10"/><circle cx="96" cy="96" r="74"/><path d="M133 77a13.998 13.998 0 0 0-28 0m-34 46s8 15 24 15 24-15 24-15m14-46c0 11-14 24-14 24s-14-14-14-24"/></svg>
    (-33b)

@KTibow KTibow marked this pull request as draft November 26, 2023 23:51
@KTibow KTibow marked this pull request as ready for review November 27, 2023 02:12
@KTibow KTibow marked this pull request as draft January 4, 2024 04:01
@KTibow KTibow marked this pull request as ready for review January 6, 2024 15:40
@Airkro
Copy link

Airkro commented Jan 22, 2024

Work fine for me, but why APPLICABLE_SHAPES not includes text ?

@KTibow
Copy link
Contributor Author

KTibow commented Jan 22, 2024

I haven't considered applying transforms to text. Would you mind providing a couple examples of places where I can apply the transform and a couple places where I can't?

@Airkro
Copy link

Airkro commented Jan 23, 2024

<svg xmlns="http://www.w3.org/2000/svg">

  <text y="7" transform="translate(1270 167)">abcdefg</text>

  <!-- ↓ could be -->

  <text x="1270" y="174">abcdefg</text>

</svg>

@KTibow
Copy link
Contributor Author

KTibow commented Jan 23, 2024

I know that there's a <tspan>. Will I still not have to worry about it as long as I stick to the outer <text>?

@Airkro
Copy link

Airkro commented Jan 23, 2024

I don't know the details of <tspan>, I have some real-world test cases where files do not include <tspan>, they are working without bugs, so maybe there's no risk.

Maybe adding options.allowSelector could ensure users know what they doing. Then you don't have to worry about that?

@johnkenny54
Copy link
Contributor

I know that there's a <tspan>. Will I still not have to worry about it as long as I stick to the outer <text>?

This comes up a lot in Inkscape - it includes a bunch of redundant and seemingly useless attributes/elements. If you create 2 text elements and resize the canvas, you get something like:

<svg xmlns="http://www.w3.org/2000/svg" width="32.496" height="51.922">
    <text xml:space="preserve" x="50" y="70" style="font-size:8px;font-family:Arial;fill:none;stroke:#000" transform="translate(-49.688 -63.672)">
        <tspan x="50" y="70">Text One</tspan>
    </text>
    <text xml:space="preserve" x="50" y="115" style="font-size:8px;font-family:Arial;fill:none;stroke:#000" transform="translate(-49.688 -63.672)">
        <tspan x="50" y="115">Text Two</tspan>
    </text>
</svg>

The x/y on the text don't seem to do anything, it's only the tspan coordinates that matter.

@Airkro
Copy link

Airkro commented Jan 31, 2024

It looks complicated, Maybe you guys can merge this PR first, leave The / problem to next round ?

@AnweshGangula
Copy link

AnweshGangula commented Feb 7, 2024

is this feature implemented? I see that the svg transformations get flattened when I try to use SVGR, but i'm not sure if this is is a SVGR issue or implementation from SVGO

I noticed this file in the repo, is this the plugin?
https://github.com/svg/svgo/blob/main/plugins/applyTransforms.js

@KTibow
Copy link
Contributor Author

KTibow commented Feb 7, 2024

SVGO already flattens transforms for paths. This PR adds a new plugin that does it for shapes, like circles or images. SVGO won't have it until this is merged.

@KTibow
Copy link
Contributor Author

KTibow commented Feb 25, 2024

@SethFalco any blockers on this?

@SethFalco
Copy link
Member

SethFalco commented Feb 25, 2024

No blockers, I was just prioritizing issues that fix regression tests. Sorry to keep you waiting though, in fact, I'll review this tomorrow evening soon for you.

Copy link
Member

@SethFalco SethFalco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please rebase and review the failed tests from KDE Oxygen Icons?

This breaks quite a lot of the icons, so before giving it a proper review, it'd be worth resolving those locally.

I've only manually reviewed/confirmed that battery-charging-080.svg broke, but overall it introduced 600+ failures.

plugins/applyTransformsShapes.js Outdated Show resolved Hide resolved
plugins/applyTransformsShapes.js Show resolved Hide resolved
plugins/applyTransformsShapes.js Outdated Show resolved Hide resolved
@KTibow
Copy link
Contributor Author

KTibow commented Mar 1, 2024

So looks like the mismatches are because gradients can get messed up by changing the orientation. This would be solved by checking for gradients, but I don't want to duplicate code, and now I'm seeing that we have many places where we might want to check for references in SVGO:

  • Do any of this group's attributes that can reference something reference a URL? (moveGroupAttrsToElems, applyTransforms, future applyTransformsShapes)
  • In this element's computed styles, do we use anything that is sensitive to bounding boxes, namely gradient fills, filters, or strokes? (mergePaths, future removing trailing M)

I would normally want to simply extract it all out to a function, but the behavior is different for different cases (in fact, we don't run applyTransforms for anything with style right now). And arguably, we should update the existing cases so that they can cope with style by using computedStyles instead of just attributes. What would you suggest here?

@SethFalco
Copy link
Member

check for references

We do already have the #findReferences which can be used for checking for href and url() references. (Check if the resulting array is empty.)

There's also #includesUrlReference to check the value of a specific attribute.

If we don't need to check for all references props, happy for us to pass a third parameter with an array of referencesProps that defaults to the one defined in _collections.js.

but the behavior is different for different cases

Just checking, when you note that the behavior is different for different cases, are you referring to in theory we need to handle this differently in certain plugins, or just that how we approach this now is inconsistent?

Immediately, as you say, it'd be good to extract some of the logic (or at least the common parts of it) into a function like isBoundingBoxSensitive that accepts the node and computed styles.

And arguably, we should update the existing cases so that they can cope with style by using computedStyles instead of just attributes.

Agreed, but best for a separate issue/PR. Once this is finalized, we can refactor applyTransforms in general to use the new helper methods too.

@KTibow
Copy link
Contributor Author

KTibow commented Mar 1, 2024

Just checking, when you note that the behavior is different for different cases, are you referring to in theory we need to handle this differently in certain plugins, or just that how we approach this now is inconsistent?

After a bit of thinking, I've concluded that these should be the same for the most part.

If we were to have a basis, it would be mergePaths. The way we currently do it in applyTransforms is oversensitive (color-profile doesn't affect things) and doesn't check all the cases (doesn't notice <style>, doesn't notice inline clip-path).

Here's how we handle it now in mergePaths:

            computedStyle['marker-start'] ||
            computedStyle['marker-mid'] ||
            computedStyle['marker-end'] ||
            computedStyle['clip-path'] ||
            computedStyle['mask'] ||
            computedStyle['mask-image'] ||
            ['fill', 'filter', 'stroke'].some((attName) =>
              elementHasUrl(computedStyle, attName),
            )

I believe in almost all of the cases where this is tripped, applyTransforms shouldn't run, and if it isn't tripped a path is likely safe. The only possible change we might make when using it in applyTransforms would be to allow markers as long as there is no scaling, but it's arguable that the added complexity isn't worth it when markers are rare and this combination would be even rarer.

@KTibow
Copy link
Contributor Author

KTibow commented Mar 1, 2024

But more deliberation can be saved for later. Which way would you recommend moving forward with this PR?

  • Make the function extracted from mergePaths, use it here, and apply more refactoring in a separate PR
  • Make the function extracted from mergePaths, use it here and in mergePaths, and apply more refactoring in a separate PR
  • Duplicate the applyTransforms logic and apply more refactoring in a separate PR
  • Duplicate the mergePaths logic and apply more refactoring in a separate PR

@SethFalco
Copy link
Member

For the most part, just whatever you're comfortable with, tbh. 👍🏽

In terms of reviewing, I think the easiest would be:

Make the function extracted from mergePaths, use it here and in mergePaths, and apply more refactoring in a separate PR

Makes sense to refactor mergePaths as part of this PR since the helper function is ultimately pulling out code from there. Then any changes/refactors to applyTransforms itself can be handled in another PR.

@KTibow KTibow requested a review from SethFalco March 3, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply translate transforms to shapes too Apply transforms to an element's coordinates
5 participants