From 14c0bfc4d8cf1eb57b865856e6ecf0a7461d0be4 Mon Sep 17 00:00:00 2001
From: Miguel Rincon <mrincon@gitlab.com>
Date: Mon, 24 Mar 2025 10:49:53 +0100
Subject: [PATCH] feat(GlChart): Allow charts to opt-out of aria

This change allows charts to explicitly disable
aria by setting it to false. If aria's properties
are not set, it gets enabled by default.

Additionally, it refactors the merging the options
object to make the code more readable.
---
 src/components/charts/chart/chart.spec.js     | 112 +++++++++++-------
 src/components/charts/chart/chart.vue         |  37 +++---
 .../__snapshots__/column_chart.spec.js.snap   |   9 --
 .../__snapshots__/stacked_column.spec.js.snap |  12 --
 src/utils/charts/config.js                    |   3 -
 5 files changed, 89 insertions(+), 84 deletions(-)

diff --git a/src/components/charts/chart/chart.spec.js b/src/components/charts/chart/chart.spec.js
index cf1744c8af..0093f67a5c 100644
--- a/src/components/charts/chart/chart.spec.js
+++ b/src/components/charts/chart/chart.spec.js
@@ -104,42 +104,88 @@ describe('chart component', () => {
     expect(wrapper.vm.chart).toBeDefined();
   });
 
