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

Add chainFilters plugin #733

Closed
wants to merge 17 commits into from
Closed

Add chainFilters plugin #733

wants to merge 17 commits into from

Conversation

strarsis
Copy link
Contributor

@strarsis strarsis commented Jun 2, 2017

This PR adds the chainFilters plugin which chains filter elements using CSS filters as
workaround for some issues with specific browsers, closes #732.
Furthermore a DOM walker is added which can be used for traversing the svgo DOM (by full mode plugins).
Currently some plugins and svgo core all use their own custom "monkeys" implementations for this.

If a filter is already assigned to an element using styles and the plugin also got a filter attribute to the same filter element, this plugin would still create a chain - this may be a feature to add - but this kind of corner case should be very rare, also the element shouldn't have a filter assigned by filter attribute and styles in the first place. Also this plugin currently always assumes relative paths in fragment identifiers.

@strarsis strarsis changed the title Add chainFilter plugin Add chainFilters plugin Jun 2, 2017
@strarsis
Copy link
Contributor Author

strarsis commented Jun 2, 2017

@drschwabe: You can already test this plugin using:

# using npm:
$ npm install github:strarsis/svgo#chain-filters

# using yarn
$ yarn add github:strarsis/svgo#chain-filters

@strarsis
Copy link
Contributor Author

strarsis commented Jun 3, 2017

@GreLI: The filter bug is indeed a problem in Firefox - maybe this plugin should be enabled by default?
Currently the filter element is chained with the element using a CSS class with filter property via IRI.
Would it be better to convert the whole filter element into CSS? - Would this be always possible?

@strarsis
Copy link
Contributor Author

strarsis commented Nov 8, 2017

@GreLI: What can I do for further speeding up the review/merge process?

@GreLI
Copy link
Member

GreLI commented Nov 8, 2017

I haven't made a decision yet. Not sure about the plugin. Need some time to consider.

@strarsis
Copy link
Contributor Author

strarsis commented Nov 19, 2017

@GreLI, @caub:
Arguments against merging this plugin into svgo:

  • It doesn't minify or simplify, rather it is a polyfill for a Firefox issue.
  • (When a plugin-as-package architecture has been implemented in svgo) This could also fit in a separate package.
  • I am biased because I made the plugin.

Arguments for merging this plugin into svgo:

  • This Firefox bug can be subtle and puzzling when suddenly some parts of the SVG are missing when viewed in Firefox. Just having this extra step will ensure this problem doesn't happen again for all SVGs processed by svgo.
  • One doesn't necessarily finds this bug when searching for it (I encountered this bug in a svgo issue created by another user and found the exact same symptoms).
  • This plugin can always be disabled or even removed when Firefox has this bug finally fixed.

@strarsis
Copy link
Contributor Author

@TrySound: Does it make sense to add this plugin to svgo? Loading plugins from other packages (installed from npm) isn't supported yet by svgo.

@TrySound
Copy link
Member

@strarsis It is supported. Just require the plugin and pass to plugins array.

@strarsis
Copy link
Contributor Author

@TrySound: So should this plugin now be merged into svgo repository or rather provided as standalone plugin (e.g. via npm)?

@TrySound
Copy link
Member

This looks more like polyfill than optimisation. So yes, this would probably look better as separate package.


var walk = require('tree-walk');

var domWalker = walk(function(node) {
Copy link
Member

Choose a reason for hiding this comment

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

I added traverse utility in lib/xast.js.

@TrySound
Copy link
Member

TrySound commented Dec 7, 2021

See

@strarsis
Copy link
Contributor Author

@TrySound: Thanks for the adjustment. I had not looked at this PR for some while.

When I try this test SVG in Chrome and Firefox (both, regular and Developer Edition), the SVG is displayed correctly and equally:
https://gist.githubusercontent.com/strarsis/2c769b63b9573b9cf81faa9242adf4ec/raw/1571560b8ca1c895a52fb3669a4ad32615350993/chainfilters-test.svg
So does this mean that this issue had been eventually fixed in Firefox?

@SethFalco
Copy link
Member

Hey! Thanks for creating the PR.

So does this mean that this issue had been eventually fixed in Firefox?

Indeed, it looks like the issue has already been resolved. Makes sense given the amount of time that's passed. ^-^'

@SethFalco SethFalco closed this Dec 9, 2023
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.

Plugin idea/request for filter cross browser compatibility
4 participants