From 724bbd2f3f3388feb4ad04162d3e32f5a0853e05 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Wed, 22 Sep 2021 12:35:24 -0700 Subject: [PATCH 01/11] Remove unnecessary ID from stopSlash icon --- src/components/icon/__snapshots__/icon.test.tsx.snap | 1 - src/components/icon/assets/stop_slash.js | 5 +---- src/components/icon/assets/stop_slash.svg | 2 +- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/components/icon/__snapshots__/icon.test.tsx.snap b/src/components/icon/__snapshots__/icon.test.tsx.snap index da324d45aee..e6be04a6279 100644 --- a/src/components/icon/__snapshots__/icon.test.tsx.snap +++ b/src/components/icon/__snapshots__/icon.test.tsx.snap @@ -7867,7 +7867,6 @@ exports[`EuiIcon props type stopSlash is rendered 1`] = ` > `; diff --git a/src/components/icon/assets/stop_slash.js b/src/components/icon/assets/stop_slash.js index 13d8e89d65e..4352ef68c57 100644 --- a/src/components/icon/assets/stop_slash.js +++ b/src/components/icon/assets/stop_slash.js @@ -12,10 +12,7 @@ const EuiIconStopSlash = ({ title, titleId, ...props }) => ( {...props} > {title ? {title} : null} - + ); diff --git a/src/components/icon/assets/stop_slash.svg b/src/components/icon/assets/stop_slash.svg index 31d24729f97..7ef4b0cf4c7 100644 --- a/src/components/icon/assets/stop_slash.svg +++ b/src/components/icon/assets/stop_slash.svg @@ -1,3 +1,3 @@ - + From 8ce769b19d7448d3cb6bccadd3f7d9da42a9c48e Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Wed, 22 Sep 2021 12:41:28 -0700 Subject: [PATCH 02/11] [scripts setup] Add check for static SVG IDs - DRY out toString() to one var - switch from indexOf to the slightly more readable `.includes()` API --- scripts/compile-icons.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/compile-icons.js b/scripts/compile-icons.js index a42ef619eb9..22cc2f8fa71 100644 --- a/scripts/compile-icons.js +++ b/scripts/compile-icons.js @@ -17,13 +17,15 @@ const iconFiles = glob.sync('**/*.svg', { cwd: iconsDir, realpath: true }); iconFiles.forEach(async (filePath) => { const svgSource = fs.readFileSync(filePath); + const svgString = svgSource.toString(); try { - const viewBoxPosition = svgSource.toString().indexOf('viewBox'); - if (viewBoxPosition === -1) { + if (!svgString.includes('viewBox')) { throw new Error(`${filePath} is missing a 'viewBox' attribute`); } + const hasIds = svgString.includes('id="'); + const jsxSource = await svgr( svgSource, { From b6745e4609e42469c26d0d0f11a56cf2bf14b0a1 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Wed, 22 Sep 2021 12:45:28 -0700 Subject: [PATCH 03/11] Set up htmlIdGenerator util for SVGs with IDs - Use different template with htmlIdGenerator import/setup + non-implicit return - tweak template literal indenting - DRY out fileName for use as generator prefix --- scripts/compile-icons.js | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/scripts/compile-icons.js b/scripts/compile-icons.js index 22cc2f8fa71..3ff84015688 100644 --- a/scripts/compile-icons.js +++ b/scripts/compile-icons.js @@ -16,6 +16,7 @@ function pascalCase(x) { const iconFiles = glob.sync('**/*.svg', { cwd: iconsDir, realpath: true }); iconFiles.forEach(async (filePath) => { + const fileName = path.basename(filePath, '.svg'); const svgSource = fs.readFileSync(filePath); const svgString = svgSource.toString(); @@ -45,14 +46,27 @@ iconFiles.forEach(async (filePath) => { { template }, opts, { imports, componentName, props, jsx } - ) => template.ast` + ) => + hasIds + ? template.ast` +${imports} +import { htmlIdGenerator } from '../../../services'; +const ${componentName} = (${props}) => { + const generateId = htmlIdGenerator('${fileName}'); + return ( + ${jsx} + ); +}; +export const icon = ${componentName}; +` + : template.ast` ${imports} const ${componentName} = (${props}) => ${jsx} export const icon = ${componentName}; - `, +`, }, { - componentName: `EuiIcon${pascalCase(path.basename(filePath, '.svg'))}`, + componentName: `EuiIcon${pascalCase(fileName)}`, } ); From fb538a02bc816be7bcd18e0395f684850e4c3a23 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Wed, 22 Sep 2021 12:50:55 -0700 Subject: [PATCH 04/11] Replace the static SVG id attributes with dynamic JSX NOTE: I'm avoiding doing this in the `ast` process for two main reasons: 1. I'm a monster who think regexes are neat 2. the ${jsx} in the ast template is an actual tree of components and it seems much more tedious to iterate through each one & examine its attributes (of which the url() attr can have multiple - e.g. fill or mask), rather than use a basic regex capture group --- scripts/compile-icons.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/scripts/compile-icons.js b/scripts/compile-icons.js index 3ff84015688..ae35474715b 100644 --- a/scripts/compile-icons.js +++ b/scripts/compile-icons.js @@ -27,7 +27,7 @@ iconFiles.forEach(async (filePath) => { const hasIds = svgString.includes('id="'); - const jsxSource = await svgr( + let jsxSource = await svgr( svgSource, { plugins: ['@svgr/plugin-svgo', '@svgr/plugin-jsx'], @@ -70,6 +70,14 @@ export const icon = ${componentName}; } ); + // Replace static SVGs IDs with dynamic JSX that uses the htmlIdGenerator + if (hasIds) { + jsxSource = jsxSource + .replace(/id="(\S+)"/gi, "id={generateId('$1')}") + .replace(/"url\(#(\S+)\)"/gi, "{`url(#${generateId('$1')})`}") + .replace(/xlinkHref="#(\S+)"/gi, "xlinkHref={`#${generateId('$1')}`}"); + } + const outputFilePath = filePath.replace(/\.svg$/, '.js'); const comment = '// THIS IS A GENERATED FILE. DO NOT MODIFY MANUALLY\n\n'; fs.writeFileSync(outputFilePath, comment + jsxSource); From e3b05827f32da484003a7fe211448bcbb3378fcd Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Wed, 22 Sep 2021 12:52:44 -0700 Subject: [PATCH 05/11] Clean up ID prefixes - in their current state, all icons (except for dropwizard) will automatically truncate their ID names to 'a', 'b', etc. when cleanupIDs is set to true --- scripts/compile-icons.js | 2 +- src/components/icon/assets/logo_dropwizard.svg | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts/compile-icons.js b/scripts/compile-icons.js index ae35474715b..f1f84046439 100644 --- a/scripts/compile-icons.js +++ b/scripts/compile-icons.js @@ -33,7 +33,7 @@ iconFiles.forEach(async (filePath) => { plugins: ['@svgr/plugin-svgo', '@svgr/plugin-jsx'], svgoConfig: { plugins: [ - { cleanupIDs: false }, + { cleanupIDs: true }, { prefixIds: false }, { removeViewBox: false }, ], diff --git a/src/components/icon/assets/logo_dropwizard.svg b/src/components/icon/assets/logo_dropwizard.svg index c62372fa8e8..5bd368ec9bf 100644 --- a/src/components/icon/assets/logo_dropwizard.svg +++ b/src/components/icon/assets/logo_dropwizard.svg @@ -1,17 +1,17 @@ - - + + - + - + From 0de93598c933ec70094e98f37c1711805c78e1d1 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Wed, 22 Sep 2021 12:53:41 -0700 Subject: [PATCH 06/11] Run yarn compile-icons moment of truth! --- src/components/icon/assets/logo_apache.js | 272 +++++++++--------- src/components/icon/assets/logo_dropwizard.js | 126 ++++---- src/components/icon/assets/logo_gcp.js | 100 +++---- src/components/icon/assets/logo_google_g.js | 138 ++++----- src/components/icon/assets/logo_haproxy.js | 160 ++++++----- src/components/icon/assets/logo_ibm.js | 204 ++++++------- src/components/icon/assets/logo_memcached.js | 166 ++++++----- src/components/icon/assets/logo_php.js | 170 ++++++----- 8 files changed, 691 insertions(+), 645 deletions(-) diff --git a/src/components/icon/assets/logo_apache.js b/src/components/icon/assets/logo_apache.js index 746c8c74ce5..dc155aa00b3 100644 --- a/src/components/icon/assets/logo_apache.js +++ b/src/components/icon/assets/logo_apache.js @@ -1,140 +1,144 @@ // THIS IS A GENERATED FILE. DO NOT MODIFY MANUALLY import * as React from 'react'; +import { htmlIdGenerator } from '../../../services'; -const EuiIconLogoApache = ({ title, titleId, ...props }) => ( - - {title ? {title} : null} - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -); +const EuiIconLogoApache = ({ title, titleId, ...props }) => { + const generateId = htmlIdGenerator('logo_apache'); + return ( + + {title ? {title} : null} + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + ); +}; export const icon = EuiIconLogoApache; diff --git a/src/components/icon/assets/logo_dropwizard.js b/src/components/icon/assets/logo_dropwizard.js index 472e28bf7fe..7c9bbbcfad3 100644 --- a/src/components/icon/assets/logo_dropwizard.js +++ b/src/components/icon/assets/logo_dropwizard.js @@ -1,67 +1,71 @@ // THIS IS A GENERATED FILE. DO NOT MODIFY MANUALLY import * as React from 'react'; +import { htmlIdGenerator } from '../../../services'; -const EuiIconLogoDropwizard = ({ title, titleId, ...props }) => ( - - {title ? {title} : null} - - - - - - - - - - - - - - - - - - - - -); +const EuiIconLogoDropwizard = ({ title, titleId, ...props }) => { + const generateId = htmlIdGenerator('logo_dropwizard'); + return ( + + {title ? {title} : null} + + + + + + + + + + + + + + + + + + + + + ); +}; export const icon = EuiIconLogoDropwizard; diff --git a/src/components/icon/assets/logo_gcp.js b/src/components/icon/assets/logo_gcp.js index ba76bf6f2f9..9ecd296a4c5 100644 --- a/src/components/icon/assets/logo_gcp.js +++ b/src/components/icon/assets/logo_gcp.js @@ -1,58 +1,62 @@ // THIS IS A GENERATED FILE. DO NOT MODIFY MANUALLY import * as React from 'react'; +import { htmlIdGenerator } from '../../../services'; -const EuiIconLogoGcp = ({ title, titleId, ...props }) => ( - - {title ? {title} : null} - - - - - - - - - - - - +const EuiIconLogoGcp = ({ title, titleId, ...props }) => { + const generateId = htmlIdGenerator('logo_gcp'); + return ( + + {title ? {title} : null} + + + + + + + + + + + + + + - - - -); + + ); +}; export const icon = EuiIconLogoGcp; diff --git a/src/components/icon/assets/logo_google_g.js b/src/components/icon/assets/logo_google_g.js index 17cb77e1909..495c7dcda54 100644 --- a/src/components/icon/assets/logo_google_g.js +++ b/src/components/icon/assets/logo_google_g.js @@ -1,82 +1,86 @@ // THIS IS A GENERATED FILE. DO NOT MODIFY MANUALLY import * as React from 'react'; +import { htmlIdGenerator } from '../../../services'; -const EuiIconLogoGoogleG = ({ title, titleId, ...props }) => ( - - {title ? {title} : null} - - - - - - - - - - - +const EuiIconLogoGoogleG = ({ title, titleId, ...props }) => { + const generateId = htmlIdGenerator('logo_google_g'); + return ( + + {title ? {title} : null} + - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + - - -); + + ); +}; export const icon = EuiIconLogoGoogleG; diff --git a/src/components/icon/assets/logo_haproxy.js b/src/components/icon/assets/logo_haproxy.js index d960a45fa92..36370a837dc 100644 --- a/src/components/icon/assets/logo_haproxy.js +++ b/src/components/icon/assets/logo_haproxy.js @@ -1,84 +1,88 @@ // THIS IS A GENERATED FILE. DO NOT MODIFY MANUALLY import * as React from 'react'; +import { htmlIdGenerator } from '../../../services'; -const EuiIconLogoHaproxy = ({ title, titleId, ...props }) => ( - - {title ? {title} : null} - - - - - - - - - - - - - - - - - - - - -); +const EuiIconLogoHaproxy = ({ title, titleId, ...props }) => { + const generateId = htmlIdGenerator('logo_haproxy'); + return ( + + {title ? {title} : null} + + + + + + + + + + + + + + + + + + + + + ); +}; export const icon = EuiIconLogoHaproxy; diff --git a/src/components/icon/assets/logo_ibm.js b/src/components/icon/assets/logo_ibm.js index 6923b5c8f8f..634d865974c 100644 --- a/src/components/icon/assets/logo_ibm.js +++ b/src/components/icon/assets/logo_ibm.js @@ -1,106 +1,110 @@ // THIS IS A GENERATED FILE. DO NOT MODIFY MANUALLY import * as React from 'react'; +import { htmlIdGenerator } from '../../../services'; -const EuiIconLogoIbm = ({ title, titleId, ...props }) => ( - - {title ? {title} : null} - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -); +const EuiIconLogoIbm = ({ title, titleId, ...props }) => { + const generateId = htmlIdGenerator('logo_ibm'); + return ( + + {title ? {title} : null} + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + ); +}; export const icon = EuiIconLogoIbm; diff --git a/src/components/icon/assets/logo_memcached.js b/src/components/icon/assets/logo_memcached.js index 5c3f5138d75..3d7fd7aef4b 100644 --- a/src/components/icon/assets/logo_memcached.js +++ b/src/components/icon/assets/logo_memcached.js @@ -1,84 +1,94 @@ // THIS IS A GENERATED FILE. DO NOT MODIFY MANUALLY import * as React from 'react'; +import { htmlIdGenerator } from '../../../services'; -const EuiIconLogoMemcached = ({ title, titleId, ...props }) => ( - - {title ? {title} : null} - - - - - - - - - - - - - - - - - - - - - - - - - - - -); +const EuiIconLogoMemcached = ({ title, titleId, ...props }) => { + const generateId = htmlIdGenerator('logo_memcached'); + return ( + + {title ? {title} : null} + + + + + + + + + + + + + + + + + + + + + + + + + + + + ); +}; export const icon = EuiIconLogoMemcached; diff --git a/src/components/icon/assets/logo_php.js b/src/components/icon/assets/logo_php.js index b2ed53c6b2a..9b52bec56ea 100644 --- a/src/components/icon/assets/logo_php.js +++ b/src/components/icon/assets/logo_php.js @@ -1,89 +1,101 @@ // THIS IS A GENERATED FILE. DO NOT MODIFY MANUALLY import * as React from 'react'; +import { htmlIdGenerator } from '../../../services'; -const EuiIconLogoPhp = ({ title, titleId, ...props }) => ( - - {title ? {title} : null} - - - - - { + const generateId = htmlIdGenerator('logo_php'); + return ( + + {title ? {title} : null} + + + + + + + + + + + + - - - - - - - - - - + + + + + + + + + + - - + + + + + + + + + + + + + - - - - - - - - - - - - - - - - - - - -); + + ); +}; export const icon = EuiIconLogoPhp; From 306c375dfe737312e32cdd73ed5f14c6b2f2f81b Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Wed, 22 Sep 2021 12:53:57 -0700 Subject: [PATCH 07/11] Update snapshots --- .../icon/__snapshots__/icon.test.tsx.snap | 144 +++++++++--------- 1 file changed, 72 insertions(+), 72 deletions(-) diff --git a/src/components/icon/__snapshots__/icon.test.tsx.snap b/src/components/icon/__snapshots__/icon.test.tsx.snap index e6be04a6279..4533372f50d 100644 --- a/src/components/icon/__snapshots__/icon.test.tsx.snap +++ b/src/components/icon/__snapshots__/icon.test.tsx.snap @@ -3761,7 +3761,7 @@ exports[`EuiIcon props type logoApache is rendered 1`] = ` > @@ -4255,11 +4255,11 @@ exports[`EuiIcon props type logoDropwizard is rendered 1`] = ` > @@ -4911,7 +4911,7 @@ exports[`EuiIcon props type logoHAproxy is rendered 1`] = ` xmlns="http://www.w3.org/2000/svg" > @@ -5321,7 +5321,7 @@ exports[`EuiIcon props type logoMemcached is rendered 1`] = ` cy="42.708%" fx="41.406%" fy="42.708%" - id="logo_memcached-c" + id="logo_memcached_generated-id_c" r="0%" > Date: Tue, 21 Sep 2021 08:48:40 -0700 Subject: [PATCH 08/11] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d2ca11fe922..113c2d35718 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ - Fixed `EuiDataGrid` cells when focused getting a higher `z-index` which was causing long content to overlap surrounding cells ([#5194](https://github.com/elastic/eui/pull/5194)) - Replaced the `EuiMarkdownEditor` help syntax modal with a popover when no custom plugins are available ([#5147](https://github.com/elastic/eui/pull/5147)) - Fixed multiple components unnecessarily rerendering generated IDs on every update ([#5195](https://github.com/elastic/eui/pull/5195), [#5196](https://github.com/elastic/eui/pull/5196), [#5197](https://github.com/elastic/eui/pull/5197), [#5200](https://github.com/elastic/eui/pull/#5200), [#5201](https://github.com/elastic/eui/pull/#5201)) +- Fixed logo icons with static SVG IDs causing accessibility errors when multiples of the same logo were present ([#5204](https://github.com/elastic/eui/pull/5204)) **Theme: Amsterdam** From 49b228e0eb6451f1330fec392aa8f1a932c2bc64 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 21 Sep 2021 08:49:42 -0700 Subject: [PATCH 09/11] [REVERT ME] Documentation example to test IDs/changes --- src-docs/src/views/icon/logos.js | 36 ++++++++++++++------------------ 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/src-docs/src/views/icon/logos.js b/src-docs/src/views/icon/logos.js index 8d264e3948c..5c24a15339f 100644 --- a/src-docs/src/views/icon/logos.js +++ b/src-docs/src/views/icon/logos.js @@ -11,26 +11,22 @@ import { } from '../../../../src/components'; export const iconTypes = [ - 'logoAppSearch', - 'logoBeats', - 'logoBusinessAnalytics', - 'logoCode', - 'logoCloud', - 'logoCloudEnterprise', - 'logoElastic', - 'logoElasticStack', - 'logoElasticsearch', - 'logoEnterpriseSearch', - 'logoKibana', - 'logoLogging', - 'logoLogstash', - 'logoMaps', - 'logoMetrics', - 'logoObservability', - 'logoSecurity', - 'logoSiteSearch', - 'logoUptime', - 'logoWorkplaceSearch', + 'logoApache', + 'logoApache', + 'logoDropwizard', + 'logoDropwizard', + 'logoMemcached', + 'logoMemcached', + 'logoIBM', + 'logoIBM', + 'logoHAproxy', + 'logoHAproxy', + 'logoGCP', + 'logoGCP', + 'logoGoogleG', + 'logoGoogleG', + 'logoPhp', + 'logoPhp', ].sort(); export default () => ( From 9942426cce51817e92415892e8162c02b2ecfaec Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 23 Sep 2021 09:24:34 -0700 Subject: [PATCH 10/11] Update changelog --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59c2dd54908..5d70cffa87b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ ## [`master`](https://github.com/elastic/eui/tree/master) -No public interface changes since `38.1.0`. +**Bug fixes** + +- Fixed logo icons with static SVG IDs causing accessibility errors when multiples of the same logo were present ([#5204](https://github.com/elastic/eui/pull/5204)) ## [`38.1.0`](https://github.com/elastic/eui/tree/v38.1.0) @@ -17,7 +19,6 @@ No public interface changes since `38.1.0`. - Fixed `EuiDataGrid` cells when focused getting a higher `z-index` which was causing long content to overlap surrounding cells ([#5194](https://github.com/elastic/eui/pull/5194)) - Replaced the `EuiMarkdownEditor` help syntax modal with a popover when no custom plugins are available ([#5147](https://github.com/elastic/eui/pull/5147)) - Fixed multiple components unnecessarily rerendering generated IDs on every update ([#5195](https://github.com/elastic/eui/pull/5195), [#5196](https://github.com/elastic/eui/pull/5196), [#5197](https://github.com/elastic/eui/pull/5197), [#5200](https://github.com/elastic/eui/pull/#5200), [#5201](https://github.com/elastic/eui/pull/#5201)) -- Fixed logo icons with static SVG IDs causing accessibility errors when multiples of the same logo were present ([#5204](https://github.com/elastic/eui/pull/5204)) **Theme: Amsterdam** From 19249a463e148077b4e031125ed131b3a592964a Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 23 Sep 2021 09:26:16 -0700 Subject: [PATCH 11/11] Revert "[REVERT ME] Documentation example to test IDs/changes" This reverts commit 49b228e0eb6451f1330fec392aa8f1a932c2bc64. --- src-docs/src/views/icon/logos.js | 36 ++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src-docs/src/views/icon/logos.js b/src-docs/src/views/icon/logos.js index 5c24a15339f..8d264e3948c 100644 --- a/src-docs/src/views/icon/logos.js +++ b/src-docs/src/views/icon/logos.js @@ -11,22 +11,26 @@ import { } from '../../../../src/components'; export const iconTypes = [ - 'logoApache', - 'logoApache', - 'logoDropwizard', - 'logoDropwizard', - 'logoMemcached', - 'logoMemcached', - 'logoIBM', - 'logoIBM', - 'logoHAproxy', - 'logoHAproxy', - 'logoGCP', - 'logoGCP', - 'logoGoogleG', - 'logoGoogleG', - 'logoPhp', - 'logoPhp', + 'logoAppSearch', + 'logoBeats', + 'logoBusinessAnalytics', + 'logoCode', + 'logoCloud', + 'logoCloudEnterprise', + 'logoElastic', + 'logoElasticStack', + 'logoElasticsearch', + 'logoEnterpriseSearch', + 'logoKibana', + 'logoLogging', + 'logoLogstash', + 'logoMaps', + 'logoMetrics', + 'logoObservability', + 'logoSecurity', + 'logoSiteSearch', + 'logoUptime', + 'logoWorkplaceSearch', ].sort(); export default () => (