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

Convert <use> tags. By default only defs with a single <use>. #670

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

Conversation

nikparo
Copy link

@nikparo nikparo commented Feb 26, 2017

I ran into exported svg's that use a lot of unnecessary defs, and decided to take matters into my own hands...

This plugin:

  • loops through the svg to first find all <defs> element definitions. It then finds all <use> items.
    (In hindsight, this should have been done in a single loop.)
  • Filters out the element definitions that are only used once (by default, but optional)
  • Converts <use> to <g> and inserts a clone of the definition.
  • Removes the definitions that have been inlined.

This plugin does not remove unused definitions or collapse the resulting groups, leaving that for removeUselessDefs and collapseGroups.

I believe this plugin would cover the request in #563

[edit: now removes the definitions that have been inlined]

@nikparo
Copy link
Author

nikparo commented Feb 26, 2017

May have created the request a bit too early, noticed that there are some problems with at least masks and clip paths.

[Edit: My test was faulty.]

@GreLI
Copy link
Member

GreLI commented Mar 11, 2017

There are several caveats in your code.

@alexjlockwood
Copy link
Contributor

alexjlockwood commented Mar 12, 2017

I don't think this PR handles CSS cascades correctly. According to the SVG spec, CSS cascade is applied to the referenced elements, not the (conceptually) cloned elements that make up the shadow DOM. What I think you would have to do is:

  1. Run some sort of inlineStyles plugin (i.e. Add inlineStyles plugin (rewrite of localStyles plugin) #592) on the SVG so that all of the elements that are descendants of a <defs> element are assigned their styled attributes.
  2. Then run this plugin (i.e. find the <use> tags and replace them with the deeply cloned elements).
  3. Make sure that any property-inheritance-related plugins are run after step (2).

Also I would take a look at the SVG spec on the <use> and <defs> tags since I also saw some things that were missing. For example, the plugin doesn't handle cases where x, y, width, and/or height attributes are specified on the <use> tag, etc.

Also this plugin doesn't handle the case where the <use> tag references another <use> tag, which apparently is possible.

@GreLI
Copy link
Member

GreLI commented Mar 12, 2017

Well, SVGO doesn't handle CSS almost at all.

@strarsis
Copy link
Contributor

strarsis commented Mar 24, 2017

A window.getComputedStyle() implementation would be useful here.
It is quite similar to inlineStyles plugin and style related API,
an extra lookup table or property would be required because it would be too slow otherwise.
This could be implemented as extra methods in css-tools when style support has been merged in.


if (item.isElem('use') && item.hasAttr('xlink:href')) {

var id = item.attr('xlink:href').value;
Copy link
Member

Choose a reason for hiding this comment

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

xlink can has any prefix not always xlink. Moreover, SVG 2 has allowed using href attribute without prefix at all.

// Make sure we don't call properties of undefined
var prevId, nextId;
try { prevId = itemAry[i-1].attr('xlink:href').value; }
catch (e) { prevId = null; }
Copy link
Member

Choose a reason for hiding this comment

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

You can just set

prevId = i > 0 ? itemAry[i - 1].attr('xlink:href').value : null;
nextId = i < itemAry.length - 1 ? itemAry[i + 1].attr('xlink:href').value : null;

without try-catch (they are preventing JIT-optimizing).

But I'd recommend using Set to check for uniqueness.

<rect id="rect2" x="40" y="40" width="140" height="120" rx="3" fill="green"/>
</g>
<g stroke="blue" fill="none" stroke-width="2">
<rect id="rect" x="0" y="0" width="200" height="200"/>
Copy link
Member

Choose a reason for hiding this comment

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

Here are two elements with the same id="rect" . Worth removing id?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants