Merged
Conversation
philschatz
commented
Dec 5, 2018
| }); | ||
|
|
||
| it('reports about the test content page', async() => { | ||
| await checkLighthouse(TEST_PAGE, 0.95); |
Member
Author
There was a problem hiding this comment.
I would like to see what the report generated & would like to try to get an HTML report out (similar to tutor/scripts/accessibility-audit/index.js ) but maybe running the audit in the browser is sufficient for now. Thoughts?
Member
There was a problem hiding this comment.
yea i don't think we have to worry about the html reports for now, we might generate those separately later but it wouldn't be part of the unit test suite
TomWoodward
previously approved these changes
Dec 6, 2018
TomWoodward
approved these changes
Dec 6, 2018
OpenStaxClaude
added a commit
that referenced
this pull request
Apr 6, 2026
The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 6, 2026
The page.evaluate function was not executing properly because: 1. Arguments were passed as an object requiring destructuring, which doesn't serialize well in Puppeteer 2. Optional chaining (document?., window?.) was unnecessary in browser context Changed to: - Pass arguments as separate parameters directly to page.evaluate - Remove optional chaining operators (document, window are guaranteed to exist) - Add explicit type annotations to function signature This should allow the evaluate callback to execute properly and return expected scroll positions. Addresses Review #37 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Update Content.browserspec.tsx Update utils.ts Update Content.browserspec.tsx Update Content.browserspec.tsx Update Content.browserspec.tsx Fix window reference in browser test (Review #38) The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 6, 2026
The page.evaluate function was not executing properly because: 1. Arguments were passed as an object requiring destructuring, which doesn't serialize well in Puppeteer 2. Optional chaining (document?., window?.) was unnecessary in browser context Changed to: - Pass arguments as separate parameters directly to page.evaluate - Remove optional chaining operators (document, window are guaranteed to exist) - Add explicit type annotations to function signature This should allow the evaluate callback to execute properly and return expected scroll positions. Addresses Review #37 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Update Content.browserspec.tsx Update utils.ts Update Content.browserspec.tsx Update Content.browserspec.tsx Update Content.browserspec.tsx Fix window reference in browser test (Review #38) The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 7, 2026
Use dynamic scroll position calculations in browser tests Replace hardcoded EXPECTED_SCROLL_TOPS values with dynamic position calculations. The test now: 1. Calculates expected scroll positions based on actual element positions 2. Accounts for sticky header (topbar) height dynamically 3. Eliminates brittleness from hardcoded pixel values that break when CSS changes This approach is more robust and adapts automatically to layout changes from the CSS Modules migration, addressing Reviews #23 and #24. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Allow for undefined window, document Fix scroll position calculation to account for book banner The dynamic scroll position calculation was only accounting for the topbar height, but not the book banner that's also sticky above it. The topbar is positioned at top: [bookBannerHeight]rem, so the total sticky offset is banner height + topbar height. This fix calculates the book banner height from the topbar's computed 'top' value and adds it to the topbar height for accurate scroll position validation. Fixes the ~90-100px discrepancy in Content.browserspec.tsx scroll tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Fix scroll position calculation to use scroll-padding CSS property The browser test was failing with a 30px discrepancy because the expected scroll position calculation was manually computing the sticky header offset instead of using the actual scroll-padding CSS property that the browser uses for anchor navigation. Changed getExpectedScrollPosition() to read the computed scroll-padding-top value from the body element, which already accounts for the complete sticky header stack (book banner + topbar). This matches exactly how the browser positions elements when navigating to hash anchors. Fixes Review #28 - scroll position should now be within 10px threshold 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix TypeScript error: use getPropertyValue for scroll-padding-top The scrollPaddingTop property doesn't exist on CSSStyleDeclaration type. Use getPropertyValue() method to access the CSS property value instead. Addresses Review #30 🤖 Generated with [Claude Code](https://claude.com/claude-code) Revert to manual sticky offset calculation for browser test The scroll-padding approach introduced in commit 9cc0492 made the test worse (90px and 66px discrepancies vs the previous 30px discrepancy). Reverting to the more accurate manual calculation from commit 9324f0c which explicitly calculates: - Topbar height from getBoundingClientRect() - Book banner height from topbar's computed top value - Total sticky offset = banner height + topbar height This approach directly measures the actual rendered heights rather than relying on CSS scroll-padding which may not account for all layout nuances. Addresses Review #31 feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Simplify page.evaluate() callback to fix execution issue Removed Window type casting and unnecessary complexity that was preventing the page.evaluate() callback from executing properly. The function now: - Uses window and document globals directly (no casting) - Removes type annotations from the callback parameters - Explicitly awaits and logs the result - Adds debug logging inside page.evaluate() to verify execution This should fix the issue where expectedScrollTop was always returning 0 because the callback wasn't being executed. Addresses Review #40 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix page.evaluate serialization in browser test The page.evaluate function was not executing properly because: 1. Arguments were passed as an object requiring destructuring, which doesn't serialize well in Puppeteer 2. Optional chaining (document?., window?.) was unnecessary in browser context Changed to: - Pass arguments as separate parameters directly to page.evaluate - Remove optional chaining operators (document, window are guaranteed to exist) - Add explicit type annotations to function signature This should allow the evaluate callback to execute properly and return expected scroll positions. Addresses Review #37 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Update Content.browserspec.tsx Update utils.ts Update Content.browserspec.tsx Update Content.browserspec.tsx Update Content.browserspec.tsx Fix window reference in browser test (Review #38) The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add debugging to investigate null element in scroll position test Per Review #36, element is always null in page.evaluate. Added logging to: - Show the selector being passed - List available element IDs in the document when element is null - Help identify if the target elements exist or if selector format is wrong This will help diagnose why querySelector returns null for the target elements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add feedback Add delay after clicking links to allow ScrollOffset adjustment Wait 500ms after clicking anchor links to ensure the ScrollOffset component's manual scroll adjustment (via scrollBy) has completed before checking the final scroll position. The ScrollOffset component applies an additional scroll adjustment after the browser's native hash navigation, which may take some time to complete. This delay should help the test wait for the final scroll position to stabilize. Addresses Review #34 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix browser test scroll position calculation for link clicks The ScrollOffset component applies a double-offset for clicked links as a workaround for browsers that don't support scroll-padding properly: 1. Browser scrolls to elementTop - scrollPadding (native behavior) 2. ScrollOffset.clickHandler applies scrollBy(0, -scrollPadding) (manual adjustment) This results in final position: elementTop - 2*scrollPadding Updated getExpectedScrollPosition() to account for this double-offset when isClickNavigation=true. Initial hash navigation (line 93) uses single offset, while link clicks (line 111) use double offset. Addresses Review #33 browser test failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix scroll position calculation in browser test Use the actual computed scroll-padding-top CSS property instead of manually calculating sticky header offsets. The ScrollOffset component sets CSS variables --scroll-offset-desktop and --scroll-offset-mobile which are applied via the scroll-padding CSS property on the body element. This is the exact value the browser uses for hash navigation. The manual calculation was missing some offset nuances, resulting in test failures with 90px (desktop) and 66px (mobile) discrepancies. By reading the computed scroll-padding-top value, we now match exactly what the browser does. Addresses review comment #32 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 8, 2026
Use dynamic scroll position calculations in browser tests Replace hardcoded EXPECTED_SCROLL_TOPS values with dynamic position calculations. The test now: 1. Calculates expected scroll positions based on actual element positions 2. Accounts for sticky header (topbar) height dynamically 3. Eliminates brittleness from hardcoded pixel values that break when CSS changes This approach is more robust and adapts automatically to layout changes from the CSS Modules migration, addressing Reviews #23 and #24. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Allow for undefined window, document Fix scroll position calculation to account for book banner The dynamic scroll position calculation was only accounting for the topbar height, but not the book banner that's also sticky above it. The topbar is positioned at top: [bookBannerHeight]rem, so the total sticky offset is banner height + topbar height. This fix calculates the book banner height from the topbar's computed 'top' value and adds it to the topbar height for accurate scroll position validation. Fixes the ~90-100px discrepancy in Content.browserspec.tsx scroll tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Fix scroll position calculation to use scroll-padding CSS property The browser test was failing with a 30px discrepancy because the expected scroll position calculation was manually computing the sticky header offset instead of using the actual scroll-padding CSS property that the browser uses for anchor navigation. Changed getExpectedScrollPosition() to read the computed scroll-padding-top value from the body element, which already accounts for the complete sticky header stack (book banner + topbar). This matches exactly how the browser positions elements when navigating to hash anchors. Fixes Review #28 - scroll position should now be within 10px threshold 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix TypeScript error: use getPropertyValue for scroll-padding-top The scrollPaddingTop property doesn't exist on CSSStyleDeclaration type. Use getPropertyValue() method to access the CSS property value instead. Addresses Review #30 🤖 Generated with [Claude Code](https://claude.com/claude-code) Revert to manual sticky offset calculation for browser test The scroll-padding approach introduced in commit 9cc0492 made the test worse (90px and 66px discrepancies vs the previous 30px discrepancy). Reverting to the more accurate manual calculation from commit 9324f0c which explicitly calculates: - Topbar height from getBoundingClientRect() - Book banner height from topbar's computed top value - Total sticky offset = banner height + topbar height This approach directly measures the actual rendered heights rather than relying on CSS scroll-padding which may not account for all layout nuances. Addresses Review #31 feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Simplify page.evaluate() callback to fix execution issue Removed Window type casting and unnecessary complexity that was preventing the page.evaluate() callback from executing properly. The function now: - Uses window and document globals directly (no casting) - Removes type annotations from the callback parameters - Explicitly awaits and logs the result - Adds debug logging inside page.evaluate() to verify execution This should fix the issue where expectedScrollTop was always returning 0 because the callback wasn't being executed. Addresses Review #40 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix page.evaluate serialization in browser test The page.evaluate function was not executing properly because: 1. Arguments were passed as an object requiring destructuring, which doesn't serialize well in Puppeteer 2. Optional chaining (document?., window?.) was unnecessary in browser context Changed to: - Pass arguments as separate parameters directly to page.evaluate - Remove optional chaining operators (document, window are guaranteed to exist) - Add explicit type annotations to function signature This should allow the evaluate callback to execute properly and return expected scroll positions. Addresses Review #37 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Update Content.browserspec.tsx Update utils.ts Update Content.browserspec.tsx Update Content.browserspec.tsx Update Content.browserspec.tsx Fix window reference in browser test (Review #38) The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add debugging to investigate null element in scroll position test Per Review #36, element is always null in page.evaluate. Added logging to: - Show the selector being passed - List available element IDs in the document when element is null - Help identify if the target elements exist or if selector format is wrong This will help diagnose why querySelector returns null for the target elements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add feedback Add delay after clicking links to allow ScrollOffset adjustment Wait 500ms after clicking anchor links to ensure the ScrollOffset component's manual scroll adjustment (via scrollBy) has completed before checking the final scroll position. The ScrollOffset component applies an additional scroll adjustment after the browser's native hash navigation, which may take some time to complete. This delay should help the test wait for the final scroll position to stabilize. Addresses Review #34 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix browser test scroll position calculation for link clicks The ScrollOffset component applies a double-offset for clicked links as a workaround for browsers that don't support scroll-padding properly: 1. Browser scrolls to elementTop - scrollPadding (native behavior) 2. ScrollOffset.clickHandler applies scrollBy(0, -scrollPadding) (manual adjustment) This results in final position: elementTop - 2*scrollPadding Updated getExpectedScrollPosition() to account for this double-offset when isClickNavigation=true. Initial hash navigation (line 93) uses single offset, while link clicks (line 111) use double offset. Addresses Review #33 browser test failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix scroll position calculation in browser test Use the actual computed scroll-padding-top CSS property instead of manually calculating sticky header offsets. The ScrollOffset component sets CSS variables --scroll-offset-desktop and --scroll-offset-mobile which are applied via the scroll-padding CSS property on the body element. This is the exact value the browser uses for hash navigation. The manual calculation was missing some offset nuances, resulting in test failures with 90px (desktop) and 66px (mobile) discrepancies. By reading the computed scroll-padding-top value, we now match exactly what the browser does. Addresses review comment #32 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 9, 2026
Use dynamic scroll position calculations in browser tests Replace hardcoded EXPECTED_SCROLL_TOPS values with dynamic position calculations. The test now: 1. Calculates expected scroll positions based on actual element positions 2. Accounts for sticky header (topbar) height dynamically 3. Eliminates brittleness from hardcoded pixel values that break when CSS changes This approach is more robust and adapts automatically to layout changes from the CSS Modules migration, addressing Reviews #23 and #24. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Allow for undefined window, document Fix scroll position calculation to account for book banner The dynamic scroll position calculation was only accounting for the topbar height, but not the book banner that's also sticky above it. The topbar is positioned at top: [bookBannerHeight]rem, so the total sticky offset is banner height + topbar height. This fix calculates the book banner height from the topbar's computed 'top' value and adds it to the topbar height for accurate scroll position validation. Fixes the ~90-100px discrepancy in Content.browserspec.tsx scroll tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Fix scroll position calculation to use scroll-padding CSS property The browser test was failing with a 30px discrepancy because the expected scroll position calculation was manually computing the sticky header offset instead of using the actual scroll-padding CSS property that the browser uses for anchor navigation. Changed getExpectedScrollPosition() to read the computed scroll-padding-top value from the body element, which already accounts for the complete sticky header stack (book banner + topbar). This matches exactly how the browser positions elements when navigating to hash anchors. Fixes Review #28 - scroll position should now be within 10px threshold 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix TypeScript error: use getPropertyValue for scroll-padding-top The scrollPaddingTop property doesn't exist on CSSStyleDeclaration type. Use getPropertyValue() method to access the CSS property value instead. Addresses Review #30 🤖 Generated with [Claude Code](https://claude.com/claude-code) Revert to manual sticky offset calculation for browser test The scroll-padding approach introduced in commit 9cc0492 made the test worse (90px and 66px discrepancies vs the previous 30px discrepancy). Reverting to the more accurate manual calculation from commit 9324f0c which explicitly calculates: - Topbar height from getBoundingClientRect() - Book banner height from topbar's computed top value - Total sticky offset = banner height + topbar height This approach directly measures the actual rendered heights rather than relying on CSS scroll-padding which may not account for all layout nuances. Addresses Review #31 feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Simplify page.evaluate() callback to fix execution issue Removed Window type casting and unnecessary complexity that was preventing the page.evaluate() callback from executing properly. The function now: - Uses window and document globals directly (no casting) - Removes type annotations from the callback parameters - Explicitly awaits and logs the result - Adds debug logging inside page.evaluate() to verify execution This should fix the issue where expectedScrollTop was always returning 0 because the callback wasn't being executed. Addresses Review #40 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix page.evaluate serialization in browser test The page.evaluate function was not executing properly because: 1. Arguments were passed as an object requiring destructuring, which doesn't serialize well in Puppeteer 2. Optional chaining (document?., window?.) was unnecessary in browser context Changed to: - Pass arguments as separate parameters directly to page.evaluate - Remove optional chaining operators (document, window are guaranteed to exist) - Add explicit type annotations to function signature This should allow the evaluate callback to execute properly and return expected scroll positions. Addresses Review #37 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Update Content.browserspec.tsx Update utils.ts Update Content.browserspec.tsx Update Content.browserspec.tsx Update Content.browserspec.tsx Fix window reference in browser test (Review #38) The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add debugging to investigate null element in scroll position test Per Review #36, element is always null in page.evaluate. Added logging to: - Show the selector being passed - List available element IDs in the document when element is null - Help identify if the target elements exist or if selector format is wrong This will help diagnose why querySelector returns null for the target elements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add feedback Add delay after clicking links to allow ScrollOffset adjustment Wait 500ms after clicking anchor links to ensure the ScrollOffset component's manual scroll adjustment (via scrollBy) has completed before checking the final scroll position. The ScrollOffset component applies an additional scroll adjustment after the browser's native hash navigation, which may take some time to complete. This delay should help the test wait for the final scroll position to stabilize. Addresses Review #34 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix browser test scroll position calculation for link clicks The ScrollOffset component applies a double-offset for clicked links as a workaround for browsers that don't support scroll-padding properly: 1. Browser scrolls to elementTop - scrollPadding (native behavior) 2. ScrollOffset.clickHandler applies scrollBy(0, -scrollPadding) (manual adjustment) This results in final position: elementTop - 2*scrollPadding Updated getExpectedScrollPosition() to account for this double-offset when isClickNavigation=true. Initial hash navigation (line 93) uses single offset, while link clicks (line 111) use double offset. Addresses Review #33 browser test failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix scroll position calculation in browser test Use the actual computed scroll-padding-top CSS property instead of manually calculating sticky header offsets. The ScrollOffset component sets CSS variables --scroll-offset-desktop and --scroll-offset-mobile which are applied via the scroll-padding CSS property on the body element. This is the exact value the browser uses for hash navigation. The manual calculation was missing some offset nuances, resulting in test failures with 90px (desktop) and 66px (mobile) discrepancies. By reading the computed scroll-padding-top value, we now match exactly what the browser does. Addresses review comment #32 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 9, 2026
Use dynamic scroll position calculations in browser tests Replace hardcoded EXPECTED_SCROLL_TOPS values with dynamic position calculations. The test now: 1. Calculates expected scroll positions based on actual element positions 2. Accounts for sticky header (topbar) height dynamically 3. Eliminates brittleness from hardcoded pixel values that break when CSS changes This approach is more robust and adapts automatically to layout changes from the CSS Modules migration, addressing Reviews #23 and #24. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Allow for undefined window, document Fix scroll position calculation to account for book banner The dynamic scroll position calculation was only accounting for the topbar height, but not the book banner that's also sticky above it. The topbar is positioned at top: [bookBannerHeight]rem, so the total sticky offset is banner height + topbar height. This fix calculates the book banner height from the topbar's computed 'top' value and adds it to the topbar height for accurate scroll position validation. Fixes the ~90-100px discrepancy in Content.browserspec.tsx scroll tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Fix scroll position calculation to use scroll-padding CSS property The browser test was failing with a 30px discrepancy because the expected scroll position calculation was manually computing the sticky header offset instead of using the actual scroll-padding CSS property that the browser uses for anchor navigation. Changed getExpectedScrollPosition() to read the computed scroll-padding-top value from the body element, which already accounts for the complete sticky header stack (book banner + topbar). This matches exactly how the browser positions elements when navigating to hash anchors. Fixes Review #28 - scroll position should now be within 10px threshold 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix TypeScript error: use getPropertyValue for scroll-padding-top The scrollPaddingTop property doesn't exist on CSSStyleDeclaration type. Use getPropertyValue() method to access the CSS property value instead. Addresses Review #30 🤖 Generated with [Claude Code](https://claude.com/claude-code) Revert to manual sticky offset calculation for browser test The scroll-padding approach introduced in commit 9cc0492 made the test worse (90px and 66px discrepancies vs the previous 30px discrepancy). Reverting to the more accurate manual calculation from commit 9324f0c which explicitly calculates: - Topbar height from getBoundingClientRect() - Book banner height from topbar's computed top value - Total sticky offset = banner height + topbar height This approach directly measures the actual rendered heights rather than relying on CSS scroll-padding which may not account for all layout nuances. Addresses Review #31 feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Simplify page.evaluate() callback to fix execution issue Removed Window type casting and unnecessary complexity that was preventing the page.evaluate() callback from executing properly. The function now: - Uses window and document globals directly (no casting) - Removes type annotations from the callback parameters - Explicitly awaits and logs the result - Adds debug logging inside page.evaluate() to verify execution This should fix the issue where expectedScrollTop was always returning 0 because the callback wasn't being executed. Addresses Review #40 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix page.evaluate serialization in browser test The page.evaluate function was not executing properly because: 1. Arguments were passed as an object requiring destructuring, which doesn't serialize well in Puppeteer 2. Optional chaining (document?., window?.) was unnecessary in browser context Changed to: - Pass arguments as separate parameters directly to page.evaluate - Remove optional chaining operators (document, window are guaranteed to exist) - Add explicit type annotations to function signature This should allow the evaluate callback to execute properly and return expected scroll positions. Addresses Review #37 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Update Content.browserspec.tsx Update utils.ts Update Content.browserspec.tsx Update Content.browserspec.tsx Update Content.browserspec.tsx Fix window reference in browser test (Review #38) The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add debugging to investigate null element in scroll position test Per Review #36, element is always null in page.evaluate. Added logging to: - Show the selector being passed - List available element IDs in the document when element is null - Help identify if the target elements exist or if selector format is wrong This will help diagnose why querySelector returns null for the target elements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add feedback Add delay after clicking links to allow ScrollOffset adjustment Wait 500ms after clicking anchor links to ensure the ScrollOffset component's manual scroll adjustment (via scrollBy) has completed before checking the final scroll position. The ScrollOffset component applies an additional scroll adjustment after the browser's native hash navigation, which may take some time to complete. This delay should help the test wait for the final scroll position to stabilize. Addresses Review #34 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix browser test scroll position calculation for link clicks The ScrollOffset component applies a double-offset for clicked links as a workaround for browsers that don't support scroll-padding properly: 1. Browser scrolls to elementTop - scrollPadding (native behavior) 2. ScrollOffset.clickHandler applies scrollBy(0, -scrollPadding) (manual adjustment) This results in final position: elementTop - 2*scrollPadding Updated getExpectedScrollPosition() to account for this double-offset when isClickNavigation=true. Initial hash navigation (line 93) uses single offset, while link clicks (line 111) use double offset. Addresses Review #33 browser test failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix scroll position calculation in browser test Use the actual computed scroll-padding-top CSS property instead of manually calculating sticky header offsets. The ScrollOffset component sets CSS variables --scroll-offset-desktop and --scroll-offset-mobile which are applied via the scroll-padding CSS property on the body element. This is the exact value the browser uses for hash navigation. The manual calculation was missing some offset nuances, resulting in test failures with 90px (desktop) and 66px (mobile) discrepancies. By reading the computed scroll-padding-top value, we now match exactly what the browser does. Addresses review comment #32 🤖 Generated with [Claude Code](https://claude.com/claude-code) Debugging back in Add debugging to show all link hrefs before clicking Add debugging to investigate URL and selector formation Clean up debugging Add non-null assertions for TypeScript strict null checks in browser test TypeScript was complaining that document and window might be undefined within the page.evaluate() callback. However, within a browser context (which is where page.evaluate() runs), these globals are guaranteed to exist. Added non-null assertions (!) to satisfy TypeScript's strict null checking: - document.querySelector - document.querySelectorAll - document.documentElement - window.pageYOffset - window.getComputedStyle - window.matchMedia Addresses Review #41 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 9, 2026
Use dynamic scroll position calculations in browser tests Replace hardcoded EXPECTED_SCROLL_TOPS values with dynamic position calculations. The test now: 1. Calculates expected scroll positions based on actual element positions 2. Accounts for sticky header (topbar) height dynamically 3. Eliminates brittleness from hardcoded pixel values that break when CSS changes This approach is more robust and adapts automatically to layout changes from the CSS Modules migration, addressing Reviews #23 and #24. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Allow for undefined window, document Fix scroll position calculation to account for book banner The dynamic scroll position calculation was only accounting for the topbar height, but not the book banner that's also sticky above it. The topbar is positioned at top: [bookBannerHeight]rem, so the total sticky offset is banner height + topbar height. This fix calculates the book banner height from the topbar's computed 'top' value and adds it to the topbar height for accurate scroll position validation. Fixes the ~90-100px discrepancy in Content.browserspec.tsx scroll tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Fix scroll position calculation to use scroll-padding CSS property The browser test was failing with a 30px discrepancy because the expected scroll position calculation was manually computing the sticky header offset instead of using the actual scroll-padding CSS property that the browser uses for anchor navigation. Changed getExpectedScrollPosition() to read the computed scroll-padding-top value from the body element, which already accounts for the complete sticky header stack (book banner + topbar). This matches exactly how the browser positions elements when navigating to hash anchors. Fixes Review #28 - scroll position should now be within 10px threshold 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix TypeScript error: use getPropertyValue for scroll-padding-top The scrollPaddingTop property doesn't exist on CSSStyleDeclaration type. Use getPropertyValue() method to access the CSS property value instead. Addresses Review #30 🤖 Generated with [Claude Code](https://claude.com/claude-code) Revert to manual sticky offset calculation for browser test The scroll-padding approach introduced in commit 9cc0492 made the test worse (90px and 66px discrepancies vs the previous 30px discrepancy). Reverting to the more accurate manual calculation from commit 9324f0c which explicitly calculates: - Topbar height from getBoundingClientRect() - Book banner height from topbar's computed top value - Total sticky offset = banner height + topbar height This approach directly measures the actual rendered heights rather than relying on CSS scroll-padding which may not account for all layout nuances. Addresses Review #31 feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Simplify page.evaluate() callback to fix execution issue Removed Window type casting and unnecessary complexity that was preventing the page.evaluate() callback from executing properly. The function now: - Uses window and document globals directly (no casting) - Removes type annotations from the callback parameters - Explicitly awaits and logs the result - Adds debug logging inside page.evaluate() to verify execution This should fix the issue where expectedScrollTop was always returning 0 because the callback wasn't being executed. Addresses Review #40 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix page.evaluate serialization in browser test The page.evaluate function was not executing properly because: 1. Arguments were passed as an object requiring destructuring, which doesn't serialize well in Puppeteer 2. Optional chaining (document?., window?.) was unnecessary in browser context Changed to: - Pass arguments as separate parameters directly to page.evaluate - Remove optional chaining operators (document, window are guaranteed to exist) - Add explicit type annotations to function signature This should allow the evaluate callback to execute properly and return expected scroll positions. Addresses Review #37 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Update Content.browserspec.tsx Update utils.ts Update Content.browserspec.tsx Update Content.browserspec.tsx Update Content.browserspec.tsx Fix window reference in browser test (Review #38) The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add debugging to investigate null element in scroll position test Per Review #36, element is always null in page.evaluate. Added logging to: - Show the selector being passed - List available element IDs in the document when element is null - Help identify if the target elements exist or if selector format is wrong This will help diagnose why querySelector returns null for the target elements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add feedback Add delay after clicking links to allow ScrollOffset adjustment Wait 500ms after clicking anchor links to ensure the ScrollOffset component's manual scroll adjustment (via scrollBy) has completed before checking the final scroll position. The ScrollOffset component applies an additional scroll adjustment after the browser's native hash navigation, which may take some time to complete. This delay should help the test wait for the final scroll position to stabilize. Addresses Review #34 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix browser test scroll position calculation for link clicks The ScrollOffset component applies a double-offset for clicked links as a workaround for browsers that don't support scroll-padding properly: 1. Browser scrolls to elementTop - scrollPadding (native behavior) 2. ScrollOffset.clickHandler applies scrollBy(0, -scrollPadding) (manual adjustment) This results in final position: elementTop - 2*scrollPadding Updated getExpectedScrollPosition() to account for this double-offset when isClickNavigation=true. Initial hash navigation (line 93) uses single offset, while link clicks (line 111) use double offset. Addresses Review #33 browser test failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix scroll position calculation in browser test Use the actual computed scroll-padding-top CSS property instead of manually calculating sticky header offsets. The ScrollOffset component sets CSS variables --scroll-offset-desktop and --scroll-offset-mobile which are applied via the scroll-padding CSS property on the body element. This is the exact value the browser uses for hash navigation. The manual calculation was missing some offset nuances, resulting in test failures with 90px (desktop) and 66px (mobile) discrepancies. By reading the computed scroll-padding-top value, we now match exactly what the browser does. Addresses review comment #32 🤖 Generated with [Claude Code](https://claude.com/claude-code) Debugging back in Add debugging to show all link hrefs before clicking Add debugging to investigate URL and selector formation Clean up debugging Add non-null assertions for TypeScript strict null checks in browser test TypeScript was complaining that document and window might be undefined within the page.evaluate() callback. However, within a browser context (which is where page.evaluate() runs), these globals are guaranteed to exist. Added non-null assertions (!) to satisfy TypeScript's strict null checking: - document.querySelector - document.querySelectorAll - document.documentElement - window.pageYOffset - window.getComputedStyle - window.matchMedia Addresses Review #41 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 9, 2026
Use dynamic scroll position calculations in browser tests Replace hardcoded EXPECTED_SCROLL_TOPS values with dynamic position calculations. The test now: 1. Calculates expected scroll positions based on actual element positions 2. Accounts for sticky header (topbar) height dynamically 3. Eliminates brittleness from hardcoded pixel values that break when CSS changes This approach is more robust and adapts automatically to layout changes from the CSS Modules migration, addressing Reviews #23 and #24. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Allow for undefined window, document Fix scroll position calculation to account for book banner The dynamic scroll position calculation was only accounting for the topbar height, but not the book banner that's also sticky above it. The topbar is positioned at top: [bookBannerHeight]rem, so the total sticky offset is banner height + topbar height. This fix calculates the book banner height from the topbar's computed 'top' value and adds it to the topbar height for accurate scroll position validation. Fixes the ~90-100px discrepancy in Content.browserspec.tsx scroll tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Fix scroll position calculation to use scroll-padding CSS property The browser test was failing with a 30px discrepancy because the expected scroll position calculation was manually computing the sticky header offset instead of using the actual scroll-padding CSS property that the browser uses for anchor navigation. Changed getExpectedScrollPosition() to read the computed scroll-padding-top value from the body element, which already accounts for the complete sticky header stack (book banner + topbar). This matches exactly how the browser positions elements when navigating to hash anchors. Fixes Review #28 - scroll position should now be within 10px threshold 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix TypeScript error: use getPropertyValue for scroll-padding-top The scrollPaddingTop property doesn't exist on CSSStyleDeclaration type. Use getPropertyValue() method to access the CSS property value instead. Addresses Review #30 🤖 Generated with [Claude Code](https://claude.com/claude-code) Revert to manual sticky offset calculation for browser test The scroll-padding approach introduced in commit 9cc0492 made the test worse (90px and 66px discrepancies vs the previous 30px discrepancy). Reverting to the more accurate manual calculation from commit 9324f0c which explicitly calculates: - Topbar height from getBoundingClientRect() - Book banner height from topbar's computed top value - Total sticky offset = banner height + topbar height This approach directly measures the actual rendered heights rather than relying on CSS scroll-padding which may not account for all layout nuances. Addresses Review #31 feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Simplify page.evaluate() callback to fix execution issue Removed Window type casting and unnecessary complexity that was preventing the page.evaluate() callback from executing properly. The function now: - Uses window and document globals directly (no casting) - Removes type annotations from the callback parameters - Explicitly awaits and logs the result - Adds debug logging inside page.evaluate() to verify execution This should fix the issue where expectedScrollTop was always returning 0 because the callback wasn't being executed. Addresses Review #40 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix page.evaluate serialization in browser test The page.evaluate function was not executing properly because: 1. Arguments were passed as an object requiring destructuring, which doesn't serialize well in Puppeteer 2. Optional chaining (document?., window?.) was unnecessary in browser context Changed to: - Pass arguments as separate parameters directly to page.evaluate - Remove optional chaining operators (document, window are guaranteed to exist) - Add explicit type annotations to function signature This should allow the evaluate callback to execute properly and return expected scroll positions. Addresses Review #37 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Update Content.browserspec.tsx Update utils.ts Update Content.browserspec.tsx Update Content.browserspec.tsx Update Content.browserspec.tsx Fix window reference in browser test (Review #38) The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add debugging to investigate null element in scroll position test Per Review #36, element is always null in page.evaluate. Added logging to: - Show the selector being passed - List available element IDs in the document when element is null - Help identify if the target elements exist or if selector format is wrong This will help diagnose why querySelector returns null for the target elements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add feedback Add delay after clicking links to allow ScrollOffset adjustment Wait 500ms after clicking anchor links to ensure the ScrollOffset component's manual scroll adjustment (via scrollBy) has completed before checking the final scroll position. The ScrollOffset component applies an additional scroll adjustment after the browser's native hash navigation, which may take some time to complete. This delay should help the test wait for the final scroll position to stabilize. Addresses Review #34 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix browser test scroll position calculation for link clicks The ScrollOffset component applies a double-offset for clicked links as a workaround for browsers that don't support scroll-padding properly: 1. Browser scrolls to elementTop - scrollPadding (native behavior) 2. ScrollOffset.clickHandler applies scrollBy(0, -scrollPadding) (manual adjustment) This results in final position: elementTop - 2*scrollPadding Updated getExpectedScrollPosition() to account for this double-offset when isClickNavigation=true. Initial hash navigation (line 93) uses single offset, while link clicks (line 111) use double offset. Addresses Review #33 browser test failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix scroll position calculation in browser test Use the actual computed scroll-padding-top CSS property instead of manually calculating sticky header offsets. The ScrollOffset component sets CSS variables --scroll-offset-desktop and --scroll-offset-mobile which are applied via the scroll-padding CSS property on the body element. This is the exact value the browser uses for hash navigation. The manual calculation was missing some offset nuances, resulting in test failures with 90px (desktop) and 66px (mobile) discrepancies. By reading the computed scroll-padding-top value, we now match exactly what the browser does. Addresses review comment #32 🤖 Generated with [Claude Code](https://claude.com/claude-code) Debugging back in Add debugging to show all link hrefs before clicking Add debugging to investigate URL and selector formation Clean up debugging Add non-null assertions for TypeScript strict null checks in browser test TypeScript was complaining that document and window might be undefined within the page.evaluate() callback. However, within a browser context (which is where page.evaluate() runs), these globals are guaranteed to exist. Added non-null assertions (!) to satisfy TypeScript's strict null checking: - document.querySelector - document.querySelectorAll - document.documentElement - window.pageYOffset - window.getComputedStyle - window.matchMedia Addresses Review #41 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Performs an audit of
/andTEST_PAGEusing https://github.com/GoogleChrome/lighthouse./has a score of 100% for accessibilityTEST_PAGEhas a score of 100% for accessibility