[Icon] Use img tag instead of inlining svgs #926

theodoretan · 2019-01-23T17:12:23Z

WHY are these changes introduced?

Part of #885

Inlining svgs is potentially dangerous as scripts can be executed if they are in the svg.

WHAT is this pull request doing?

If the user provides an svg, we will display it in an img tag instead. Since we can't use fill on img tag we need to use a filter from the polaris-tokens to get the correct colour that we want.

This is adding a untrusted prop to Icon that a user will have to manually set in order to render a svg string. This is temporary as we wait for the work on SVGR to be completed.

`white`	`black`

`skyLighter`	`skyLight`	`sky`	`skyDark`

`inkLightest`	`inkLighter`	`inkLight`	`ink`

`blueLighter`	`blueLight`	`blue`	`blueDark`	`blueDarker`

`indigoLighter`	`indigoLight`	`indigo`	`indigoDark`	`indigoDarker`

`tealLighter`	`tealLight`	`teal`	`tealDark`	`tealDarker`

`greenLighter`	`green`	`greenDark`

`yellowLighter`	`yellow`	`yellowDark`

`orange`	`orangeDark`

`redLighter`	`red`	`redDark`

`purple`

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import * as React from 'react';
import {Page, Icon} from '@shopify/polaris';

interface State {}

const svg = `<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px" viewBox="0 0 438.549 438.549" style="enable-background:new 0 0 438.549 438.549;"
xml:space="preserve">
<g><path d="M409.132,114.573c-19.608-33.596-46.205-60.194-79.798-79.8C295.736,15.166,259.057,5.365,219.271,5.365
 c-39.781,0-76.472,9.804-110.063,29.408c-33.596,19.605-60.192,46.204-79.8,79.8C9.803,148.168,0,184.854,0,224.63
 c0,47.78,13.94,90.745,41.827,128.906c27.884,38.164,63.906,64.572,108.063,79.227c5.14,0.954,8.945,0.283,11.419-1.996
 c2.475-2.282,3.711-5.14,3.711-8.562c0-0.571-0.049-5.708-0.144-15.417c-0.098-9.709-0.144-18.179-0.144-25.406l-6.567,1.136
 c-4.187,0.767-9.469,1.092-15.846,1c-6.374-0.089-12.991-0.757-19.842-1.999c-6.854-1.231-13.229-4.086-19.13-8.559
 c-5.898-4.473-10.085-10.328-12.56-17.556l-2.855-6.57c-1.903-4.374-4.899-9.233-8.992-14.559
 c-4.093-5.331-8.232-8.945-12.419-10.848l-1.999-1.431c-1.332-0.951-2.568-2.098-3.711-3.429c-1.142-1.331-1.997-2.663-2.568-3.997
 c-0.572-1.335-0.098-2.43,1.427-3.289c1.525-0.859,4.281-1.276,8.28-1.276l5.708,0.853c3.807,0.763,8.516,3.042,14.133,6.851
 c5.614,3.806,10.229,8.754,13.846,14.842c4.38,7.806,9.657,13.754,15.846,17.847c6.184,4.093,12.419,6.136,18.699,6.136
 c6.28,0,11.704-0.476,16.274-1.423c4.565-0.952,8.848-2.383,12.847-4.285c1.713-12.758,6.377-22.559,13.988-29.41
 c-10.848-1.14-20.601-2.857-29.264-5.14c-8.658-2.286-17.605-5.996-26.835-11.14c-9.235-5.137-16.896-11.516-22.985-19.126
 c-6.09-7.614-11.088-17.61-14.987-29.979c-3.901-12.374-5.852-26.648-5.852-42.826c0-23.035,7.52-42.637,22.557-58.817
 c-7.044-17.318-6.379-36.732,1.997-58.24c5.52-1.715,13.706-0.428,24.554,3.853c10.85,4.283,18.794,7.952,23.84,10.994
 c5.046,3.041,9.089,5.618,12.135,7.708c17.705-4.947,35.976-7.421,54.818-7.421s37.117,2.474,54.823,7.421l10.849-6.849
 c7.419-4.57,16.18-8.758,26.262-12.565c10.088-3.805,17.802-4.853,23.134-3.138c8.562,21.509,9.325,40.922,2.279,58.24
 c15.036,16.18,22.559,35.787,22.559,58.817c0,16.178-1.958,30.497-5.853,42.966c-3.9,12.471-8.941,22.457-15.125,29.979
 c-6.191,7.521-13.901,13.85-23.131,18.986c-9.232,5.14-18.182,8.85-26.84,11.136c-8.662,2.286-18.415,4.004-29.263,5.146
 c9.894,8.562,14.842,22.077,14.842,40.539v60.237c0,3.422,1.19,6.279,3.572,8.562c2.379,2.279,6.136,2.95,11.276,1.995
 c44.163-14.653,80.185-41.062,108.068-79.226c27.88-38.161,41.825-81.126,41.825-128.906
 C438.536,184.851,428.728,148.168,409.132,114.573z"/>
</g></svg>`;

