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

fix(PPDSC-2605): safari svg filters fix #487

Merged
merged 23 commits into from
Nov 30, 2022

Conversation

evgenitsn
Copy link
Contributor

PPDSC-2605

What

This fixes the issue with Safari where some of the SVG are not rendered correctly.
The issue is that Safari does not support additional filters like shadows, blurs and more if the svg is external i.e.imported using <use href={...} />.

The workaround for this is to create a new empty <svg> and add the <defs> section of the svgs (the part where the filters are located) and add this svg anywhere in the DOM.

What I did is to check if the browser is Safari, and if it is to read the svg as raw string, sanitize it, get substring of that <defs> .. </defs> and append it as empty, hidden svg next to the original svg.

If the browser is not Safari, this logic is skipped and nothing changes.
Looks like this approach fixes the Safari issues with the illustrations.

Example of the DOM for Safari vs Chrome

Safari:
image

Chrome:
image

I have done:

  • Written unit tests against changes
  • Written functional tests against the component and/or NewsKit site
  • Updated relevant documentation

I have tested manually:

  • The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
  • The screen reader reads and flows through the elements as expected.
  • There are no new errors in the browser console coming from this PR.
  • When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
  • The Playground feature is working as expected

Before:

image

After:
image

Who should review this PR:

How to test:

@evgenitsn evgenitsn added fix This change fixes a bug ready for review Please assist in getting this reviewed labels Nov 23, 2022
@evgenitsn evgenitsn added WIP Work in progress and removed ready for review Please assist in getting this reviewed labels Nov 23, 2022
@evgenitsn evgenitsn added ready for review Please assist in getting this reviewed and removed WIP Work in progress labels Nov 23, 2022
package.json Show resolved Hide resolved
*/
const loadSVGFiltersForSafari = (path: string) => {
if (!isSafari) return null;
const {sanitize} = dompurify;
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if we can load the dompurify with dynamic import so that our bundle size does not increase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case we can, but we are already using the dompurify in another Component: site/components/tools/svg-previewer.tsx. So I think the bundle size is already including that.

I tried to import it dynamically there as well but I wasn't able to figure out how to dynamically import an npm package in a react component body. If you have any ideas, happy to discuss.

Copy link
Contributor

Choose a reason for hiding this comment

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

there is a difference I believe, there is a bundle per page, so the previewer will include it only there, and since we start using it for SVG's and they are on all pages it will start to be included everywhere.
I think you might need to do that inside useEffect and maybe create a custom hook in order to achieve it. I am not sure if its with the time these extra 10kb but have a try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code, the sanitizer should be load dynamically only for Safari now.

@mstuartf
Copy link
Contributor

Screenshot 2022-11-25 at 10 39 18

Screenshot 2022-11-25 at 10 39 22

@evgenitsn fyi it looks like this is being picked up by the Percy tests, so you will have some to resolve.

@evgenitsn
Copy link
Contributor Author

Screenshot 2022-11-25 at 10 39 18 Screenshot 2022-11-25 at 10 39 22

@evgenitsn fyi it looks like this is being picked up by the Percy tests, so you will have some to resolve.

Thanks Mike, yes I noticed this in a few more cases. I will push an update. Marking it as WIP now as the implementation would change a bit.

@evgenitsn evgenitsn added WIP Work in progress and removed ready for review Please assist in getting this reviewed labels Nov 25, 2022
@evgenitsn
Copy link
Contributor Author

evgenitsn commented Nov 25, 2022

@mutebg @mstuartf I've updated the implementation, it turned out that appending only the section is not reliable solution because not all filters are contained inside it.

So currently for Safari we are just inlining the SVG into the code (same as before the svg/tsx change) and for the rest of the browsers we are using external SVGs inside the i.e. how it is supposed to work.

@evgenitsn
Copy link
Contributor Author

@mstuartf @mutebg @LukeFinch Thank for looking into this. I think it's in a good shape now, ready for reviews.

LukeFinch
LukeFinch previously approved these changes Nov 28, 2022
mstuartf
mstuartf previously approved these changes Nov 29, 2022
LukeFinch
LukeFinch previously approved these changes Nov 29, 2022
@evgenitsn evgenitsn dismissed stale reviews from LukeFinch and mstuartf via 612b7c3 November 29, 2022 09:45
LukeFinch
LukeFinch previously approved these changes Nov 29, 2022
@evgenitsn evgenitsn merged commit 63269e1 into main Nov 30, 2022
@evgenitsn evgenitsn deleted the fix/PPDSC-2605-svg-shadow-elements-missing-in-safari branch November 30, 2022 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix This change fixes a bug ready for review Please assist in getting this reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants