feat(#431, #463): the messaging surface joins the depth vocabulary, and a correction to my own justification - #464
Merged
Merged
Conversation
… turns out to fail AAA Presentational only. No changes to key handling, RLS, or the authorization contract, and the MessageThread scroll container is untouched - `parentRef` at :369 is `absolute inset-0 overflow-y-auto` with `data-testid="message-thread"`, and four sites in tests/e2e/messaging/ dispatch scroll events against it because WebKit does not reliably fire them for programmatic scrollTop. Restyling it would have put that at risk for no visual gain. Two tab strips become rails - ConversationList's All/Unread/Archived and UnifiedSidebar's Chats/Connections. `.tab` gets no depth from #427, and DaisyUI's `tabs-boxed` and `tabs-bordered` were a fifth vocabulary alongside the rails already used by GlobalNav, /blog, /docs, /payment and the admin console. Two hand-rolled modals - ReAuthModal ("Enter Your Messaging Password", which is what /messages actually shows a returning user) and ConsentModal - carried `shadow-xl` as their only elevation. They are not `.modal-box`, so #427's plate rule never reached them, and simply deleting the shadow would have left them flat. They now carry `sh-plate` explicitly. CHECKED WHAT ASSERTS ON THESE TABS BEFORE TOUCHING THEM, AND MY FIRST CHECK WAS INCOMPLETE. I grepped tests/e2e and found `toHaveClass(/tab-active/)` belonging to ConnectionManager (friend-requests.spec.ts:307,310) and to PaymentButton's stripe/paypal tabs (three payment specs) - both left alone, deliberately. Then the vitest run failed: UnifiedSidebar.test.tsx:70 asserts `tab-active` too. Colocated unit tests are as much a consumer of these classes as the E2E suite is, and grepping only tests/e2e missed them. The assertion is updated to `sh-rail-active` WITH a negative case on the inactive tab - as written it would have passed on a component that marked every tab active - and a note that `aria-selected` on the line below is the real contract, since a class being present says nothing about whether it renders (#430). /80 AND /85 ARE NOT SAFE, AND NOW THERE ARE NUMBERS. #411 measured /60 and /70 failing and removed them; /80 and /85 were left as presumed safe. Measured properly by canvas readback, compositing the dimmed foreground over each surface, on scripthammer-light: /100 /85 /80 /70 base-100 12.96 8.30 7.08 5.20 base-200 11.73 7.73 6.62 4.98 base-300 10.10 6.88 6.05 4.57 /80 on base-200 is 6.62:1, under the 7:1 gate - and MessagingGate's card is `bg-base-200` with dimmed text inside it. /85 on base-300 is 6.88:1. The dark theme is comfortable everywhere except /70 on base-300. Full tables in #462. So the nine dim levels removed here were a real AAA failure, not just an unmeasured risk. ConversationListItem:205 keeps its `/90` deliberately: it is the read/unread distinction, it measures ~9:1 on base-100 and ~8.6:1 on base-200, and it passes. Removing it would have cost meaning for nothing. VERIFIED AGAINST THE STATE THAT SHOWS THE PAGE. /messages/setup redirects to /messages for any user who already has messaging keys, which the standing test user does - so the first run measured /messages twice and reported /messages/setup clean without ever loading it. The same shape as #454. Created a throwaway keyless user, minted a session for it by service-role grant, measured the real page (h1 "Set Up Encrypted Messaging", redirects=0), then deleted the user. 3 routes x 2 house themes: contrast fails 0, horizontal overflow 0 at 320/390/428/768/1024/1280/1440, shadow-xl 0 on the messaging surface. 82 unit tests pass across 10 files, type-check clean. Closes #431. Refs #462.
AdminGate.tsx:105, game/page.tsx:25 and schedule/page.tsx:20 each said, in the present tense, that Tailwind's `container` clamps to the previous registered breakpoint and throws away up to 43.9% of the viewport at 767px, citing #373 — as the reason those call sites had been given an explicit measure instead. None of it is true of this stylesheet, and it was not true when those comments were written. #373's §A1 was fixed by the `@utility container` override in globals.css:56-59. Measured live on /blog: viewport container max-width rendered margin-left 390px 1280px 390 0px 428px 1280px 428 0px 767px 1280px 767 0px 1023px 1280px 1023 0px 1440px 1280px 1280 80px Full width at every tier below 1280, nothing wasted. #373's table describes the state before its own fix landed. The code is unchanged — the explicit measures are fine, they are just a readability choice rather than a bug fix, and the comments now say so. Verified that removing `container`'s cap did not matter above 1280px either: /account and /profile hold a 672px box with gutters 384/384, 624/624 and 944/944 at 1440/1920/2560, /contact and /accessibility hold 896px, and horizontal overflow is zero throughout. globals.css:40 keeps the 43.9% figure on purpose: it sits inside the `@utility container` block documenting why the override exists. Historical context for a fix is not a stale claim. Why it happened, which is the part worth keeping: I read #373's headline and its measurement table and skipped its own resolution note, which is the FIRST LINE of the body. That ticket is a mix of fixed and open sections — §A1 fixed, §A2 onward open — and I quoted a figure from the fixed part as if it were current. A ticket that documents its own partial resolution has to be read to the end before any number in it is quoted. Folded into this branch rather than sent as its own PR because the E2E concurrency group allows one waiting run (#444), and an hour of queue for a three-comment correction is not a good trade. Closes #463.
…un-dim its empty state `A11yDevOverlay.tsx:252` read "No accessibility violations 🎉". The emoji is gone — it cannot take `currentColor` and renders differently on every OS, which #385 lists as a real problem regardless of which direction that ticket settles on. The sentence carries the meaning without it. The same line carried `text-base-content/60`, which measures 5.20:1 on base-100 in scripthammer-light — under the 7:1 gate. #411 removed /60 and /70 elsewhere; this one survived because the overlay is dev chrome that no route sweep visits. Full per-surface opacity table is in #462. Scope kept deliberately narrow. #385 is BLOCKED on an owner decision about whether design-specified surfaces move from icons to typographic marks, and its body now records that plus the measured state: five of its seven surfaces already carry no decorative glyph after PR #405. `SEOAnalysisPanel.tsx:42` keeps its `✗` on purpose — that string is built for the CLIPBOARD, not for markup, and a plain-text marker is the right instrument in text a user pastes into a document. The JSX comment for this change went in as a child of the `<ul>`, not inside the ternary branch. Placed between `? (` and its element it is a second root node in a parenthesised expression, which is a syntax error — the same trap recorded in #429's commit, walked into again and caught by type-check. Refs #385. Refs #462.
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
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.
Three commits: the messaging reskin, a correction to my own false justification, and the last decorative emoji on the a11y overlay.
#431 — the messaging surface
Presentational only. No changes to key handling, RLS, or the authorization contract.
MessageThread's scroll container is untouched on purpose.parentRefat:369isabsolute inset-0 overflow-y-autowithdata-testid="message-thread", and four sites intests/e2e/messaging/dispatch scroll events against it because WebKit does not reliably fire them for programmaticscrollTop. Restyling it would put that at risk for no visual gain.Two tab strips become rails —
ConversationList's All/Unread/Archived andUnifiedSidebar's Chats/Connections..tabgets no depth from #427, andtabs-boxed/tabs-borderedwere a fifth vocabulary beside the rails already used byGlobalNav,/blog,/docs,/paymentand the admin console.Two hand-rolled modals —
ReAuthModal("Enter Your Messaging Password", which is what/messagesactually shows a returning user) andConsentModal— carriedshadow-xlas their only elevation. They are not.modal-box, so #427's plate rule never reached them, and deleting the shadow would have left them flat. They now carrysh-plate.My first collision check was incomplete
I grepped
tests/e2eand foundtoHaveClass(/tab-active/)belonging toConnectionManager(friend-requests.spec.ts:307,310) and toPaymentButton's stripe/paypal tabs — both deliberately left alone.Then the vitest run failed:
UnifiedSidebar.test.tsx:70assertstab-activetoo. Colocated unit tests are as much a consumer of these classes as the E2E suite. The assertion now targetssh-rail-activewith a negative case on the inactive tab — as written it would have passed on a component that marked every tab active — and a note thataria-selectedon the next line is the real contract./80and/85are not safe, and now there are numbers#411 measured
/60and/70failing and removed them;/80and/85were left as presumed safe. Measured by canvas readback onscripthammer-light:/100/85/80/70base-100base-200base-300MessagingGate's card isbg-base-200with dimmed text inside it, so the nine removals here fixed a real AAA failure, not an unmeasured risk.ConversationListItem:205keeps its/90deliberately — it is the read/unread distinction, measures ~9:1, and passes. Full tables in #462.Verified against the state that shows the page
/messages/setupredirects to/messagesfor any user who already has messaging keys, which the standing test user does — so the first run measured/messagestwice and reported/messages/setupclean without ever loading it, the #454 shape. Created a throwaway keyless user, minted a session by service-role grant, measured the real page (h1"Set Up Encrypted Messaging",redirected=0), then deleted the user. Re-verified after rebasing onto #436's.label { display: flex }, since that changes label geometry on this surface.3 routes × 2 house themes: contrast fails 0, horizontal overflow 0 at 320/390/428/768/1024/1280/1440,
shadow-xl0 on the messaging surface. 82 unit tests across 10 files, type-check clean.#463 — I justified a fix with a bug that was already fixed
Three comments (
AdminGate.tsx:105,game/page.tsx:25,schedule/page.tsx:20) stated in the present tense thatcontainerclamps to the previous breakpoint and throws away 43.9% of the viewport, citing #373 — as the reason those call sites had an explicit measure. It was already false when written. #373's §A1 was fixed by the@utility containeroverride. Measured on/blog:containermax-widthFull width at every tier below 1280, nothing wasted. The code is unchanged — the explicit measures are fine, they are a readability choice rather than a bug fix, and the comments now say so.
globals.css:40keeps the 43.9% figure on purpose: it documents why the override exists.Why it happened: I read #373's headline and measurement table and skipped its own resolution note, which is the first line of the body. That ticket mixes fixed and open sections. A ticket documenting its own partial resolution has to be read to the end before any figure in it is quoted.
#385 — one emoji, and an un-dimmed empty state
A11yDevOverlay.tsx:252read "No accessibility violations 🎉". Emoji cannot takecurrentColorand render differently on every OS — which #385 lists as real regardless of direction. The same line carriedtext-base-content/60at 5.20:1; now solid.Scope deliberately narrow: #385 is blocked on an owner decision about whether design-specified surfaces move from icons to typographic marks, and its body now records that plus the measured state — five of its seven surfaces already carry no decorative glyph after PR #405.
SEOAnalysisPanel.tsx:42keeps its✗on purpose: that string is built for the clipboard, not markup.Also filed while measuring the nav
#378 —
Demos ▾is shipped withreload: trueintact, butDisplay ▾is not, and measuring found two defects: every one of the 13 items in the mobile hamburger is 26×144px against a 44px floor (the touch-target selector exempts bare<a>as "inline text links", which these are not — they are the entire mobile navigation), and no appearance control is reachable below 1024px at all, because the right cluster ishidden lg:block. Recorded in that ticket's body, not fixed here.Closes #431. Closes #463. Refs #462. Refs #385. Refs #378.