Skip to content

Commit 84db777

Browse files
authored
Merge pull request #44981 from github/repo-sync
Repo sync
2 parents b587ce0 + cc560f5 commit 84db777

22 files changed

Lines changed: 649 additions & 135 deletions

File tree

src/color-schemes/README.md

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,15 @@ Primer React uses slightly different terminology than the underlying CSS or the
3535
- CSS `light` -> Component `day`
3636
- CSS `dark` -> Component `night`
3737

38+
### Pre-paint Inline Script
39+
40+
`useTheme` only runs after the React bundle hydrates, so the page would first paint with the SSR default theme and then switch, causing a visible flash. To avoid that, `src/color-schemes/lib/color-mode-script.ts` exports `colorModeScript`: a small synchronous script that `_document.tsx` inlines in the `<head>`. It runs before the first paint, reads the `color_mode` cookie, and sets `data-color-mode`, `data-light-theme`, and `data-dark-theme` on `<html>`.
41+
42+
Key properties:
43+
- **Cache-safe**: The script is identical for every request, so the HTML stays shared-cacheable in the CDN. The theme is never server-rendered from the cookie (that would vary per user and poison the cache).
44+
- **No drift**: Its validation allowlists and defaults are derived from the same `CssColorMode`, `SupportedTheme`, and `defaultCSSTheme` exports used by `useTheme`. A test in `tests/color-mode-script.ts` runs the script against a fake `document` and asserts parity with `getCssTheme`.
45+
- **CSP**: Because the script is inline, `src/frame/middleware/helmet.ts` adds its `sha256` hash to the `script-src` directive. The hash is computed from the exact script string at startup, so it never needs manual maintenance, and a hash (not a nonce) keeps the response cacheable.
46+
3847
## Setup & Usage
3948