export default class Playground extends React.Component<never, State> {
  render() {
    return (
      <Page title="Playground">
        <Icon source={svg} untrusted color="indigo" />
        <br />
        <Icon source="home" color="indigo" />
      </Page>
    );
  }
}

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility

cc @ragalie

CautionTapeBot · 2019-01-23T17:12:24Z

We noticed that this PR either modifies or introduces usage of the dangerouslySetInnerHTML attribute, which can cause cross-site scripting (XSS) vulnerabilities. Our team will take a look soon, but for now we recommend reviewing your code to ensure that this is what you intended to use and that there is not a safe alternative available. Docs are available here.

ghost · 2019-01-23T17:12:26Z

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

amrocha

Looks good, a few questions and suggestions

amrocha · 2019-02-15T19:57:50Z

src/components/Icon/Icon.tsx

@@ -188,6 +193,7 @@ function Icon({
  color,
  backdrop,
  accessibilityLabel,
+  untrusted,


What do you think of adding a default value of false here? I think it would reduce ambiguity and make typing a bit easier

amrocha · 2019-02-15T19:59:56Z

src/components/Icon/Icon.tsx

-          aria-hidden="true"
-        />
-      );
+  } else if (isBundledIcon(source)) {


nice change 👍

amrocha · 2019-02-15T20:03:45Z

src/components/Icon/Icon.tsx

+  );
+}
+
+function isUntrustedSVG(source: IconSource): source is UntrustedSVG {


What do you think of moving this check inline instead of in a function? It's a really simple check, so I don't want to inspire undue confidence in it

I think keeping it a separate function is okay because it's clear what the conditional is doing through the function name. If we use typeof source === 'string' above, it's not as clear what we are testing. The function also ensures source is UntrustedSVG though type guarding.

amrocha · 2019-02-15T20:06:07Z

src/styles/foundation/filters.scss

+/// base.
+/// @return {List} The filter list.
+
+@function filter($hue, $value: base) {


Did you write this or did you copy it from somewhere else?

I copied the color function and modified it to use the filter map instead.

amrocha

Add an entry to UNRELEASED.md and this is good to go imo

amrocha

Oh actually I forgot one thing, can you add a simple example under the Examples section in the README.md file? That will make sure that this gets tested as part of our visual regression testing so we don't break it accidentally, and document usage

theodoretan · 2019-02-15T20:50:24Z

@amrocha Are you talking about this readme? https://github.com/Shopify/polaris-react/blob/master/src/components/Icon/README.md

BPScott · 2019-02-15T20:51:37Z

src/components/Icon/Icon.tsx

+    contentMarkup = (
+      <img
+        className={styles.Img}
+        src={`data:image/svg+xml;base64,${btoa(source)}`}


I think if you omit the base64, then you can output your source as-is without running it through btoa()

`data:image/svg+xml;${source}`

amrocha · 2019-02-15T20:52:55Z

@theodoretan yep that's the one

ghost · 2019-02-15T21:44:43Z

🎉 Thanks for your contribution to Polaris React!

theodoretan self-assigned this Jan 23, 2019

BPScott temporarily deployed to polaris-react-pr-926 January 23, 2019 17:12 Inactive

BPScott temporarily deployed to polaris-react-pr-926 February 7, 2019 16:43 Inactive

theodoretan force-pushed the update_icon_component branch from 966cf15 to 52690a7 Compare February 7, 2019 20:04

BPScott temporarily deployed to polaris-react-pr-926 February 7, 2019 20:04 Inactive

BPScott temporarily deployed to polaris-react-pr-926 February 7, 2019 20:52 Inactive

BPScott temporarily deployed to polaris-react-pr-926 February 7, 2019 22:23 Inactive

theodoretan force-pushed the update_icon_component branch from b37bea2 to a16559e Compare February 8, 2019 14:19

BPScott temporarily deployed to polaris-react-pr-926 February 8, 2019 14:19 Inactive

theodoretan changed the title ~~[Icon][WIP] Use img tag instead of inlining svgs~~ [Icon] Use img tag instead of inlining svgs Feb 8, 2019

theodoretan requested review from kaelig and tmlayton February 8, 2019 14:21

shopify-admins requested review from a team February 8, 2019 14:21

theodoretan requested a review from amrocha February 11, 2019 14:53

amrocha reviewed Feb 15, 2019

View reviewed changes

amrocha approved these changes Feb 15, 2019

View reviewed changes

amrocha suggested changes Feb 15, 2019

View reviewed changes

BPScott reviewed Feb 15, 2019

View reviewed changes

theodoretan force-pushed the update_icon_component branch from a16559e to d6faf7b Compare February 15, 2019 21:18

BPScott temporarily deployed to polaris-react-pr-926 February 15, 2019 21:18 Inactive

Add untrusted prop to render svg strings in an img tag

9f92b42

theodoretan force-pushed the update_icon_component branch from d6faf7b to 9f92b42 Compare February 15, 2019 21:35

BPScott deployed to polaris-react-pr-926 February 15, 2019 21:35 View deployment

amrocha approved these changes Feb 15, 2019

View reviewed changes

theodoretan merged commit 28e4c43 into master Feb 15, 2019

theodoretan deleted the update_icon_component branch February 15, 2019 21:44

alex-page temporarily deployed to production February 20, 2019 15:24 Inactive

alex-page temporarily deployed to production February 20, 2019 15:31 Inactive

alex-page temporarily deployed to production February 20, 2019 15:40 Inactive

alex-page temporarily deployed to production February 20, 2019 15:45 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Icon] Use img tag instead of inlining svgs #926

[Icon] Use img tag instead of inlining svgs #926

theodoretan commented Jan 23, 2019 •

edited

Loading

CautionTapeBot commented Jan 23, 2019

ghost commented Jan 23, 2019

amrocha left a comment

amrocha Feb 15, 2019

amrocha Feb 15, 2019

amrocha Feb 15, 2019

theodoretan Feb 15, 2019

amrocha Feb 15, 2019

theodoretan Feb 15, 2019

amrocha left a comment

amrocha left a comment

theodoretan commented Feb 15, 2019

BPScott Feb 15, 2019

amrocha commented Feb 15, 2019

ghost commented Feb 15, 2019

[Icon] Use img tag instead of inlining svgs #926

[Icon] Use img tag instead of inlining svgs #926

Conversation

theodoretan commented Jan 23, 2019 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

CautionTapeBot commented Jan 23, 2019

ghost commented Jan 23, 2019

amrocha left a comment

Choose a reason for hiding this comment

amrocha Feb 15, 2019

Choose a reason for hiding this comment

amrocha Feb 15, 2019

Choose a reason for hiding this comment

amrocha Feb 15, 2019

Choose a reason for hiding this comment

theodoretan Feb 15, 2019

Choose a reason for hiding this comment

amrocha Feb 15, 2019

Choose a reason for hiding this comment

theodoretan Feb 15, 2019

Choose a reason for hiding this comment

amrocha left a comment

Choose a reason for hiding this comment

amrocha left a comment

Choose a reason for hiding this comment

theodoretan commented Feb 15, 2019

BPScott Feb 15, 2019

Choose a reason for hiding this comment

amrocha commented Feb 15, 2019

ghost commented Feb 15, 2019

theodoretan commented Jan 23, 2019 •

edited

Loading