[ntp][cr-elements] Give Panorama shortcut buttons accessible names

UI screenshot: screenshot/3c33cebpM7XihUw.png Accessibility tree screenshot: screenshot/78egjf3SGauAQoV.png Problem: We don’t define labels for our `cr-radio-button`s. This causes `aria-labelledby` [1] for our `cr-radio-button`s to be an empty string [2]. Caveat: We cannot use `cr-radio-button`'s `label` attribute as is because the styling is different than what we need. The default cr-radio-button is left aligned and only has one label. Our buttons are right aligned and have two labels. Proposed Solution: Create attribute that hides `cr-radio-button`s label text when added. *Other alternatives were discussed and vetted i.e. moving our custom button label into `cr-radio-button.` This solution was chosen because it allows us to preserve our existing shortcuts UI and create a custom aria-label which consists of both labels (follow up CL). [1] https://source.chromium.org/chromium/chromium/src/+/main:ui/webui/resources/cr_elements/cr_radio_button/cr_radio_button.html;drc=730ff74c221e59ccdf7b116ae924d9d1b150341e;l=6 [2] https://source.chromium.org/chromium/chromium/src/+/main:ui/webui/resources/cr_elements/cr_radio_button/cr_radio_button_mixin.ts;drc=ab67842ff06675153972984b5c7b7dcd7f6df395;l=53. Change-Id: I525ef69eb9bf26a65e026d0219debe1ae654beab Bug: 1416276 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4264930 Reviewed-by: John Lee <johntlee@chromium.org> Commit-Queue: Paul Adedeji <pauladedeji@google.com> Cr-Commit-Position: refs/heads/main@{#1107079}
chromium · Feb 18, 2023 · 91b2e66 · 91b2e66
1 parent 401ade9
commit 91b2e66
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 13 deletions.
diff --git a/chrome/browser/resources/side_panel/customize_chrome/shortcuts.html b/chrome/browser/resources/side_panel/customize_chrome/shortcuts.html
@@ -62,14 +62,16 @@
       on-selected-changed="onShortcutsRadioSelectionChanged_">
       <cr-radio-button id="customLinksButton"
           name="customLinksOption"
-          aria-label="$i18n{myShortcuts}">
+          label="$i18n{myShortcuts}"
+          hide-label-text>
       </cr-radio-button>
       <customize-chrome-button-label label="$i18n{myShortcuts}"
           label-description="$i18n{shortcutsCurated}">
       </customize-chrome-button-label>
       <cr-radio-button id="mostVisitedButton"
           name="mostVisitedOption"
-          aria-label="$i18n{mostVisited}">
+          label="$i18n{mostVisited}"
+          hide-label-text>
       </cr-radio-button>
       <customize-chrome-button-label label="$i18n{mostVisited}"
           label-description="$i18n{shortcutsSuggested}">

diff --git a/chrome/test/data/webui/cr_elements/cr_radio_button_test.ts b/chrome/test/data/webui/cr_elements/cr_radio_button_test.ts
@@ -6,7 +6,7 @@
 import 'chrome://resources/cr_elements/cr_radio_button/cr_radio_button.js';
 
 import {CrRadioButtonElement} from 'chrome://resources/cr_elements/cr_radio_button/cr_radio_button.js';