-  describe('sets chart options', () => {
-    beforeEach(async () => {
-      createWrapper({
-        options: {
-          title: { text: 'My chart title' },
-        },
+  describe('chart options', () => {
+    describe('when options are set', () => {
+      beforeEach(async () => {
+        createWrapper({
+          options: {
+            title: { text: 'My chart title' },
+          },
+        });
+        await nextTick();
       });
-      await nextTick();
-    });
 
-    it('inits chart options, adding aria.enabled', async () => {
-      expect(wrapper.vm.chart.setOption).toHaveBeenCalledTimes(1);
-      expect(wrapper.vm.chart.setOption).toHaveBeenCalledWith({
-        title: { text: 'My chart title' },
-        aria: { enabled: true },
+      it('inits chart options, adding aria.enabled', async () => {
+        expect(wrapper.vm.chart.setOption).toHaveBeenCalledTimes(1);
+        expect(wrapper.vm.chart.setOption).toHaveBeenCalledWith({
+          title: { text: 'My chart title' },
+          aria: { enabled: true },
+        });
+
+        expect(wrapper.emitted('updated').length).toEqual(1);
       });
 
-      expect(wrapper.emitted('updated').length).toEqual(1);
-    });
+      it('reacts to options changes by merging options', async () => {
+        wrapper.setProps({
+          options: {
+            title: { text: 'My new chart title' },
+            xAxis: { show: true },
+          },
+        });
+        await nextTick();
 
-    it('reacts to options changes by merging options', async () => {
-      wrapper.setProps({
-        options: {
+        expect(wrapper.vm.chart.setOption).toHaveBeenCalledTimes(2);
+        expect(wrapper.vm.chart.setOption).toHaveBeenLastCalledWith({
+          aria: { enabled: true },
           title: { text: 'My new chart title' },
           xAxis: { show: true },
-        },
+        });
+        expect(wrapper.emitted('updated').length).toEqual(2);
       });
-      await nextTick();
+    });
+
+    describe('when aria is explicitly disabled', () => {
+      it('does not enable aria', async () => {
+        createWrapper({
+          options: { aria: { enabled: false } },
+        });
+        await nextTick();
 
-      expect(wrapper.vm.chart.setOption).toHaveBeenCalledTimes(2);
-      expect(wrapper.vm.chart.setOption).toHaveBeenLastCalledWith({
-        title: { text: 'My new chart title' },
-        xAxis: { show: true },
-        aria: { enabled: true },
+        expect(wrapper.vm.chart.setOption).toHaveBeenCalledWith({
+          aria: { enabled: false },
+        });
+      });
+    });
+
+    describe('when toolbox is enabled', () => {
+      it('sets grid top to `toolboxHeight`', async () => {
+        createWrapper({
+          options: { toolbox: { show: true } },
+        });
+        await nextTick();
+
+        expect(wrapper.vm.chart.setOption).toHaveBeenCalledWith({
+          aria: { enabled: true },
+          toolbox: { show: true },
+          grid: { top: toolboxHeight },
+        });
+      });
+
+      it('increases grid top by `toolboxHeight`', async () => {
+        createWrapper({
+          options: {
+            toolbox: { show: true },
+            grid: { top: 100 },
+          },
+        });
+        await nextTick();
+
+        expect(wrapper.vm.chart.setOption).toHaveBeenCalledWith({
+          aria: { enabled: true },
+          toolbox: { show: true },
+          grid: { top: 100 + toolboxHeight },
+        });
       });
-      expect(wrapper.emitted('updated').length).toEqual(2);
     });
   });
 
@@ -206,18 +252,4 @@ describe('chart component', () => {
       });
     });
   });
-
-  describe('with toolbox in options', () => {
-    it('increases grid top by `toolboxHeight`', async () => {
-      const optionsWithToolbox = { toolbox: { show: true } };
-      wrapper = shallowMount(Chart, { propsData: { options: optionsWithToolbox } });
-      await nextTick();
-
-      expect(wrapper.vm.chart.setOption).toHaveBeenCalledWith({
-        ...optionsWithToolbox,
-        grid: { top: toolboxHeight },
-        aria: { enabled: true },
-      });
-    });
-  });
 });
diff --git a/src/components/charts/chart/chart.vue b/src/components/charts/chart/chart.vue
index c2e81dad29..361d22ac81 100644
--- a/src/components/charts/chart/chart.vue
+++ b/src/components/charts/chart/chart.vue
@@ -1,6 +1,7 @@
 <!-- eslint-disable vue/multi-word-component-names -->
 <script>
 import * as echarts from 'echarts';
+import merge from 'lodash/merge';
 import {
   defaultHeight,
   defaultWidth,
@@ -17,16 +18,6 @@ import { debounceByAnimationFrame } from '../../../utils/utils';
  */
 const sizeValidator = (size) => Number.isFinite(size) || size === 'auto' || size == null;
 
-const isChartWithToolbox = (options) => Boolean(options?.toolbox?.show);
-
-const increaseChartGridTop = (options, increaseBy) => ({
-  ...options,
-  grid: {
-    ...options.grid,
-    top: (options?.grid?.top || 0) + increaseBy,
-  },
-});
-
 export default {
   name: 'GlChart',
   directives: {
@@ -90,10 +81,21 @@ export default {
     };
   },
   computed: {
-    normalizedOptions() {
-      return isChartWithToolbox(this.options)
-        ? increaseChartGridTop(this.options, toolboxHeight)
-        : this.options;
+    modifiedOptions() {
+      let options = { ...this.options };
+
+      // Enable aria by default
+      if (options.aria?.enabled === undefined) {
+        options = merge({}, options, { aria: { enabled: true } });
+      }
+
+      // Add space at the top to fit the toolbox
+      if (options.toolbox?.show === true) {
+        const top = (options.grid?.top || 0) + toolboxHeight;
+        options = merge({}, options, { grid: { top } });
+      }
+
+      return options;
     },
   },
   watch: {
@@ -143,12 +145,7 @@ export default {
   },
   methods: {
     draw() {
-      this.chart.setOption({
-        ...this.normalizedOptions,
-        aria: {
-          enabled: true,
-        },
-      });
+      this.chart.setOption(this.modifiedOptions);
       /**
        * Emitted after calling `echarts.setOption`
        */
diff --git a/src/components/charts/column/__snapshots__/column_chart.spec.js.snap b/src/components/charts/column/__snapshots__/column_chart.spec.js.snap
index 6f16ad0299..ceb8baad24 100644
--- a/src/components/charts/column/__snapshots__/column_chart.spec.js.snap
+++ b/src/components/charts/column/__snapshots__/column_chart.spec.js.snap
@@ -2,9 +2,6 @@
 
 exports[`column chart component mounted should correctly render the chart 1`] = `
 Object {
-  "aria": Object {
-    "enabled": true,
-  },
   "grid": Object {
     "bottom": 44,
     "left": 64,
@@ -99,9 +96,6 @@ Object {
 
 exports[`column chart component with line data provided should correctly render the chart 1`] = `
 Object {
-  "aria": Object {
-    "enabled": true,
-  },
   "grid": Object {
     "bottom": 44,
     "left": 64,
@@ -190,9 +184,6 @@ Object {
 
 exports[`column chart component with secondary axis data provided should correctly render the chart 1`] = `
 Object {
-  "aria": Object {
-    "enabled": true,
-  },
   "grid": Object {
     "bottom": 44,
     "left": 64,
diff --git a/src/components/charts/stacked_column/__snapshots__/stacked_column.spec.js.snap b/src/components/charts/stacked_column/__snapshots__/stacked_column.spec.js.snap
index e8450d88b7..7fd15aa36a 100644
--- a/src/components/charts/stacked_column/__snapshots__/stacked_column.spec.js.snap
+++ b/src/components/charts/stacked_column/__snapshots__/stacked_column.spec.js.snap
@@ -2,9 +2,6 @@
 
 exports[`stacked column chart component color palette custom palette matches the snapshot 1`] = `
 Object {
-  "aria": Object {
-    "enabled": true,
-  },
   "grid": Object {
     "bottom": 44,
     "left": 64,
@@ -261,9 +258,6 @@ Object {
 
 exports[`stacked column chart component should correctly render the chart 1`] = `
 Object {
-  "aria": Object {
-    "enabled": true,
-  },
   "grid": Object {
     "bottom": 44,
     "left": 64,
@@ -520,9 +514,6 @@ Object {
 
 exports[`stacked column chart component with line data provided should correctly render the chart 1`] = `
 Object {
-  "aria": Object {
-    "enabled": true,
-  },
   "grid": Object {
     "bottom": 44,
     "left": 64,
@@ -628,9 +619,6 @@ Object {
 
 exports[`stacked column chart component with secondary axis data provided should correctly render the chart 1`] = `
 Object {
-  "aria": Object {
-    "enabled": true,
-  },
   "grid": Object {
     "bottom": 44,
     "left": 64,
diff --git a/src/utils/charts/config.js b/src/utils/charts/config.js
index eab3e9c521..023deb5b99 100644
--- a/src/utils/charts/config.js
+++ b/src/utils/charts/config.js
@@ -46,9 +46,6 @@ export const defaultChartOptions = {
   grid,
   xAxis,
   yAxis,
-  aria: {
-    enabled: true,
-  },
 };
 
 export const gridWithSecondaryYAxis = {
-- 
GitLab