shipReview: Windows NextSteps feedback (#1316)

* chore: Update NextStepsGroup bottom margin

* chore: Increase min-height of card

* chore: Address d4

* fix: Bottom margin

* fix: Add blur to click handler on ShowHideButton

* fix: lint

* chore: wat

* fix: Focus-visible on ShowHideButton implementations. remove blur code

* fix: lint

* fix: card border color

* Added a failing test case for favorites expansion

* fixed favorites positioning

---------

Co-authored-by: Shane Osbourne <[email protected]>

Author: vkraucunas

special-pages/package.json

@@ -9,6 +9,7 @@
     "prebuild": "node types.mjs && node translations.mjs",
     "build": "node index.mjs",
     "build.dev": "npm run build -- --env development",
+    "lint-fix": "cd ../ && npm run lint-fix",
     "test-unit": "node --test unit-test/translations.mjs pages/duckplayer/unit-tests/embed-settings.mjs pages/new-tab/app/freemium-pir-banner/unit-tests/utils.spec.mjs",
     "test-int": "npm run test-unit && npm run build.dev && playwright test --grep-invert '@screenshots'",
     "test-int-x": "npm run test-int",

special-pages/pages/new-tab/app/components/App.js

@@ -28,7 +28,7 @@ export function App({ children }) {
 
     return (
         <div class={cn(styles.layout)} ref={wrapperRef} data-drawer-visibility={visibility}>
-            <main class={cn(styles.main)} data-customizer-kind={customizerKind}>
+            <main class={cn(styles.main)} data-main-scroller data-customizer-kind={customizerKind}>
                 <div class={styles.tube} data-platform={platformName}>
                     <WidgetList />
                     <CustomizerMenuPositionedFixed>

special-pages/pages/new-tab/app/components/ShowHide.module.css

@@ -56,6 +56,10 @@
         }
     }
 
+    > * {
+        pointer-events: none;
+    }
+
     svg {
         transition: transform .3s;
     }

special-pages/pages/new-tab/app/entry-points/nextSteps.js

@@ -4,7 +4,7 @@ import { NextStepsCustomized } from '../next-steps/NextSteps.js';
 
 export function factory() {
     return (
-        <Centered>
+        <Centered data-entry-point="nextSteps">
             <NextStepsCustomized />
         </Centered>
     );

special-pages/pages/new-tab/app/favorites/components/Favorites.js

@@ -179,54 +179,62 @@ function Inner({ rows, safeAreaRef, rowHeight, add }) {
     // hold a mutable value that we update on resize
     const gridOffset = useRef(0);
 
-    // When called, make the expensive call to `getBoundingClientRect` to determine the offset of
-    // the grid wrapper.
-    function updateGlobals() {
+    /**
+     * When called, make the expensive call to `getBoundingClientRect` to determine the offset of
+     * the grid wrapper.
+     * @param {number} scrollY
+     */
+    function updateGlobals(scrollY) {
         if (!safeAreaRef.current) return;
         const rec = safeAreaRef.current.getBoundingClientRect();
-        gridOffset.current = rec.y + window.scrollY;
+        gridOffset.current = rec.y + scrollY;
     }
 
-    // decide which the start/end indexes should be, based on scroll position.
-    // NOTE: this is called on scroll, so must not incur expensive checks/measurements - math only!
-    function setVisibleRows() {
+    /**
+     * decide which the start/end indexes should be, based on scroll position.
+     * NOTE: this is called on scroll, so must not incur expensive checks/measurements - math only!
+     * @param {number} scrollY
+     */
+    function setVisibleRowsForOffset(scrollY) {
         if (!safeAreaRef.current) return console.warn('cannot access ref');
         if (!gridOffset.current) return console.warn('cannot access ref');
         const offset = gridOffset.current;
-        const end = window.scrollY + window.innerHeight - offset;
+        const end = scrollY + window.innerHeight - offset;
         let start;
-        if (offset > window.scrollY) {
+        if (offset > scrollY) {
             start = 0;
         } else {
-            start = window.scrollY - offset;
+            start = scrollY - offset;
         }
         const startIndex = Math.floor(start / rowHeight);
         const endIndex = Math.min(Math.ceil(end / rowHeight), rows.length);
         setVisibleRange({ start: startIndex, end: endIndex });
     }
 
     useLayoutEffect(() => {
+        const stableElement = document.querySelector('[data-main-scroller]');
+        if (!stableElement) return console.warn('cannot find scrolling element');
         // always update globals first
-        updateGlobals();
+        updateGlobals(stableElement.scrollTop);
 
         // and set visible rows once the size is known
-        setVisibleRows();
+        setVisibleRowsForOffset(stableElement.scrollTop);
 
         const controller = new AbortController();
 
         window.addEventListener(
             'resize',
             () => {
-                updateGlobals();
-                setVisibleRows();
+                updateGlobals(stableElement.scrollTop);
+                setVisibleRowsForOffset(stableElement.scrollTop);
             },
             { signal: controller.signal },
         );
 
-        window.addEventListener(
+        stableElement.addEventListener(
             'scroll',
             () => {
-                setVisibleRows();
+                setVisibleRowsForOffset(stableElement.scrollTop);
             },
             { signal: controller.signal },
         );

special-pages/pages/new-tab/app/favorites/components/Favorites.module.css

@@ -17,11 +17,6 @@
             opacity: 1;
         }
     }
-    &:focus-within {
-        .showhideVisible [aria-controls] {
-            opacity: 1;
-        }
-    }
 }
 
 .showhide {

special-pages/pages/new-tab/app/favorites/integration-tests/favorites.page.js

@@ -1,6 +1,7 @@
 import { expect } from '@playwright/test';
 
 export class FavoritesPage {
+    static ENTRY_POINT = '[data-entry-point="favorites"]';
     /**
      * @param {import("../../../integration-tests/new-tab.page.js").NewtabPage} ntp
      */
@@ -10,7 +11,7 @@ export class FavoritesPage {
 
     async togglesExpansion() {
         const { page } = this.ntp;
-        await page.getByLabel('Show more (10 remaining)').click();
+        await this.showMore();
         await expect(page.getByLabel('Add Favorite')).toBeVisible();
         await page.getByLabel('Show less').click();
         await expect(page.getByLabel('Add Favorite')).not.toBeVisible();
@@ -36,12 +37,19 @@ export class FavoritesPage {
 
     async addsAnItem() {
         const { page } = this.ntp;
-        await page.pause();
-        await page.getByLabel('Show more (10 remaining)').click();
+        await this.showMore();
         await page.getByLabel('Add Favorite').click();
         await this.ntp.mocks.waitForCallCount({ method: 'favorites_add', count: 1 });
     }
 
+    /**
+     * @param {number|string} count
+     */
+    async showMore(count = '10') {
+        const { page } = this.ntp;
+        await page.locator(FavoritesPage.ENTRY_POINT).getByLabel(`Show more (${count} remaining)`).click();
+    }
+
     async rightClickInvokesContextMenuFor() {
         const first = this.nthFavorite(0);
         const second = this.nthFavorite(1);
@@ -263,4 +271,14 @@ export class FavoritesPage {
             },
         });
     }
+
+    async scrollToContainer() {
+        const { page } = this.ntp;
+        const rect = await page.locator(FavoritesPage.ENTRY_POINT).evaluate((e) => e.getBoundingClientRect());
+        // scroll to the top of the container
+        await page.evaluate((y) => window.scrollBy(0, y), rect.top);
+
+        // give chance for any DOM changes to occur
+        await page.waitForTimeout(500);
+    }
 }

special-pages/pages/new-tab/app/favorites/integration-tests/favorites.spec.js

@@ -115,4 +115,35 @@ test.describe('newtab favorites', () => {
         await ntp.reducedMotion();
         await ntp.openPage({ favorites: 'fallbacks' });
     });
+    test('expansion works with expanded items above', async ({ page }, workerInfo) => {
+        const ntp = NewtabPage.create(page, workerInfo);
+        await ntp.reducedMotion();
+
+        const favorites = new FavoritesPage(ntp);
+
+        // open the page with enough next-step + favorites for both to be expanded
+        await ntp.openPage({
+            nextSteps: ['bringStuff', 'defaultApp', 'blockCookies', 'duckplayer'],
+            favorites: '16',
+        });
+
+        // expand next-steps
+        // todo: move this to a page-object in next-steps
+        await page.locator('[data-entry-point="nextSteps"]').getByLabel('Show More', { exact: true }).click();
+
+        // first load should have 6
+        await favorites.waitForNumFavorites(6);
+
+        // show more
+        await favorites.showMore(10);
+
+        // now should have 16 rendered
+        await favorites.waitForNumFavorites(16);
+
+        // scroll to the top of the favorites widget
+        await favorites.scrollToContainer();
+
+        // assert there's still 16 showing
+        await favorites.waitForNumFavorites(16);
+    });
 });

special-pages/pages/new-tab/app/next-steps/components/NextSteps.module.css

@@ -9,8 +9,9 @@
     justify-content: flex-start;
     align-items: center;
     text-align: center;
+    backdrop-filter: blur(48px);
     max-width: calc(240 * var(--px-in-rem));
-    min-height: calc(150 * var(--px-in-rem));
+    min-height: calc(166 * var(--px-in-rem));
     font-size: var(--body-font-size);
 }
 
@@ -137,6 +138,7 @@
     height: 100%;
     width: 100%;
     position: relative;
+    margin-bottom: var(--sp-4);
 
     &:hover {
         .showhide {
@@ -192,11 +194,6 @@
             opacity: 1;
         }
     }
-    &:focus-within {
-        .showhide [aria-controls] {
-            opacity: 1;
-        }
-    }
 }
 
 :root:has(body[data-platform-name="windows"]) {

special-pages/pages/new-tab/app/styles/ntp-theme.css

@@ -1,8 +1,8 @@
 :root {
-    --ntp-background-color: white;
-    --ntp-surface-background-color: white;
+    --ntp-background-color: var(--color-gray-0);
+    --ntp-surface-background-color: var(--color-white-at-30);
     --ntp-surfaces-panel-background-color: white;
-    --ntp-surface-border-color: var(--color-black-at-6);
+    --ntp-surface-border-color: var(--color-black-at-9);
     --ntp-text-normal: var(--color-black-at-84);
     --ntp-text-muted: var(--color-black-at-60);
     --ntp-text-on-primary: var(--color-white-at-84);
@@ -36,9 +36,9 @@
 
     @media (prefers-color-scheme: dark) {
         --ntp-background-color: var(--color-gray-85);
-        --ntp-surface-background-color: #2a2a2a;
+        --ntp-surface-background-color: var(--color-black-at-18);
         --ntp-surfaces-panel-background-color: #222222;
-        --ntp-surface-border-color: var(--color-white-at-6);
+        --ntp-surface-border-color: var(--color-white-at-9);
         --ntp-text-normal: var(--color-white-at-84);
         --ntp-text-muted: var(--color-white-at-60);
         --ntp-color-primary: var(--color-blue-30);