-import {assertEquals, assertFalse, assertTrue} from 'chrome://webui-test/chai_assert.js';
+import {assertEquals, assertNotEquals, assertFalse, assertTrue} from 'chrome://webui-test/chai_assert.js';
 // clang-format on
 
 suite('cr-radio-button', function() {
@@ -18,34 +18,46 @@ suite('cr-radio-button', function() {
     document.body.appendChild(radioButton);
   });
 
+  function assertStyle(element: Element, name: string, expected: string) {
+    const actual = getComputedStyle(element).getPropertyValue(name).trim();
+    assertEquals(expected, actual);
+  }
+
+  function assertNotStyle(element: Element, name: string, not: string) {
+    const actual = getComputedStyle(element).getPropertyValue(name).trim();
+    assertNotEquals(not, actual);
+  }
+
   function assertChecked() {
     assertTrue(radioButton.hasAttribute('checked'));
     assertEquals('true', radioButton.$.button.getAttribute('aria-checked'));
-    assertTrue(
-        getComputedStyle(radioButton.shadowRoot!.querySelector('.disc')!)
-            .backgroundColor !== 'rgba(0, 0, 0, 0)');
+    assertNotStyle(
+        radioButton.shadowRoot!.querySelector('.disc')!, 'background-color',
+        'rgba(0, 0, 0, 0)');
   }
 
   function assertNotChecked() {
     assertFalse(radioButton.hasAttribute('checked'));
     assertEquals('false', radioButton.$.button.getAttribute('aria-checked'));
-    assertEquals(
-        'rgba(0, 0, 0, 0)',
-        getComputedStyle(radioButton.shadowRoot!.querySelector('.disc')!)
-            .backgroundColor);
+    assertStyle(
+        radioButton.shadowRoot!.querySelector('.disc')!, 'background-color',
+        'rgba(0, 0, 0, 0)');
+    assertStyle(
+        radioButton.shadowRoot!.querySelector('.disc')!, 'background-color',
+        'rgba(0, 0, 0, 0)');
   }
 
   function assertDisabled() {
     assertTrue(radioButton.hasAttribute('disabled'));
     assertEquals('true', radioButton.$.button.getAttribute('aria-disabled'));
-    assertEquals('none', getComputedStyle(radioButton).pointerEvents);
-    assertTrue('1' !== getComputedStyle(radioButton).opacity);
+    assertStyle(radioButton, 'pointer-events', 'none');
+    assertNotStyle(radioButton, 'opacity', '1');
   }
 
   function assertNotDisabled() {
     assertFalse(radioButton.hasAttribute('disabled'));
     assertEquals('false', radioButton.$.button.getAttribute('aria-disabled'));
-    assertEquals('1', getComputedStyle(radioButton).opacity);
+    assertStyle(radioButton, 'opacity', '1');
   }
 
   // Setting selection by mouse/keyboard is cr-radio-group's job, so
@@ -78,4 +90,32 @@ suite('cr-radio-button', function() {
     assertFalse(
         radioButton.shadowRoot!.querySelector('paper-ripple')!.holdDown);
   });
+
+  test('Label Hidden', function() {
+    // Having no label set hides label.
+    assertStyle(
+        radioButton.shadowRoot!.querySelector('#label')!, 'display', 'none');
+
+    // Setting label shows label.
+    radioButton.label = 'foo';
+    assertNotStyle(
+        radioButton.shadowRoot!.querySelector('#label')!, 'display', 'none');
+    assertNotStyle(
+        radioButton.shadowRoot!.querySelector('#label')!, 'clip',
+        'rect(0px, 0px, 0px, 0px)');
+    assertEquals(radioButton.$.button.getAttribute('aria-labelledby'), 'label');
+    assertEquals(
+        radioButton.shadowRoot!.querySelector('#label')!.textContent!.trim(),
+        'foo');
+
+    // Setting hideLabelText true clips label from screen reader.
+    radioButton.hideLabelText = true;
+    assertStyle(
+        radioButton.shadowRoot!.querySelector('#label')!, 'clip',
+        'rect(0px, 0px, 0px, 0px)');
+    assertEquals(radioButton.$.button.getAttribute('aria-labelledby'), 'label');
+    assertEquals(
+        radioButton.shadowRoot!.querySelector('#label')!.textContent!.trim(),
+        'foo');
+  });
 });
diff --git a/chrome/test/data/webui/side_panel/customize_chrome/shortcuts_test.ts b/chrome/test/data/webui/side_panel/customize_chrome/shortcuts_test.ts
@@ -38,6 +38,13 @@ suite('ShortcutsTest', () => {
     callbackRouterRemote.setMostVisitedSettings(
         customLinksEnabled, shortcutsVisible);
     await callbackRouterRemote.$.flushForTesting();
+    assertTrue(customizeShortcutsElement.$.customLinksButton.hideLabelText);
+    assertTrue(customizeShortcutsElement.$.mostVisitedButton.hideLabelText);
+    assertEquals(
+        customizeShortcutsElement.$.customLinksButton.label, 'My shortcuts');
+    assertEquals(
+        customizeShortcutsElement.$.mostVisitedButton.label,
+        'Most visited sites');
   }
 
   function assertShown(shown: boolean) {

diff --git a/ui/webui/resources/cr_elements/cr_radio_button/cr_radio_button_mixin.ts b/ui/webui/resources/cr_elements/cr_radio_button/cr_radio_button_mixin.ts
@@ -48,6 +48,12 @@ export const CrRadioButtonMixin = dedupingMixin(
               observer: 'onFocusableChanged_',
             },
 
+            hideLabelText: {
+              type: Boolean,
+              value: false,
+              reflectToAttribute: true,
+            },
+
             label: {
               type: String,
               value: '',  // Allows hidden$= binding to run without being set.
@@ -72,6 +78,7 @@ export const CrRadioButtonMixin = dedupingMixin(
         checked: boolean;
         disabled: boolean;
         focusable: boolean;
+        hideLabelText: boolean;
         label: string;
         name: string;
         private buttonTabIndex_: number;
@@ -146,6 +153,7 @@ export interface CrRadioButtonMixinInterface {
   checked: boolean;
   disabled: boolean;
   focusable: boolean;
+  hideLabelText: boolean;
   label: string;
   name: string;
 

diff --git a/ui/webui/resources/cr_elements/cr_radio_button/cr_radio_button_style.css b/ui/webui/resources/cr_elements/cr_radio_button/cr_radio_button_style.css
@@ -65,6 +65,13 @@
         color: inherit;
       }
 
+      /* Visually hide the label but allow the screen reader to pick it up. */
+      :host([hide-label-text]) #label {
+        clip: rect(0,0,0,0);
+        display: block;
+        position: fixed;
+      }
+
       .disc-border,
       .disc,
       .disc-wrapper,