Use IDs instead of labels to identify chart legend items (https://github.com/woocommerce/woocommerce-admin/pull/1730)

* Use instanceId to generate unique ids for chart legend items

* Decouple item key and label in Chart components

* Remove more duplicate IDs in the chart legend

* Use double underscores in element IDs following BEM rules

* Move 'withInstanceId' inside D3Legend component

* Simplify screen reader labels logic

* Add CHANGELOG message

* Use 'primary' and 'secondary' as items keys in time-comparison charts

plugins/woocommerce-admin/client/analytics/components/report-chart/index.js

@@ -42,7 +42,8 @@ export class ReportChart extends Component {
 					const label = intervalData[ segment.segment_label ]
 						? segment.segment_label + ' (#' + segment.segment_id + ')'
 						: segment.segment_label;
-					intervalData[ label ] = {
+					intervalData[ segment.segment_id ] = {
+						label,
 						value: segment.subtotals[ selectedChart.key ] || 0,
 					};
 				}
@@ -59,8 +60,6 @@ export class ReportChart extends Component {
 		const { query, primaryData, secondaryData, selectedChart } = this.props;
 		const currentInterval = getIntervalForQuery( query );
 		const { primary, secondary } = getCurrentDates( query );
-		const primaryKey = `${ primary.label } (${ primary.range })`;
-		const secondaryKey = `${ secondary.label } (${ secondary.range })`;
 
 		const chartData = primaryData.data.intervals.map( function( interval, index ) {
 			const secondaryDate = getPreviousDate(
@@ -74,11 +73,13 @@ export class ReportChart extends Component {
 			const secondaryInterval = secondaryData.data.intervals[ index ];
 			return {
 				date: formatDate( 'Y-m-d\\TH:i:s', interval.date_start ),
-				[ primaryKey ]: {
+				primary: {
+					label: `${ primary.label } (${ primary.range })`,
 					labelDate: interval.date_start,
 					value: interval.subtotals[ selectedChart.key ] || 0,
 				},
-				[ secondaryKey ]: {
+				secondary: {
+					label: `${ secondary.label } (${ secondary.range })`,
 					labelDate: secondaryDate.format( 'YYYY-MM-DD HH:mm:ss' ),
 					value: ( secondaryInterval && secondaryInterval.subtotals[ selectedChart.key ] ) || 0,
 				},

plugins/woocommerce-admin/packages/components/CHANGELOG.md

@@ -1,3 +1,6 @@
+# (unreleased)
+- Chart legend component now uses withInstanceId HOC so the ids used in several HTML elements are unique.
+
 # 1.6.0
 - Chart component: new props `emptyMessage` and `baseValue`. When an empty message is provided, it will be displayed on top of the chart if there are no values different than `baseValue`.
 - Chart component: remove d3-array dependency.

plugins/woocommerce-admin/packages/components/src/chart/d3chart/legend.js

@@ -5,6 +5,7 @@
 import { __, sprintf } from '@wordpress/i18n';
 import classNames from 'classnames';
 import { Component, createRef } from '@wordpress/element';
+import { withInstanceId } from '@wordpress/compose';
 import PropTypes from 'prop-types';
 
 /**
@@ -57,6 +58,7 @@ class D3Legend extends Component {
 			interactive,
 			legendDirection,
 			legendValueFormat,
+			instanceId,
 			totalLabel,
 		} = this.props;
 		const { isScrollable } = this.state;
@@ -89,7 +91,7 @@ class D3Legend extends Component {
 								'woocommerce-legend__item-checked': row.visible,
 							} ) }
 							key={ row.key }
-							id={ row.key }
+							id={ `woocommerce-legend-${ instanceId }__item__${ row.key }` }
 							onMouseEnter={ handleLegendHover }
 							onMouseLeave={ handleLegendHover }
 							onBlur={ handleLegendHover }
@@ -97,7 +99,7 @@ class D3Legend extends Component {
 						>
 							<button
 								onClick={ handleLegendToggle }
-								id={ row.key }
+								id={ `woocommerce-legend-${ instanceId }__item-button__${ row.key }` }
 								disabled={
 									( row.visible && numberOfRowsVisible <= 1 ) ||
 									( ! row.visible && numberOfRowsVisible >= selectionLimit ) ||
@@ -108,18 +110,17 @@ class D3Legend extends Component {
 									: ''
 								}
 							>
-								<div className="woocommerce-legend__item-container" id={ row.key }>
+								<div className="woocommerce-legend__item-container">
 									<span
 										className={ classNames( 'woocommerce-legend__item-checkmark', {
 											'woocommerce-legend__item-checkmark-checked': row.visible,
 										} ) }
-										id={ row.key }
 										style={ row.visible ? { color: getColor( keys, colorScheme )( row.key ) } : null }
 									/>
-									<span className="woocommerce-legend__item-title" id={ row.key }>
+									<span className="woocommerce-legend__item-title">
-										{ row.key }
+										{ row.label }
 									</span>
-									<span className="woocommerce-legend__item-total" id={ row.key }>
+									<span className="woocommerce-legend__item-total">
 										{ getFormatter( legendValueFormat )( row.total ) }
 									</span>
 								</div>
@@ -173,6 +174,8 @@ D3Legend.propTypes = {
 	 * comparison charts when there are many.
 	 */
 	totalLabel: PropTypes.string,
+	// from withInstanceId
+	instanceId: PropTypes.number,
 };
 
 D3Legend.defaultProps = {
@@ -181,4 +184,4 @@ D3Legend.defaultProps = {
 	legendValueFormat: ',',
 };
 
-export default D3Legend;
+export default withInstanceId( D3Legend );

plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/bar-chart.js

@@ -47,7 +47,7 @@ export const drawBars = ( node, data, params, scales, formats, tooltip ) => {
 				key: row.key,
 				focus: row.focus,
 				value: get( d, [ row.key, 'value' ], 0 ),
-				label: get( d, [ row.key, 'label' ], '' ),
+				label: row.label,
 				visible: row.visible,
 				date: d.date,
 			} ) )
@@ -63,14 +63,10 @@ export const drawBars = ( node, data, params, scales, formats, tooltip ) => {
 		.attr( 'pointer-events', 'none' )
 		.attr( 'tabindex', '0' )
 		.attr( 'aria-label', d => {
-			let label = d.key;
+			let label = d.label || d.key;
 			if ( params.mode === 'time-comparison' ) {
-				if ( d.label ) {
+				const dayData = data.find( e => e.date === d.date );
-					label = d.label;
+				label = formats.screenReaderFormat( moment( dayData[ d.key ].labelDate ).toDate() );
-				} else {
-					const dayData = data.find( e => e.date === d.date );
-					label = formats.screenReaderFormat( moment( dayData[ d.key ].labelDate ).toDate() );
-				}
 			}
 			return `${ label } ${ tooltip.valueFormat( d.value ) }`;
 		} )

plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/line-chart.js

@@ -74,10 +74,10 @@ export const getLineData = ( data, orderedKeys ) =>
 		key: row.key,
 		focus: row.focus,
 		visible: row.visible,
+		label: row.label,
 		values: data.map( d => ( {
 			date: d.date,
 			focus: row.focus,
-			label: get( d, [ row.key, 'label' ], '' ),
 			value: get( d, [ row.key, 'value' ], 0 ),
 			visible: row.visible,
 		} ) ),
@@ -97,7 +97,7 @@ export const drawLines = ( node, data, params, scales, formats, tooltip ) => {
 		.append( 'g' )
 		.attr( 'class', 'line-g' )
 		.attr( 'role', 'region' )
-		.attr( 'aria-label', d => d.key );
+		.attr( 'aria-label', d => d.label || d.key );
 	const dateSpaces = getDateSpaces( data, params.uniqueDates, width, scales.xScale );
 
 	let lineStroke = width <= wideBreak || params.uniqueDates.length > 50 ? 2 : 3;
@@ -138,9 +138,7 @@ export const drawLines = ( node, data, params, scales, formats, tooltip ) => {
 			.attr( 'cy', d => scales.yScale( d.value ) )
 			.attr( 'tabindex', '0' )
 			.attr( 'aria-label', d => {
-				const label = d.label
+				const label = formats.screenReaderFormat( d.date instanceof Date ? d.date : moment( d.date ).toDate() );
-					? d.label
-					: formats.screenReaderFormat( d.date instanceof Date ? d.date : moment( d.date ).toDate() );
 				return `${ label } ${ tooltip.valueFormat( d.value ) }`;
 			} )
 			.on( 'focus', ( d, i, nodes ) => {

plugins/woocommerce-admin/packages/components/src/chart/d3chart/utils/tooltip.js

@@ -105,7 +105,7 @@ class ChartTooltip {
 		if ( d[ row.key ].labelDate ) {
 			return this.labelFormat( moment( d[ row.key ].labelDate ).toDate() );
 		}
-		return row.key;
+		return row.label || row.key;
 	}
 
 	show( d, triggerElement, parentNode, elementWidthRatio = 1 ) {

plugins/woocommerce-admin/packages/components/src/chart/index.js

@@ -51,18 +51,20 @@ d3FormatDefaultLocale( {
 } );
 
 function getOrderedKeys( props, previousOrderedKeys = [] ) {
-	const updatedKeys = [
+	const uniqueKeys = props.data.reduce( ( accum, curr ) => {
-		...new Set(
+		Object.entries( curr ).forEach( ( [ key, value ] ) => {
-			props.data.reduce( ( accum, curr ) => {
+			if ( key !== 'date' && ! accum[ key ] ) {
-				Object.keys( curr ).forEach( key => key !== 'date' && accum.push( key ) );
+				accum[ key ] = value.label;
-				return accum;
+			}
-			}, [] )
+		} );
-		),
+		return accum;
-	].map( ( key ) => {
+	}, {} );
+	const updatedKeys = Object.entries( uniqueKeys ).map( ( [ key, label ] ) => {
 		const previousKey = previousOrderedKeys.find( item => key === item.key );
 		const defaultVisibleStatus = 'item-comparison' === props.mode ? false : true;
 		return {
 			key,
+			label,
 			total: props.data.reduce( ( a, c ) => a + c[ key ].value, 0 ),
 			visible: previousKey ? previousKey.visible : defaultVisibleStatus,
 			focus: true,
@@ -155,9 +157,10 @@ class Chart extends Component {
 		if ( ! interactiveLegend ) {
 			return;
 		}
+		const key = event.currentTarget.id.split( '_' ).pop();
 		const orderedKeys = this.state.orderedKeys.map( d => ( {
 			...d,
-			visible: d.key === event.target.id ? ! d.visible : d.visible,
+			visible: d.key === key ? ! d.visible : d.visible,
 		} ) );
 		const copyEvent = { ...event }; // can't pass a synthetic event into the hover handler
 		this.setState(
@@ -172,10 +175,11 @@ class Chart extends Component {
 	}
 
 	handleLegendHover( event ) {
-		const hoverTarget = this.state.orderedKeys.filter( d => d.key === event.target.id )[ 0 ];
+		const key = event.currentTarget.id.split( '__' ).pop();
+		const hoverTarget = this.state.orderedKeys.filter( d => d.key === key )[ 0 ];
 		this.setState( {
 			orderedKeys: this.state.orderedKeys.map( d => {
-				let enterFocus = d.key === event.target.id ? true : false;
+				let enterFocus = d.key === key ? true : false;
 				enterFocus = ! hoverTarget.visible ? true : enterFocus;
 				return {
 					...d,