From 5a9524cd3fb474a5db7d884249d438874c218a58 Mon Sep 17 00:00:00 2001 From: Valerie Kraucunas Date: Mon, 16 Dec 2024 14:43:58 -0700 Subject: [PATCH] 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 --- special-pages/package.json | 1 + .../pages/new-tab/app/components/App.js | 2 +- .../app/components/ShowHide.module.css | 4 ++ .../new-tab/app/entry-points/nextSteps.js | 2 +- .../app/favorites/components/Favorites.js | 40 +++++++++++-------- .../favorites/components/Favorites.module.css | 5 --- .../integration-tests/favorites.page.js | 24 +++++++++-- .../integration-tests/favorites.spec.js | 31 ++++++++++++++ .../components/NextSteps.module.css | 9 ++--- .../pages/new-tab/app/styles/ntp-theme.css | 10 ++--- 10 files changed, 91 insertions(+), 37 deletions(-) diff --git a/special-pages/package.json b/special-pages/package.json index 5fa59308c..c414aabae 100644 --- a/special-pages/package.json +++ b/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", diff --git a/special-pages/pages/new-tab/app/components/App.js b/special-pages/pages/new-tab/app/components/App.js index fa41764f8..9237dc2f0 100644 --- a/special-pages/pages/new-tab/app/components/App.js +++ b/special-pages/pages/new-tab/app/components/App.js @@ -28,7 +28,7 @@ export function App({ children }) { return (
-
+
diff --git a/special-pages/pages/new-tab/app/components/ShowHide.module.css b/special-pages/pages/new-tab/app/components/ShowHide.module.css index cd4a74908..ba748154e 100644 --- a/special-pages/pages/new-tab/app/components/ShowHide.module.css +++ b/special-pages/pages/new-tab/app/components/ShowHide.module.css @@ -56,6 +56,10 @@ } } + > * { + pointer-events: none; + } + svg { transition: transform .3s; } diff --git a/special-pages/pages/new-tab/app/entry-points/nextSteps.js b/special-pages/pages/new-tab/app/entry-points/nextSteps.js index 939e7a3a4..0557f7821 100644 --- a/special-pages/pages/new-tab/app/entry-points/nextSteps.js +++ b/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 ( - + ); diff --git a/special-pages/pages/new-tab/app/favorites/components/Favorites.js b/special-pages/pages/new-tab/app/favorites/components/Favorites.js index 66eab567b..98017554e 100644 --- a/special-pages/pages/new-tab/app/favorites/components/Favorites.js +++ b/special-pages/pages/new-tab/app/favorites/components/Favorites.js @@ -179,26 +179,32 @@ 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); @@ -206,27 +212,29 @@ function Inner({ rows, safeAreaRef, rowHeight, add }) { } 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 }, ); diff --git a/special-pages/pages/new-tab/app/favorites/components/Favorites.module.css b/special-pages/pages/new-tab/app/favorites/components/Favorites.module.css index 343f9433d..5ddbb509a 100644 --- a/special-pages/pages/new-tab/app/favorites/components/Favorites.module.css +++ b/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 { diff --git a/special-pages/pages/new-tab/app/favorites/integration-tests/favorites.page.js b/special-pages/pages/new-tab/app/favorites/integration-tests/favorites.page.js index e9e28b549..9e6e22372 100644 --- a/special-pages/pages/new-tab/app/favorites/integration-tests/favorites.page.js +++ b/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); + } } diff --git a/special-pages/pages/new-tab/app/favorites/integration-tests/favorites.spec.js b/special-pages/pages/new-tab/app/favorites/integration-tests/favorites.spec.js index 27bf64e5b..f475819eb 100644 --- a/special-pages/pages/new-tab/app/favorites/integration-tests/favorites.spec.js +++ b/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); + }); }); diff --git a/special-pages/pages/new-tab/app/next-steps/components/NextSteps.module.css b/special-pages/pages/new-tab/app/next-steps/components/NextSteps.module.css index dfbe3d23d..4b4e52424 100644 --- a/special-pages/pages/new-tab/app/next-steps/components/NextSteps.module.css +++ b/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"]) { diff --git a/special-pages/pages/new-tab/app/styles/ntp-theme.css b/special-pages/pages/new-tab/app/styles/ntp-theme.css index 8be2d183c..9714e3561 100644 --- a/special-pages/pages/new-tab/app/styles/ntp-theme.css +++ b/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);