Merge pull request google#5580 from google/fix-5572-webfont

Only try to load custom fonts in Google Charts when Google WebFont is not loaded
techanvil · Jul 20, 2022 · 23b93e4 · 23b93e4
2 parents 24e6a63 + 428e90e
commit 23b93e4
Show file tree

Hide file tree

Showing 4 changed files with 80 additions and 5 deletions.
diff --git a/assets/js/components/GoogleChart.js b/assets/js/components/GoogleChart.js
@@ -48,6 +48,7 @@ import { useFeature } from '../hooks/useFeature';
 import { CORE_USER } from '../googlesitekit/datastore/user/constants';
 import GatheringDataNotice, { NOTICE_STYLE } from './GatheringDataNotice';
 import Data from 'googlesitekit-data';
+import { chartFontName } from '../util/chart';
 const { useSelect } = Data;
 
 export default function GoogleChart( props ) {
@@ -228,21 +229,21 @@ export default function GoogleChart( props ) {
 	merge( chartOptions, {
 		hAxis: {
 			textStyle: {
-				fontName: 'Google Sans Text',
+				fontName: chartFontName(),
 				fontSize: 10,
 				color: '#5f6561',
 			},
 		},
 		vAxis: {
 			textStyle: {
-				fontName: 'Google Sans Text',
+				fontName: chartFontName(),
 				color: '#5f6561',
 				fontSize: 10,
 			},
 		},
 		legend: {
 			textStyle: {
-				fontName: 'Google Sans Text',
+				fontName: chartFontName(),
 				color: '#131418',
 				fontSize: 12,
 			},

diff --git a/...odules/analytics/components/dashboard/DashboardAllTrafficWidget/UserDimensionsPieChart.js b/...odules/analytics/components/dashboard/DashboardAllTrafficWidget/UserDimensionsPieChart.js
@@ -49,6 +49,7 @@ import {
 	trackEvent,
 	getChartDifferenceArrow,
 	isSingleSlice,
+	chartFontName,
 } from '../../../../../util';
 import { extractAnalyticsDataForPieChart } from '../../../util';
 import GoogleChart from '../../../../../components/GoogleChart';
@@ -725,7 +726,7 @@ UserDimensionsPieChart.chartOptions = {
 	pieSliceTextStyle: {
 		color: '#131418',
 		fontSize: 12,
-		fontName: 'Google Sans Text',
+		fontName: chartFontName(),
 	},
 	slices: {
 		0: { color: '#fece72' },

diff --git a/assets/js/util/chart.js b/assets/js/util/chart.js
@@ -98,3 +98,31 @@ export const calculateDifferenceBetweenChartValues = (
 	}
 	return 0;
 };
+
+/**
+ * Returns a font name only if the Google Web Font loader script isn't loaded.
+ *
+ * @since n.e.x.t
+ *
+ * @param {string=} fontName Optional. Font name string to return. Defaults to `'"Google Sans Text", "Helvetica Neue", Helvetica, Arial, sans-serif'`.
+ * @return {string|undefined} The font string supplied if `WebFont` is not set. `undefined` if the `WebFont` global is set.
+ */
+export const chartFontName = (
+	fontName = '"Google Sans Text", "Helvetica Neue", Helvetica, Arial, sans-serif'
+) => {
+	// If the Google `WebFont` global exists and we try to use
+	// the `fontName` property in a Google Chart, we'll encounter an
+	// exception and the entire dashboard will crash.
+	//
+	// Our "workaround" is not to load custom fonts when the `WebFont`
+	// global exists, as these are usually very small UI elements
+	// and there's no clear way to get them to load without errors
+	// when the `WebFont` global/script is loaded.
+	//
+	// See: https://github.com/google/site-kit-wp/issues/5572
+	if ( typeof WebFont !== 'undefined' ) {
+		return undefined;
+	}
+
+	return fontName;
+};
diff --git a/assets/js/util/chart.test.js b/assets/js/util/chart.test.js
@@ -19,7 +19,11 @@
 /**
  * Internal dependencies.
  */
-import { isSingleSlice, calculateDifferenceBetweenChartValues } from './chart';
+import {
+	isSingleSlice,
+	calculateDifferenceBetweenChartValues,
+	chartFontName,
+} from './chart';
 
 describe( 'isSingleSlice', () => {
 	it( 'returns undefined when undefined is passed', () => {
@@ -264,3 +268,44 @@ describe( 'calculateDifferenceBetweenChartValues', () => {
 		}
 	);
 } );
+
+describe( 'chartFontName', () => {
+	beforeEach( () => {
+		global.WebFont = undefined;
+	} );
+
+	afterAll( () => {
+		global.WebFont = undefined;
+	} );
+
+	it( 'should return the default font name when no fontName argument is used', () => {
+		expect( chartFontName() ).toMatchInlineSnapshot(
+			'"\\"Google Sans Text\\", \\"Helvetica Neue\\", Helvetica, Arial, sans-serif"'
+		);
+	} );
+
+	it( 'should not return the default font styles WebFont is defined', () => {
+		global.WebFont = {};
+
+		expect( chartFontName() ).toBeUndefined();
+	} );
+
+	it( 'should return the supplied font name string when WebFont is undefined', () => {
+		expect( chartFontName( '"Times New Roman"' ) ).toEqual(
+			'"Times New Roman"'
+		);
+	} );
+
+	it( 'should not return the the supplied font name string when WebFont is defined', () => {
+		global.WebFont = {};
+
+		expect( chartFontName( '"Times New Roman"' ) ).toBeUndefined();
+	} );
+
+	it( 'considers any non-undefined value to be defined', () => {
+		global.WebFont = null;
+
+		expect( chartFontName() ).toBeUndefined();
+		expect( chartFontName( '"Times New Roman"' ) ).toBeUndefined();
+	} );
+} );