diff --git a/frontend/apps/remark42/app/profile.ts b/frontend/apps/remark42/app/profile.ts index 15adc4c1..5bc052a8 100644 --- a/frontend/apps/remark42/app/profile.ts +++ b/frontend/apps/remark42/app/profile.ts @@ -22,7 +22,15 @@ function createElement( function createFragment(params: Profile & Record) { removeIframe(); - iframe = createIframe({ ...params, page: 'profile', styles: styles.iframe }); + // the iframe is hidden until it reveals itself, and a hidden element cannot take + // focus, so focus from the reveal rather than on a timer + const created: HTMLIFrameElement = createIframe({ + ...params, + page: 'profile', + styles: styles.iframe, + onReveal: () => created.isConnected && created.focus(), + }); + iframe = created; if (!root) { root = createElement('div', styles.root); @@ -31,7 +39,6 @@ function createFragment(params: Profile & Record) { root.appendChild(iframe); setStyles(root, styles.rootShown); - setTimeout(() => iframe?.focus()); } function animateAppear(): void { diff --git a/frontend/apps/remark42/app/utils/create-iframe.test.ts b/frontend/apps/remark42/app/utils/create-iframe.test.ts new file mode 100644 index 00000000..537faa6d --- /dev/null +++ b/frontend/apps/remark42/app/utils/create-iframe.test.ts @@ -0,0 +1,95 @@ +import { createIframe } from './create-iframe'; + +const REVEAL_TIMEOUT = 5000; + +function postInited(source: MessageEventSource | null) { + window.dispatchEvent(new MessageEvent('message', { data: { inited: true }, source })); +} + +describe('createIframe', () => { + beforeEach(() => { + jest.useFakeTimers(); + document.body.innerHTML = ''; + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it('starts hidden', () => { + const iframe = createIframe({ site_id: 'remark' }); + expect(iframe.style.visibility).toBe('hidden'); + }); + + it('lets caller styles override visibility', () => { + const iframe = createIframe({ site_id: 'remark', styles: { visibility: 'visible' } }); + expect(iframe.style.visibility).toBe('visible'); + }); + + it('reveals when its own document reports inited', () => { + const iframe = createIframe({ site_id: 'remark' }); + document.body.appendChild(iframe); + + postInited(iframe.contentWindow); + expect(iframe.style.visibility).toBe('visible'); + }); + + it('ignores inited from a foreign source', () => { + const iframe = createIframe({ site_id: 'remark' }); + const other = document.createElement('iframe'); + document.body.append(iframe, other); + + postInited(other.contentWindow); + expect(iframe.style.visibility).toBe('hidden'); + + postInited(window); + expect(iframe.style.visibility).toBe('hidden'); + }); + + it('reveals on timeout when inited never arrives', () => { + const iframe = createIframe({ site_id: 'remark' }); + document.body.appendChild(iframe); + + jest.advanceTimersByTime(REVEAL_TIMEOUT - 1); + expect(iframe.style.visibility).toBe('hidden'); + + jest.advanceTimersByTime(1); + expect(iframe.style.visibility).toBe('visible'); + }); + + it('calls onReveal after the iframe becomes visible', () => { + const onReveal = jest.fn(() => { + expect(iframe.style.visibility).toBe('visible'); + }); + const iframe = createIframe({ site_id: 'remark', onReveal }); + document.body.appendChild(iframe); + + postInited(iframe.contentWindow); + expect(onReveal).toHaveBeenCalledTimes(1); + }); + + it('calls onReveal on the timeout path too', () => { + const onReveal = jest.fn(); + createIframe({ site_id: 'remark', onReveal }); + + jest.advanceTimersByTime(REVEAL_TIMEOUT); + expect(onReveal).toHaveBeenCalledTimes(1); + }); + + it('drops the listener and the timer on reveal', () => { + const onReveal = jest.fn(); + const iframe = createIframe({ site_id: 'remark', onReveal }); + document.body.appendChild(iframe); + + postInited(iframe.contentWindow); + postInited(iframe.contentWindow); + jest.advanceTimersByTime(REVEAL_TIMEOUT); + + expect(onReveal).toHaveBeenCalledTimes(1); + }); + + it('does not put onReveal into the iframe query', () => { + const iframe = createIframe({ site_id: 'remark', onReveal: () => undefined }); + expect(iframe.getAttribute('src')).not.toContain('onReveal'); + }); +}); diff --git a/frontend/apps/remark42/app/utils/create-iframe.ts b/frontend/apps/remark42/app/utils/create-iframe.ts index 6f694113..2c280365 100644 --- a/frontend/apps/remark42/app/utils/create-iframe.ts +++ b/frontend/apps/remark42/app/utils/create-iframe.ts @@ -1,9 +1,34 @@ import { BASE_URL } from 'common/constants.config'; +import { parseMessage } from 'utils/post-message'; import { setStyles, setAttributes, StylesDeclaration } from 'utils/set-dom-props'; -type Params = { [key: string]: unknown; __colors__?: Record; styles?: StylesDeclaration }; +type Params = { + [key: string]: unknown; + __colors__?: Record; + styles?: StylesDeclaration; + onReveal?: () => void; +}; -export function createIframe({ __colors__, styles, ...params }: Params) { +/** + * How long to wait for the iframe document to report itself as inited before + * showing it anyway. Only reached when the document fails to bootstrap. + */ +const REVEAL_TIMEOUT = 5000; + +/** + * Creates the remark42 iframe for the given params. + * + * The returned iframe starts hidden and becomes visible once its document posts + * `inited`, or after REVEAL_TIMEOUT if that never arrives. Until then it holds a + * global `message` listener and a timer, both dropped on the first reveal. + * + * `onReveal` runs right after the iframe becomes visible. Anything that needs a + * visible iframe, such as focus(), belongs there rather than on a timer. + * + * `styles` is applied last, so a caller passing `visibility` overrides the hiding + * and brings the white flash back. + */ +export function createIframe({ __colors__, styles, onReveal, ...params }: Params) { const iframe = document.createElement('iframe'); const query = new URLSearchParams(params as Record).toString(); @@ -21,8 +46,35 @@ export function createIframe({ __colors__, styles, ...params }: Params) { margin: 0, overflow: 'hidden', colorScheme: params.theme === 'dark' ? 'dark' : 'light', + // an iframe whose color-scheme differs from its document's gets an opaque canvas + // painted in the document's scheme, and browsers paint a default surface before + // the document is parsed at all, which shows as a white flash on dark host pages. + // reveal only on `inited`, which the document posts from its body script; the head + // script has applied the matching scheme by then, so the two always agree on reveal. + visibility: 'hidden', ...styles, }); + hideUntilInited(iframe, onReveal); + return iframe; } + +function hideUntilInited(iframe: HTMLIFrameElement, onReveal?: () => void) { + function reveal() { + window.removeEventListener('message', handleMessage); + window.clearTimeout(timeout); + iframe.style.visibility = 'visible'; + onReveal?.(); + } + + function handleMessage(event: MessageEvent) { + if (event.source === iframe.contentWindow && parseMessage(event).inited === true) { + reveal(); + } + } + + const timeout = window.setTimeout(reveal, REVEAL_TIMEOUT); + + window.addEventListener('message', handleMessage); +} diff --git a/frontend/apps/remark42/app/utils/post-message.ts b/frontend/apps/remark42/app/utils/post-message.ts index f14c0874..3ec62ad3 100644 --- a/frontend/apps/remark42/app/utils/post-message.ts +++ b/frontend/apps/remark42/app/utils/post-message.ts @@ -1,6 +1,7 @@ import type { Theme, Profile } from 'common/types'; type ParentMessage = { + /** the iframe document bootstrapped: drives placeholder removal and iframe reveal */ inited?: true; scrollTo?: number; height?: number; diff --git a/frontend/apps/remark42/templates/iframe.ejs b/frontend/apps/remark42/templates/iframe.ejs index eebe7d58..dbb40bed 100644 --- a/frontend/apps/remark42/templates/iframe.ejs +++ b/frontend/apps/remark42/templates/iframe.ejs @@ -10,6 +10,25 @@ if (window.location.search === '?selfClose') { window.close(); } + (function () { + // the parent sets color-scheme on the iframe element from the theme param. this document + // must match it before the first paint, otherwise the canvas is painted opaque in its own + // scheme instead of staying transparent, which flashes white on dark host pages. + try { + var pairs = window.location.search.substring(1).split('&'); + var theme = ''; + + for (var i = 0; i < pairs.length; i++) { + var pair = pairs[i].split('='); + if (pair[0] === 'theme') { + theme = decodeURIComponent(pair[1] || ''); + break; + } + } + + document.documentElement.style.colorScheme = theme === 'dark' ? 'dark' : 'light'; + } catch (e) {} + })(); (function () { function isCSSVariablesSupported() { if (typeof window === `undefined`) { diff --git a/frontend/e2e/tests/iframe-theme.spec.ts b/frontend/e2e/tests/iframe-theme.spec.ts new file mode 100644 index 00000000..25e7e0b9 --- /dev/null +++ b/frontend/e2e/tests/iframe-theme.spec.ts @@ -0,0 +1,83 @@ +import { test, expect, type Page } from '@playwright/test' + +// the parent page sets color-scheme on the iframe element from the theme param. if the iframe +// document does not carry the same color-scheme before its bundle runs, the canvas is painted +// opaque white instead of staying transparent. block the bundle to freeze the document in that +// pre-script state and assert the inline head script has already applied the scheme. +test.describe('Iframe color scheme', () => { + test.beforeEach(async ({ page }) => { + await page.route(/remark\.m?js$/, (route) => route.abort()) + }) + + const cases = [ + { name: 'dark theme', query: '?site_id=remark&theme=dark', expected: 'dark' }, + { name: 'light theme', query: '?site_id=remark&theme=light', expected: 'light' }, + { name: 'no theme falls back to light', query: '?site_id=remark', expected: 'light' }, + ] + + for (const { name, query, expected } of cases) { + test(name, async ({ page }) => { + await page.goto(`/web/iframe.html${query}`) + + const inline = await page.evaluate(() => document.documentElement.style.colorScheme) + expect(inline).toBe(expected) + + const computed = await page.evaluate(() => getComputedStyle(document.documentElement).colorScheme) + expect(computed).toBe(expected) + }) + } +}) + +// browsers paint a default surface for an iframe before its document is parsed, and that surface +// is opaque when the element carries a color-scheme the document does not have yet. WebKit shows +// it as a white flash on dark host pages. the parent keeps the iframe hidden until the document +// reports itself inited, so the surface is never presented. +test.describe('Iframe reveal', () => { + // REVEAL_TIMEOUT in app/utils/create-iframe.ts. the fallback timer starts when the + // iframe is created, during page load, so any assertion with a deadline at or past + // this value can be satisfied by the fallback alone and says nothing about the + // message path. bound the message-path assertions well under it. + const REVEAL_TIMEOUT = 5000 + const MESSAGE_REVEAL_BUDGET = 1500 + + const visibility = (page: Page) => + page.evaluate(() => { + const iframe = document.querySelector('#remark42 iframe') + return iframe ? iframe.style.visibility : 'no-iframe' + }) + + test('stays hidden until the document reports inited', async ({ page }) => { + await page.route(/\/web\/iframe\.html/, (route) => route.abort()) + const start = Date.now() + await page.goto('/web/') + await page.waitForSelector('#remark42 iframe', { state: 'attached' }) + + expect(await visibility(page)).toBe('hidden') + // a slow run could have let the fallback fire, which would make the assertion + // above pass or fail for the wrong reason. fail loudly instead of flaking. + expect(Date.now() - start).toBeLessThan(REVEAL_TIMEOUT) + }) + + // must reveal from the inited message, not the fallback: a broken message listener would + // leave the widget invisible for 5s on every load. the fallback timer starts when the + // iframe is created, partway through goto(), so bounding only the poll leaves the + // navigation window unmeasured. time the whole thing. + test('is revealed by the inited message, well before the fallback', async ({ page }) => { + const start = Date.now() + await page.goto('/web/') + + await expect.poll(() => visibility(page), { timeout: MESSAGE_REVEAL_BUDGET }).toBe('visible') + expect(Date.now() - start).toBeLessThan(REVEAL_TIMEOUT) + await expect(page.locator('#remark42 iframe')).toBeVisible() + }) + + // the aborted document never reports its height, so the iframe box stays empty and + // toBeVisible() would fail on geometry. assert the property the fallback actually sets. + test('is revealed by the timeout when inited never arrives', async ({ page }) => { + await page.route(/\/web\/iframe\.html/, (route) => route.abort()) + await page.goto('/web/') + await page.waitForSelector('#remark42 iframe', { state: 'attached' }) + + await expect.poll(() => visibility(page), { timeout: REVEAL_TIMEOUT * 2 }).toBe('visible') + }) +})