[NTP] Remove nesting of a and button in mv tiles

Having a button within an anchor doesn't work well with some screen readers. The goal was to unnest the button from the anchor without changing any functionality or visuals. It should look the same as before. * Made the tile a div instead and put the link within it. * <a> has to have the border radius and full width and height since focus will not technically be on the <a> within the tile rather than on the tile itself. * Updated tests to use the <a> for href checks and to put focus on. * Updated tests to select ".tile" instead of "a" when getting list of tiles. http://screenshot/8sRsM5KeqLroubs Bug: 1309708 Change-Id: I55455d5506afb981d89cff1fae9868a01c8d2a11 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4688456 Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org> Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com> Commit-Queue: Riley Tatum <rtatum@google.com> Cr-Commit-Position: refs/heads/main@{#1185060}
chromium · Aug 18, 2023 · aebb18b · aebb18b
1 parent 0a0bf1e
commit aebb18b
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 43 deletions.
diff --git a/chrome/test/data/webui/cr_components/most_visited_focus_test.ts b/chrome/test/data/webui/cr_components/most_visited_focus_test.ts
@@ -58,7 +58,7 @@ suite('CrComponentsMostVisitedFocusTest', () => {
   test('right focuses on addShortcut', async () => {
     await addTiles(1);
     const tile = queryTiles()[0]!;
-    tile.focus();
+    tile.querySelector('a')!.focus();
     keydown(tile, 'ArrowRight');
     assertFocus(mostVisited.$.addShortcut);
   });
@@ -74,56 +74,56 @@ suite('CrComponentsMostVisitedFocusTest', () => {
   test('right focuses next tile', async () => {
     await addTiles(2);
     const tiles = queryTiles();
-    tiles[0]!.focus();
+    tiles[0]!.querySelector('a')!.focus();
     keydown(tiles[0]!, 'ArrowRight');
-    assertFocus(tiles[1]!);
+    assertFocus(tiles[1]!.querySelector('a')!);
   });
 
   test('right focuses on next tile when menu button focused', async () => {
     await addTiles(2);
     const tiles = queryTiles();
     tiles[0]!.querySelector('cr-icon-button')!.focus();
     keydown(tiles[0]!, 'ArrowRight');
-    assertFocus(tiles[1]!);
+    assertFocus(tiles[1]!.querySelector('a')!);
   });
 
   test('down focuses on addShortcut', async () => {
     await addTiles(1);
     const tile = queryTiles()[0]!;
-    tile.focus();
+    tile.querySelector('a')!.focus();
     keydown(tile, 'ArrowDown');
     assertFocus(mostVisited.$.addShortcut);
   });
 
   test('down focuses next tile', async () => {
     await addTiles(2);
     const tiles = queryTiles();
-    tiles[0]!.focus();
+    tiles[0]!.querySelector('a')!.focus();
     keydown(tiles[0]!, 'ArrowDown');
-    assertFocus(tiles[1]!);
+    assertFocus(tiles[1]!.querySelector('a')!);
   });
 
   test('up focuses on previous tile from addShortcut', async () => {
     await addTiles(1);
     mostVisited.$.addShortcut.focus();
     keydown(mostVisited.$.addShortcut, 'ArrowUp');
-    assertFocus(queryTiles()[0]!);
+    assertFocus(queryTiles()[0]!.querySelector('a')!);
   });
 
   test('up focuses on previous tile', async () => {
     await addTiles(2);
     const tiles = queryTiles();
-    tiles[1]!.focus();
+    tiles[1]!.querySelector('a')!.focus();
     keydown(tiles[1]!, 'ArrowUp');
-    assertFocus(tiles[0]!);
+    assertFocus(tiles[0]!.querySelector('a')!);
   });
 
   test('up/left does not change focus when on first tile', async () => {
     await addTiles(1);
     const tile = queryTiles()[0]!;
-    tile.focus();
+    tile.querySelector('a')!.focus();
     keydown(tile, 'ArrowUp');
-    assertFocus(tile);
+    assertFocus(tile.querySelector('a')!);
     keydown(tile, 'ArrowLeft');
   });
 

diff --git a/chrome/test/data/webui/cr_components/most_visited_test.ts b/chrome/test/data/webui/cr_components/most_visited_test.ts
@@ -206,7 +206,7 @@ function createLayoutsSuite(singleRow: boolean, reflowOnOverflow: boolean) {
     await addTiles(4);
     assertEquals(4, queryTiles().length);
     assertAddShortcutShown();
-    const tops = queryAll<HTMLElement>('a, #addShortcut')
+    const tops = queryAll<HTMLElement>('.tile, #addShortcut')
                      .map(({offsetTop}) => offsetTop);
     assertEquals(5, tops.length);
     tops.forEach(top => {
@@ -218,7 +218,7 @@ function createLayoutsSuite(singleRow: boolean, reflowOnOverflow: boolean) {
     await addTiles(5);
     assertEquals(5, queryTiles().length);
     assertAddShortcutShown();
-    const tops = queryAll<HTMLElement>('a, #addShortcut')
+    const tops = queryAll<HTMLElement>('.tile, #addShortcut')
                      .map(({offsetTop}) => offsetTop);
     assertEquals(6, tops.length);
     const firstRowTop = tops[0];
@@ -240,7 +240,7 @@ function createLayoutsSuite(singleRow: boolean, reflowOnOverflow: boolean) {
     await addTiles(9);
     assertEquals(9, queryTiles().length);
     assertAddShortcutShown();
-    const tops = queryAll<HTMLElement>('a, #addShortcut')
+    const tops = queryAll<HTMLElement>('.tile, #addShortcut')
                      .map(({offsetTop}) => offsetTop);
     assertEquals(10, tops.length);
     const firstRowTop = tops[0];
@@ -262,7 +262,8 @@ function createLayoutsSuite(singleRow: boolean, reflowOnOverflow: boolean) {
     await addTiles(10);
     assertEquals(10, queryTiles().length);
     assertAddShortcutHidden();
-    const tops = queryAll<HTMLElement>('a:not([hidden])').map(a => a.offsetTop);
+    const tops =
+        queryAll<HTMLElement>('.tile:not([hidden])').map(a => a.offsetTop);
     assertEquals(10, tops.length);
     const firstRowTop = tops[0];
     const secondRowTop = tops[5];
@@ -546,7 +547,7 @@ suite('LoggingAndUpdates', () => {
     await addTiles(1);
 
     // Act.
-    const tileLink = queryTiles()[0]!;
+    const tileLink = queryTiles()[0]!.querySelector('a')!;
     // Prevent triggering a navigation, which would break the test.
     tileLink.href = '#';
     tileLink.click();
@@ -946,7 +947,7 @@ suite('Modification', () => {
   test('tile url is set to href of <a>', async () => {
     await addTiles(1);
     const tile = queryTiles()[0]!;
-    assertEquals('https://a/', tile.href);
+    assertEquals('https://a/', tile!.querySelector('a')!.href);
   });
 
   test('delete first tile', async () => {
@@ -1054,9 +1055,9 @@ function createDragAndDropSuite(singleRow: boolean, reflowOnOverflow: boolean) {
     const tiles = queryTiles();
     const first = tiles[0]!;
     const second = tiles[1]!;
-    assertEquals('https://a/', first.href);
+    assertEquals('https://a/', first.querySelector('a')!.href);
     assertTrue(first.draggable);
-    assertEquals('https://b/', second.href);
+    assertEquals('https://b/', second.querySelector('a')!.href);
     assertTrue(second.draggable);
     const firstRect = first.getBoundingClientRect();
     const secondRect = second.getBoundingClientRect();
@@ -1074,18 +1075,18 @@ function createDragAndDropSuite(singleRow: boolean, reflowOnOverflow: boolean) {
     assertEquals('https://a/', url.url);
     assertEquals(1, newPos);
     const [newFirst, newSecond] = queryTiles();
-    assertEquals('https://b/', newFirst!.href);
-    assertEquals('https://a/', newSecond!.href);
+    assertEquals('https://b/', newFirst!.querySelector('a')!.href);
+    assertEquals('https://a/', newSecond!.querySelector('a')!.href);
   });
 
   test('drag second tile to first position', async () => {
     await addTiles(2);
     const tiles = queryTiles();
     const first = tiles[0]!;
     const second = tiles[1]!;
-    assertEquals('https://a/', first.href);
+    assertEquals('https://a/', first.querySelector('a')!.href);
     assertTrue(first.draggable);
-    assertEquals('https://b/', second.href);
+    assertEquals('https://b/', second.querySelector('a')!.href);
     assertTrue(second.draggable);
     const firstRect = first.getBoundingClientRect();
     const secondRect = second.getBoundingClientRect();
@@ -1103,18 +1104,18 @@ function createDragAndDropSuite(singleRow: boolean, reflowOnOverflow: boolean) {
     assertEquals('https://b/', url.url);
     assertEquals(0, newPos);
     const [newFirst, newSecond] = queryTiles();
-    assertEquals('https://b/', newFirst!.href);
-    assertEquals('https://a/', newSecond!.href);
+    assertEquals('https://b/', newFirst!.querySelector('a')!.href);
+    assertEquals('https://a/', newSecond!.querySelector('a')!.href);
   });
 
   test('most visited tiles cannot be reordered', async () => {
     await addTiles(2, /* customLinksEnabled= */ false);
     const tiles = queryTiles();
     const first = tiles[0]!;
     const second = tiles[1]!;
-    assertEquals('https://a/', first.href);
+    assertEquals('https://a/', first.querySelector('a')!.href);
     assertTrue(first.draggable);
-    assertEquals('https://b/', second.href);
+    assertEquals('https://b/', second.querySelector('a')!.href);
     assertTrue(second.draggable);
     const firstRect = first.getBoundingClientRect();
     const secondRect = second.getBoundingClientRect();
@@ -1129,8 +1130,8 @@ function createDragAndDropSuite(singleRow: boolean, reflowOnOverflow: boolean) {
     await flushTasks();
     assertEquals(0, handler.getCallCount('reorderMostVisitedTile'));
     const [newFirst, newSecond] = queryTiles();
-    assertEquals('https://a/', newFirst!.href);
-    assertEquals('https://b/', newSecond!.href);
+    assertEquals('https://a/', newFirst!.querySelector('a')!.href);
+    assertEquals('https://b/', newSecond!.querySelector('a')!.href);
   });
 }
 
@@ -1239,7 +1240,7 @@ suite('Prerendering', () => {
     await addTiles(1);
 
     // // Act.
-    const tileLink = queryTiles()[0]!;
+    const tileLink = queryTiles()[0]!.querySelector('a')!;
     // Prevent triggering a navigation, which would break the test.
     tileLink.href = '#';
     // simulate a mousedown event.
@@ -1256,7 +1257,7 @@ suite('Prerendering', () => {
     await addTiles(1);
 
     // // Act.
-    const tileLink = queryTiles()[0]!;
+    const tileLink = queryTiles()[0]!.querySelector('a')!;
     // Prevent triggering a navigation, which would break the test.
     tileLink.href = '#';
     // simulate a mousedown event.

diff --git a/ui/webui/resources/cr_components/most_visited/most_visited.html b/ui/webui/resources/cr_components/most_visited/most_visited.html
@@ -107,17 +107,23 @@
     width: var(--tile-size);
   }
 
-  .tile {
+  .tile a {
+    border-radius: 4px;
+    display: inline-block;
+    height: 100%;
+    outline: none;
+    position: absolute;
     touch-action: none;
+    width: 100%;
   }
 
-  :host-context(.focus-outline-visible) .tile:focus,
+  :host-context(.focus-outline-visible) .tile a:focus,
   :host-context(.focus-outline-visible) #addShortcut:focus {
     box-shadow: var(--most-visited-focus-shadow);
   }
 
   @media (forced-colors: active) {
-    :host-context(.focus-outline-visible) .tile:focus,
+    :host-context(.focus-outline-visible) .tile a:focus,
     :host-context(.focus-outline-visible) #addShortcut:focus {
       /* Use outline instead of box-shadow (which does not work) in Windows
          HCM. */
@@ -245,13 +251,16 @@
            --column-count: [[columnCount_]]; --row-count: [[rowCount_]];">
   <dom-repeat id="tiles" items="[[tiles_]]" on-dom-change="onTilesRendered_">
     <template>
-      <a class="tile" href$="[[item.url.url]]" title$="[[item.title]]"
-          aria-label="[[item.title]]" on-dragstart="onDragStart_"
-          on-touchstart="onTouchStart_" hidden$="[[isHidden_(index, maxVisibleTiles_)]]"
-          on-click="onTileClick_" on-keydown="onTileKeyDown_"
+      <div class="tile" query-tile$="[[item.isQueryTile]]"
+          hidden$="[[isHidden_(index, maxVisibleTiles_)]]"
+          title$="[[item.title]]" on-dragstart="onDragStart_"
+          on-touchstart="onTouchStart_" on-click="onTileClick_"
           on-mouseenter="onTileHover_" on-mouseleave="onTileExit_"
-          on-mousedown="onTileMouseDown_"
-          query-tile$="[[item.isQueryTile]]">
+          on-mousedown="onTileMouseDown_" on-keydown="onTileKeyDown_"
+          draggable="true">
+        <a href$="[[item.url.url]]" aria-label="[[item.title]]"
+            draggable="false">
+        </a>
         <cr-icon-button id="actionMenuButton" class="icon-more-vert"
             title="[[getMoreActionText_(item.title)]]" on-click="onTileActionButtonClick_"
             tabindex="0" hidden$="[[!customLinksEnabled_]]"></cr-icon-button>
@@ -267,7 +276,7 @@
         <div class$="tile-title [[getTileTitleDirectionClass_(item)]]">
           <span>[[item.title]]</span>
         </div>
-      </a>
+      </div>
     </template>
   </dom-repeat>
   <cr-button id="addShortcut" tabindex="0" on-click="onAdd_"

diff --git a/ui/webui/resources/cr_components/most_visited/most_visited.ts b/ui/webui/resources/cr_components/most_visited/most_visited.ts
@@ -875,7 +875,7 @@ export class MostVisitedElement extends MostVisitedElementBase {
     }
     const tileElements = this.tileElements_;
     if (index < tileElements.length) {
-      tileElements[index].focus();
+      (tileElements[index] as HTMLElement).querySelector('a')!.focus();
     } else if (this.showAdd_ && index === tileElements.length) {
       this.$.addShortcut.focus();
     }