diff --git a/scripts/smoke-test.mjs b/scripts/smoke-test.mjs index 671a1a1..439083b 100644 --- a/scripts/smoke-test.mjs +++ b/scripts/smoke-test.mjs @@ -76,7 +76,7 @@ export function checkCase(testCase, state, iconState) { problems.push(`expected no publication, got "${state.pub.record?.name}" (${state.pub.uri})`) } // A control page must not merely fail to find a publication — it must have - // finished looking. `warning` here would mean detection errored. + // finished looking. `failed` here would mean detection errored. if (iconState !== 'none') { problems.push(`expected icon state "none", got "${iconState}"`) } diff --git a/scripts/smoke-test.test.mjs b/scripts/smoke-test.test.mjs index 0e6e369..eb43f57 100644 --- a/scripts/smoke-test.test.mjs +++ b/scripts/smoke-test.test.mjs @@ -33,7 +33,7 @@ describe('checkCase: a publication is expected', () => { // The shape the broken-build control produced: the hint found the record, // but the well-known check that proves the site owns it did not pass. it('fails a publication that was found but not verified', () => { - const problems = checkCase(detected, { pub: pub({ verified: false }) }, 'warning') + const problems = checkCase(detected, { pub: pub({ verified: false }) }, 'signedout-unverified') expect(problems).toEqual([ expect.stringContaining('not verified'), expect.stringContaining('signedout'), @@ -41,7 +41,7 @@ describe('checkCase: a publication is expected', () => { }) it('fails when nothing was detected, and says so with the error', () => { - expect(checkCase(detected, { error: 'fetch-failed' }, 'warning')).toEqual([ + expect(checkCase(detected, { error: 'fetch-failed' }, 'failed')).toEqual([ expect.stringContaining('fetch-failed'), ]) }) @@ -76,7 +76,7 @@ describe('checkCase: the control', () => { // "Found nothing" and "could not look" are different answers, and only one // of them means the control passed. it('fails a control page where detection errored rather than concluded', () => { - expect(checkCase(control, { error: 'fetch-failed' }, 'warning')).toEqual([ + expect(checkCase(control, { error: 'fetch-failed' }, 'failed')).toEqual([ expect.stringContaining('expected icon state "none"'), ]) }) diff --git a/src/lib/icon.test.ts b/src/lib/icon.test.ts index 2d870f8..580a558 100644 --- a/src/lib/icon.test.ts +++ b/src/lib/icon.test.ts @@ -8,20 +8,24 @@ const ALL_STATES: IconState[] = [ 'checking', 'none', 'offline', - 'warning', + 'failed', 'blocked', 'detected', 'signedout', 'subscribed', + 'detected-unverified', + 'signedout-unverified', + 'subscribed-unverified', ] -const BADGED_STATES: IconState[] = [ - 'checking', - 'warning', - 'blocked', - 'detected', - 'signedout', - 'subscribed', +const BADGED_STATES: IconState[] = ALL_STATES.filter( + (s) => s !== 'idle' && s !== 'none' && s !== 'offline', +) + +const UNVERIFIED_STATES: IconState[] = [ + 'detected-unverified', + 'signedout-unverified', + 'subscribed-unverified', ] function pub(verified: boolean): PubInfo { @@ -47,11 +51,11 @@ describe('iconStateFor', () => { expect(iconStateFor(state({}))).toBe('none') }) - it('is warning when detection failed for this page', () => { - expect(iconStateFor(state({ error: 'fetch-failed' }))).toBe('warning') + it('is failed when detection could not run for this page', () => { + expect(iconStateFor(state({ error: 'fetch-failed' }))).toBe('failed') }) - it('is offline, not warning, when the browser has no connection', () => { + it('is offline, not failed, when the browser has no connection', () => { expect(iconStateFor(state({ error: 'offline' }))).toBe('offline') }) @@ -76,14 +80,6 @@ describe('iconStateFor', () => { expect(iconStateFor(state({ pub: pub(true), subscriptionRkey: '3ksub' }))).toBe('subscribed') }) - it('prefers subscribed over a verification warning', () => { - expect(iconStateFor(state({ pub: pub(false), subscriptionRkey: '3ksub' }))).toBe('subscribed') - }) - - it('is warning for an unverified publication candidate', () => { - expect(iconStateFor(state({ pub: pub(false), subscriptionRkey: null }))).toBe('warning') - }) - it('is detected for a verified publication the user is not subscribed to', () => { expect(iconStateFor(state({ pub: pub(true), subscriptionRkey: null }))).toBe('detected') }) @@ -91,6 +87,24 @@ describe('iconStateFor', () => { it('is signedout when subscription state is unknown', () => { expect(iconStateFor(state({ pub: pub(true), subscriptionRkey: undefined }))).toBe('signedout') }) + + // The bug this split fixes: a subscription used to swallow the verification + // warning outright, so the badge said "✓, all well" on a page the popup was + // simultaneously calling unverified. + it('keeps the subscription and the verification warning together', () => { + expect(iconStateFor(state({ pub: pub(false), subscriptionRkey: '3ksub' }))).toBe( + 'subscribed-unverified', + ) + }) + + it('qualifies every other relation the same way', () => { + expect(iconStateFor(state({ pub: pub(false), subscriptionRkey: null }))).toBe( + 'detected-unverified', + ) + expect(iconStateFor(state({ pub: pub(false), subscriptionRkey: undefined }))).toBe( + 'signedout-unverified', + ) + }) }) describe('badgeFor', () => { @@ -100,7 +114,7 @@ describe('badgeFor', () => { } }) - it('uses green with a relationship glyph for every publication state', () => { + it('uses green with a relationship glyph for every verified publication state', () => { expect(badgeFor('detected')).toEqual({ text: '+', background: '#1a7f37', color: '#ffffff' }) expect(badgeFor('signedout')?.text).toBe('?') expect(badgeFor('subscribed')?.text).toBe('✓') @@ -109,6 +123,14 @@ describe('badgeFor', () => { } }) + it('marks an unverified page with a * on the relation glyph, in amber', () => { + for (const s of UNVERIFIED_STATES) { + const verified = s.replace('-unverified', '') as IconState + expect(badgeFor(s)?.text, s).toBe(`${badgeFor(verified)?.text}*`) + expect(badgeFor(s)?.background, s).toBe('#d4a72c') + } + }) + it('reserves red for the one state the user should stop at', () => { expect(badgeFor('blocked')).toEqual({ text: 'X', background: '#cf222e', color: '#ffffff' }) for (const s of ALL_STATES.filter((s) => s !== 'blocked')) { @@ -116,10 +138,20 @@ describe('badgeFor', () => { } }) - it('uses dark text on the amber warning badge', () => { - const warning = badgeFor('warning') - expect(warning?.background).toBe('#d4a72c') - expect(warning?.color).toBe('#24292f') + it('uses dark text on every amber badge', () => { + for (const s of ['failed', ...UNVERIFIED_STATES] as IconState[]) { + expect(badgeFor(s)?.background, s).toBe('#d4a72c') + expect(badgeFor(s)?.color, s).toBe('#24292f') + } + }) + + // `!` used to mean both "unverified candidate" and "could not check". Now + // the `*` states carry the first, so a bare `!` is only ever the second. + it('keeps the bare ! for detection failing outright', () => { + expect(badgeFor('failed')?.text).toBe('!') + for (const s of ALL_STATES.filter((s) => s !== 'failed')) { + expect(badgeFor(s)?.text ?? '', s).not.toContain('!') + } }) it('gives every badged state a distinct visible glyph, never color alone', () => { @@ -128,9 +160,13 @@ describe('badgeFor', () => { expect(new Set(glyphs).size).toBe(BADGED_STATES.length) }) - it('keeps badge text at most one glyph (Chrome truncates at 4)', () => { + it('keeps badge text at most two glyphs (Chrome truncates at 4)', () => { for (const s of ALL_STATES) { - expect([...(badgeFor(s)?.text ?? '')].length, s).toBeLessThanOrEqual(1) + const text = [...(badgeFor(s)?.text ?? '')] + expect(text.length, s).toBeLessThanOrEqual(2) + // The second is only ever the modifier; the badge never shows two + // states at once. + if (text.length === 2) expect(text[1], s).toBe('*') } }) }) @@ -156,5 +192,17 @@ describe('titleFor', () => { expect(titleFor('detected')).toContain('publication detected') expect(titleFor('signedout')).toContain('unknown') expect(titleFor('blocked')).toContain('blocked') + expect(titleFor('failed')).toContain('could not check') + }) + + // The * is the whole point of the state, so the words have to carry both + // halves — the relation the reader has, and the caveat on the page. + it('says both halves for a * state', () => { + for (const s of UNVERIFIED_STATES) { + expect(titleFor(s), s).toContain('not the verified publisher') + } + expect(titleFor('subscribed-unverified')).toContain('subscribed') + expect(titleFor('detected-unverified')).toContain('publication detected') + expect(titleFor('signedout-unverified')).toContain('unknown') }) }) diff --git a/src/lib/icon.ts b/src/lib/icon.ts index 2e407e2..3d425bf 100644 --- a/src/lib/icon.ts +++ b/src/lib/icon.ts @@ -8,6 +8,20 @@ import type { PageState } from './types' +/** + * What the user is to a publication the page carries. This axis is + * independent of whether the page is the publication's verified home, so it + * is named separately: every relation exists in a verified and an unverified + * form, and the badge shows both at once rather than picking a winner. + */ +export type Relation = + /** Signed in, not subscribed. */ + | 'detected' + /** Subscription state unknown: signed out, or the list could not be read. */ + | 'signedout' + /** Subscribed. */ + | 'subscribed' + export type IconState = /** Not an http(s) page (chrome://, the web store, ...); detection does not apply. */ | 'idle' @@ -18,35 +32,36 @@ export type IconState = /** Detection could not run because the browser is offline. Not a property * of the page, so it carries no badge — the popup and tooltip explain. */ | 'offline' - /** A publication candidate exists but failed bidirectional verification, - * or detection itself errored for this page. */ - | 'warning' + /** Detection errored for this page, so nothing is known about it. */ + | 'failed' /** The publication belongs to an account the signed-in user has blocked. */ | 'blocked' - /** Verified publication; the user is signed in and not subscribed. */ - | 'detected' - /** Verified publication; subscription state unknown (signed out, or the - * subscription list could not be fetched). */ - | 'signedout' - /** The user is subscribed to this publication. */ - | 'subscribed' + /** A verified publication, plus what the user is to it. */ + | Relation + /** The same, on a page that failed bidirectional verification. */ + | `${Relation}-unverified` /** Map a tab's detection state to an icon state. */ export function iconStateFor(state: PageState | undefined): IconState { if (!state) return 'idle' if (!state.pub) { if (state.error === 'offline') return 'offline' - return state.error ? 'warning' : 'none' + return state.error ? 'failed' : 'none' } // A block is the user's own standing decision about the account behind the // page, so it outranks every other publication state — including a // subscription, where the contradiction is exactly what needs surfacing. if (state.blocked) return 'blocked' - // A subscription is a definite user relationship; it wins over a - // verification warning (the popup still shows the unverified detail). - if (state.subscriptionRkey) return 'subscribed' - if (!state.pub.verified) return 'warning' - return state.subscriptionRkey === null ? 'detected' : 'signedout' + const relation: Relation = state.subscriptionRkey + ? 'subscribed' + : state.subscriptionRkey === null + ? 'detected' + : 'signedout' + // Verification does not compete with the relation, it qualifies it: a + // subscription to a publication seen somewhere that is not its home is + // both facts at once, and suppressing either one loses something the user + // would want to know. + return state.pub.verified ? relation : `${relation}-unverified` } // --- badge mapping ----------------------------------------------------------- @@ -74,6 +89,13 @@ export interface BadgeSpec { * ✓ subscribed, X blocked). Glyphs are plain chars that stay crisp at badge * size ("✗"-style dingbats render smudged there, which is why the block badge * is a bare letter). + * + * A trailing `*` is the one modifier, and it reads as a footnote: the + * relation still holds, but this page is not the publication's verified + * home, and the popup says why. It rides along with the amber so the caveat + * survives without color vision, and it keeps `!` for the other thing amber + * used to mean — detection failing outright, where there is no relation to + * qualify. */ export function badgeFor(state: IconState): BadgeSpec | null { switch (state) { @@ -83,7 +105,7 @@ export function badgeFor(state: IconState): BadgeSpec | null { return null case 'checking': return { text: '•', background: GREY, color: WHITE } - case 'warning': + case 'failed': return { text: '!', background: AMBER, color: INK } case 'blocked': return { text: 'X', background: RED, color: WHITE } @@ -93,14 +115,23 @@ export function badgeFor(state: IconState): BadgeSpec | null { return { text: '?', background: GREEN, color: WHITE } case 'subscribed': return { text: '✓', background: GREEN, color: WHITE } + case 'detected-unverified': + return { text: '+*', background: AMBER, color: INK } + case 'signedout-unverified': + return { text: '?*', background: AMBER, color: INK } + case 'subscribed-unverified': + return { text: '✓*', background: AMBER, color: INK } } } +const UNVERIFIED = 'this site is not the verified publisher' + /** * Action tooltip for a state; null restores the manifest default title. - * The badge encodes state in color + a single glyph, so the tooltip is the - * only place the toolbar spells the state out — for hover, and for screen - * readers, which announce the action by its title. + * The badge encodes state in color + a glyph, so the tooltip is the only + * place the toolbar spells the state out — for hover, and for screen + * readers, which announce the action by its title. The `*` states say both + * halves, in the order the badge shows them. */ export function titleFor(state: IconState): string | null { switch (state) { @@ -112,8 +143,8 @@ export function titleFor(state: IconState): string | null { return 'substandard — no publication on this page' case 'offline': return 'substandard — offline; publication lookups need a connection' - case 'warning': - return 'substandard — unverified publication or detection problem' + case 'failed': + return 'substandard — could not check this page' case 'blocked': return 'substandard — you have blocked the account behind this publication' case 'detected': @@ -122,5 +153,11 @@ export function titleFor(state: IconState): string | null { return 'substandard — publication detected; subscription state unknown' case 'subscribed': return 'substandard — subscribed to this publication' + case 'detected-unverified': + return `substandard — publication detected, but ${UNVERIFIED}` + case 'signedout-unverified': + return `substandard — publication detected, subscription state unknown; ${UNVERIFIED}` + case 'subscribed-unverified': + return `substandard — subscribed, but ${UNVERIFIED}` } }