4049
To access the current theme in a component:
@@ -66,7 +75,8 @@ This hook is primarily used at the root of the application (e.g., in `src/frame/
6675

6776
## Current State & Known Issues
6877

69-
- **Hydration Mismatch / Flash of Unstyled Content**: Since the theme is read from a cookie on the client side (in `useEffect`), there can be a brief moment where the default theme is applied before the user's preference loads.
78+
- **Page background flash (fixed)**: The page-level theme (the `<html>` `data-*` attributes that drive the background color) is now set before first paint by the inline `colorModeScript`, so there is no longer a light-to-dark flash of the page background on load.
79+
- **Primer component theming**: Primer React components still resolve their theme from the post-hydration `useTheme` state, so component-level theming applies slightly after the page background. The `setTimeout` workaround below is still required for that path.
7080
- **Race Condition Workaround**: There is a `setTimeout` hack in `useTheme.ts` to delay the theme application. This is necessary to prevent Primer React's internal logic from overriding the user's preference with `auto` on initial load.
7181
- *Reference*: [Primer React Issue #2229](https://github.com/primer/react/issues/2229)
7282
- **Future**: The long-term goal is to rely entirely on CSS variables, removing the need for complex JavaScript state management for theming.

src/color-schemes/components/useTheme.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { useState, useEffect } from 'react'
22
import Cookies from '../../frame/components/lib/cookies'
33
import { COLOR_MODE_COOKIE_NAME } from '@/frame/lib/constants'
44

5-
enum CssColorMode {
5+
export enum CssColorMode {
66
auto = 'auto',
77
light = 'light',
88
dark = 'dark',
@@ -14,7 +14,7 @@ enum ComponentColorMode {
1414
night = 'night',
1515
}
1616

17-
enum SupportedTheme {
17+
export enum SupportedTheme {
1818
light = 'light',
1919
dark = 'dark',
2020
dark_dimmed = 'dark_dimmed',
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
import { COLOR_MODE_COOKIE_NAME } from '@/frame/lib/constants'
2+
import { CssColorMode, SupportedTheme, defaultCSSTheme } from '@/color-schemes/components/useTheme'
3+
4+
// A tiny script that runs synchronously in the document <head>, before the
5+
// browser's first paint. It reads the `color_mode` cookie (set by github.com,
6+
// not HttpOnly) and writes the matching `data-color-mode`, `data-light-theme`,
7+
// and `data-dark-theme` attributes onto the <html> element. Without this, the
8+
// page first paints with the SSR default theme and only switches to the user's
9+
// real theme after the React bundle hydrates, causing a visible flash.
10+
//
11+
// The output is identical for every request, so the HTML stays shared-cacheable
12+
// in our CDN. The validation allowlists and defaults are derived from the same
13+
// enums used by `useTheme`, so they can't drift, and `helmet.ts` hashes this
14+
// exact string for the CSP `script-src` allowance (no nonce, no unsafe-inline).
15+
const modes = JSON.stringify(Object.values(CssColorMode))
16+
const themes = JSON.stringify(Object.values(SupportedTheme))
17+
const defaults = JSON.stringify(defaultCSSTheme)
18+
const cookieName = JSON.stringify(COLOR_MODE_COOKIE_NAME)
19+
20+
export const colorModeScript = `(function(){
21+
var MODES=${modes},THEMES=${themes},D=${defaults};
22+
var css=D;
23+
try{
24+
var m=document.cookie.match(new RegExp('(?:^|; )'+${cookieName}+'=([^;]*)'));
25+
if(m){
26+
var p=JSON.parse(decodeURIComponent(m[1]));
27+
var fMode=function(x){return MODES.indexOf(x)>-1?x:null;};
28+
var fTheme=function(t){if(!t)return null;if(THEMES.indexOf(t.name)>-1)return t.name;if(THEMES.indexOf(t.color_mode)>-1)return t.color_mode;return null;};
29+
css={colorMode:fMode(p.color_mode)||D.colorMode,lightTheme:fTheme(p.light_theme)||D.lightTheme,darkTheme:fTheme(p.dark_theme)||D.darkTheme};
30+
}
31+
}catch(e){}
32+
try{
33+
var h=document.documentElement;
34+
h.setAttribute('data-color-mode',css.colorMode);
35+
h.setAttribute('data-light-theme',css.lightTheme);
36+
h.setAttribute('data-dark-theme',css.darkTheme);
37+
}catch(e){}
38+
})();`
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
import { describe, expect, test } from 'vitest'
2+
3+
import { colorModeScript } from '../lib/color-mode-script'
4+
import { getCssTheme } from '../components/useTheme'
5+
6+
// The inline script can't import the React `useTheme` module at runtime (it
7+
// runs before any bundle loads), so it reimplements the same validation. These
8+
// tests run the script against a fake `document` and assert it produces the
9+
// exact same result as `getCssTheme`, which keeps the two in sync.
10+
function runScript(rawCookie: string) {
11+
const attrs: Record<string, string> = {}
12+
const fakeDocument = {
13+
cookie: rawCookie,
14+
documentElement: {
15+
setAttribute(name: string, value: string) {
16+
attrs[name] = value
17+
},
18+
},
19+
}
20+
new Function('document', colorModeScript)(fakeDocument)
21+
return attrs
22+
}
23+
24+
function cookieFor(value: object) {
25+
// The real cookie value is URL-encoded JSON, like the browser stores it.
26+
return `color_mode=${encodeURIComponent(JSON.stringify(value))}`
27+
}
28+
29+
function expectMatchesGetCssTheme(rawCookie: string, cookieValue: string) {
30+
const css = getCssTheme(cookieValue)
31+
expect(runScript(rawCookie)).toEqual({
32+
'data-color-mode': css.colorMode,
33+
'data-light-theme': css.lightTheme,
34+
'data-dark-theme': css.darkTheme,
35+
})
36+
}
37+
38+
describe('colorModeScript', () => {
39+
test('falls back to defaults when no cookie is set', () => {
40+
expectMatchesGetCssTheme('', '')
41+
})
42+
43+
test('falls back to defaults on junk cookie values', () => {
44+
expectMatchesGetCssTheme('color_mode=not-valid-json', '')
45+
})
46+
47+
test('respects a valid color_mode cookie', () => {
48+
const value = {
49+
color_mode: 'dark',
50+
light_theme: { name: 'light_colorblind', color_mode: 'light' },
51+
dark_theme: { name: 'dark_tritanopia', color_mode: 'dark' },
52+
}
53+
expectMatchesGetCssTheme(cookieFor(value), JSON.stringify(value))
54+
})
55+
56+
test('honors supported named themes', () => {
57+
const value = {
58+
color_mode: 'auto',
59+
light_theme: { name: 'light', color_mode: 'light' },
60+
dark_theme: { name: 'dark_dimmed', color_mode: 'dark' },
61+
}
62+
expectMatchesGetCssTheme(cookieFor(value), JSON.stringify(value))
63+
})
64+
65+
test('ignores unknown modes and themes', () => {
66+
const value = {
67+
color_mode: 'sepia',
68+
light_theme: { name: 'rainbow', color_mode: 'rainbow' },
69+
dark_theme: { name: 'midnight', color_mode: 'midnight' },
70+
}
71+
expectMatchesGetCssTheme(cookieFor(value), JSON.stringify(value))
72+
})
73+
74+
test('reads the cookie even when other cookies are present', () => {
75+
const value = { color_mode: 'light' }
76+
const rawCookie = `_octo=GH1.1; color_mode=${encodeURIComponent(
77+
JSON.stringify(value),
78+
)}; logged_in=no`
79+
expectMatchesGetCssTheme(rawCookie, JSON.stringify(value))
80+
})
81+
})

src/content-render/unified/wrap-procedural-images.ts

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -32,21 +32,28 @@ function insideOlLi(ancestors: Parent[]): boolean {
3232
}
3333

3434
function visitor(node: Element, ancestors: Parent[]): void {
35-
if (insideOlLi(ancestors)) {
36-
const shallowClone: Element = Object.assign({}, node)
37-
shallowClone.tagName = 'div'
38-
shallowClone.properties = { class: 'procedural-image-wrapper' }
39-
shallowClone.children = [node]
40-
const parent = ancestors.at(-1)
41-
if (parent && parent.children) {
42-
parent.children = parent.children.map((child) => {
43-
if (child.type === 'element' && (child as Element).tagName === 'img') {
44-
return shallowClone
45-
}
46-
return child
47-
})
35+
if (!insideOlLi(ancestors)) return
36+
const parent = ancestors.at(-1)
37+
if (!parent || !parent.children) return
38+
39+
// When the image is already inside a <p> (the writer left a blank line before
40+
// it), the paragraph already provides spacing. Wrapping a <div> inside that
41+
// <p> produces invalid HTML (`<div>` cannot be a descendant of `<p>`), which
42+
// the browser silently repairs for dangerouslySetInnerHTML but causes a React
43+
// hydration mismatch when the content is rendered as real elements. So only
44+
// add the wrapper for the no-<p> (tight list) case it was designed for.
45+
if ((parent as Element).tagName === 'p') return
46+
47+
const shallowClone: Element = Object.assign({}, node)
48+
shallowClone.tagName = 'div'
49+
shallowClone.properties = { class: 'procedural-image-wrapper' }
50+
shallowClone.children = [node]
51+
parent.children = parent.children.map((child) => {
52+
if (child.type === 'element' && (child as Element).tagName === 'img') {
53+
return shallowClone
4854
}
49-
}
55+
return child
56+
})
5057
}
5158

5259
export default function wrapProceduralImages() {

src/fixtures/tests/images.ts

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,19 @@
11
import { describe, expect, test } from 'vitest'
22
import sharp from 'sharp'
3-
import type { CheerioAPI } from 'cheerio'
3+
import type { Cheerio, CheerioAPI } from 'cheerio'
4+
import type { Element } from 'domhandler'
45

56
import { get, head, getDOM } from '@/tests/helpers/e2etest'
67
import { MAX_WIDTH } from '@/content-render/unified/rewrite-asset-img-tags'
78

9+
// `getDOM` parses with `xmlMode: true`, which is case-sensitive on attribute
10+
// names. The legacy string render path emits a lowercase `srcset`, but the
11+
// React render path (hast -> JSX) emits React 19's camelCase `srcSet`. Both are
12+
// valid HTML (attribute names are case-insensitive in browsers), so read either.
13+
function srcsetOf(el: Cheerio<Element>): string | undefined {
14+
return el.attr('srcset') ?? el.attr('srcSet')
15+
}
16+
817
describe('render Markdown image tags', () => {
918
test('page with a single image', async () => {
1019
const $: CheerioAPI = await getDOM('/get-started/images/single-image')
@@ -14,7 +23,7 @@ describe('render Markdown image tags', () => {
1423

1524
const sources = $('source', pictures)
1625
expect(sources.length).toBe(1)
17-
const srcset = sources.attr('srcset')
26+
const srcset = srcsetOf(sources)
1827
expect(srcset).toMatch(
1928
new RegExp(`^/assets/cb-\\w+/mw-${MAX_WIDTH}/images/_fixtures/screenshot\\.webp 2x$`),
2029
)
@@ -54,9 +63,9 @@ describe('render Markdown image tags', () => {
5463
const sources = $('source', pictures)
5564
expect(sources.length).toBe(3)
5665

57-
expect(sources.eq(0).attr('srcset')).toContain('1x') // 0
58-
expect(sources.eq(1).attr('srcset')).toContain('2x') // 1
59-
expect(sources.eq(2).attr('srcset')).toContain('2x') // 2
66+
expect(srcsetOf(sources.eq(0))).toContain('1x') // 0
67+
expect(srcsetOf(sources.eq(1))).toContain('2x') // 1
68+
expect(srcsetOf(sources.eq(2))).toContain('2x') // 2
6069
})
6170

6271
test('image inside a list keeps its span', async () => {

0 commit comments

Comments
 (0)