amadan
fix: the four design defects the review found, and guards for two
**Sharing defaulted to owner.** shareableRoles was ordered most-able first and is rendered straight into a <select> with no option marked selected, so the browser selected the first one. Fill in an address, press Share, never open the dropdown — and you handed the sheet away. The only repair is the new owner giving it back. The order of that slice is a security control, not a presentation choice, so it now leads with viewer and ends with owner, and two tests hold it there: one on the slice, one on the rendered option, because a reordering that reads like tidying would otherwise restore the bug with every test still green. **No sign-out below 800px.** rastrillo's topbar hides [rst-shell-tail] and reveals it from exactly two places: an [rst-shell-menu][open] that immediately precedes it, and the 800px media query. We shipped no menu, so on every phone the navigation and the whole account dropdown were absent — not collapsed, absent. The disclosure is added as the tail's previous sibling, which is load-bearing: the framework's rule is a `+` combinator, so moving it anywhere else silently restores the fault. It deliberately stays out of the account dropdown's "rst-menus" group, since `name` makes an exclusive accordion and the dropdown lives inside the tail this menu reveals — grouped, opening one would collapse the other. A guard test asserts both existence and position, and strips template comments first so the prose explaining the element cannot satisfy the check on its own. **The sign-in help sentence rendered inside a 28px icon badge.** rst-help is the little "?" beside a label — inline-flex, fixed width and height, a border. Given a sentence it kept the box and overflowed, so the paragraph rendered a word or two per line with the badge's border struck through it, and ran off the left edge of a phone. It is rst-field-help, inside the field, under the input. **The table view was a link farm.** Every cell is an anchor because the link is the edit affordance without a script, and left at browser defaults that meant ~650 blue underlined links a page — data and emptiness in the same colour and weight. Cells now read as cells with the affordance on hover and focus-visible, numbers right-align off the computed kind rather than a guess at the input string, and the placeholder is gone because a block anchor with a min-height is already a target. Its subtitle counted neither rows nor pages; it now counts pages, and no longer leads with what the page lacks.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XtQKC2QWg1cP9RQ5zAL72J
9 files changed,
+259
−14
internal/guardstest/shellmenu_test.go+68 −0internal/sheets/handlers_share.go+18 −5internal/sheets/handlers_share_test.go+57 −0internal/sheets/handlers_table.go+14 −1internal/sheets/static/sheets.css+46 −0internal/sheets/templates/layout.html+28 −0internal/sheets/templates/link.html+6 −1internal/sheets/templates/signin.html+13 −3internal/sheets/templates/table.html+9 −4
diff --git a/internal/guardstest/shellmenu_test.go b/internal/guardstest/shellmenu_test.go| new file mode 100644 |
| index 0000000..19ae263 |
| --- /dev/null |
| +++ b/internal/guardstest/shellmenu_test.go |
| @@ -0,0 +1,68 @@ |
| +package guards |
| + |
| +import ( |
| + "os" |
| + "path/filepath" |
| + "regexp" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// TestTheTopbarDisclosesItsTailOnNarrowScreens is the guard for a bug |
| +// that removed sign-out from every phone. |
| +// |
| +// rastrillo's topbar sets [rst-shell-tail] { display: none } and reveals |
| +// it from exactly two places: an [rst-shell-menu][open] that IMMEDIATELY |
| +// PRECEDES the tail, and a min-width: 800px media query. An app that |
| +// ships no [rst-shell-menu] therefore has no navigation and no account |
| +// menu below 800px — not collapsed behind a control, simply absent, with |
| +// no way for a person to get at them. |
| +// |
| +// This is not a thing a Go test would otherwise notice. The markup is |
| +// valid, every handler works, the page renders, and the whole suite |
| +// passes; the affordance is missing only in a viewport narrower than the |
| +// one nobody was measuring in. |
| +// |
| +// Two assertions, because either alone is satisfiable while the bug is |
| +// present: the element must EXIST, and it must be the tail's immediately |
| +// previous sibling, since the framework's rule is a `+` combinator and |
| +// an element moved anywhere else in the bar restores the fault while |
| +// still reading as present. |
| +func TestTheTopbarDisclosesItsTailOnNarrowScreens(t *testing.T) { |
| + b, err := os.ReadFile(filepath.Join(repoRoot(t), |
| + "internal/sheets/templates/layout.html")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + // Template comments are stripped FIRST, and that matters: the |
| + // layout explains this element in a {{/* ... */}} block that names |
| + // it, so a check run over the raw file passes on the prose alone |
| + // while the element itself is gone. |
| + layout := regexp.MustCompile(`(?s)\{\{/\*.*?\*/\}\}`).ReplaceAllString(string(b), "") |
| + |
| + if !strings.Contains(layout, "rst-shell-menu") { |
| + t.Fatal("layout.html ships no [rst-shell-menu]: below 800px the " + |
| + "shell hides [rst-shell-tail] and nothing can reveal it, so " + |
| + "there is no navigation and no way to sign out on a phone") |
| + } |
| + |
| + // Everything between the menu's close tag and the tail's open tag. |
| + // Template actions and comments are fine there; another ELEMENT is |
| + // not, because it breaks the `+` combinator. |
| + gap := regexp.MustCompile(`(?s)</details>(.*?)<div rst-shell-tail>`) |
| + m := gap.FindStringSubmatch(layout) |
| + if m == nil { |
| + t.Fatal("could not find [rst-shell-menu] followed by " + |
| + "[rst-shell-tail] in layout.html") |
| + } |
| + between := m[1] |
| + // Strip Go template actions ({{...}}, comments included) and |
| + // whitespace; what remains must hold no tag. |
| + between = regexp.MustCompile(`(?s)\{\{.*?\}\}`).ReplaceAllString(between, "") |
| + if strings.Contains(between, "<") { |
| + t.Errorf("an element sits between [rst-shell-menu] and "+ |
| + "[rst-shell-tail]: rastrillo reveals the tail with a `+` "+ |
| + "combinator, so only the immediately previous sibling works. "+ |
| + "Found: %q", strings.TrimSpace(between)) |
| + } |
| +} |
diff --git a/internal/sheets/handlers_share.go b/internal/sheets/handlers_share.go| index e86d2e7..599151c 100644 |
| --- a/internal/sheets/handlers_share.go |
| +++ b/internal/sheets/handlers_share.go |
| @@ -28,12 +28,25 @@ type linkRow struct { |
| Created time.Time |
| } |
| -// shareableRoles is what the screen offers, most able first. |
| +// shareableRoles is what the screen offers, LEAST able first. |
| // |
| -// Owner is offered: handing a sheet on is a real thing to want, and |
| -// leaving it out means the only way to transfer one is a database |
| -// edit. |
| -var shareableRoles = []SheetRole{SheetOwner, SheetEditor, SheetCommenter, SheetViewer} |
| +// The order is the security control. A <select> with no selected option |
| +// defaults to its first, and this list is rendered straight into one, so |
| +// whichever role leads is what a person shares by default — including |
| +// the person who fills in an address, presses Share, and never opens the |
| +// dropdown at all. Most-able-first made that default "owner": the |
| +// quickest possible path through this screen handed the sheet away, and |
| +// the only repair is another owner taking it back. |
| +// |
| +// Least-able-first makes the same path grant "viewer", and anything |
| +// more has to be chosen. That is the same instinct the spec applies to |
| +// link sharing, which is viewer-or-commenter and never editor. |
| +// |
| +// Owner is still offered: handing a sheet on is a real thing to want, |
| +// and leaving it out means the only way to transfer one is a database |
| +// edit. It is last because it is the one that cannot be undone by the |
| +// person who did it. |
| +var shareableRoles = []SheetRole{SheetViewer, SheetCommenter, SheetEditor, SheetOwner} |
| // handleShare renders who can reach a sheet. |
| func (a *App) handleShare(w http.ResponseWriter, r *http.Request) { |
diff --git a/internal/sheets/handlers_share_test.go b/internal/sheets/handlers_share_test.go| index 7a9c3cb..617a3f4 100644 |
| --- a/internal/sheets/handlers_share_test.go |
| +++ b/internal/sheets/handlers_share_test.go |
| @@ -205,3 +205,60 @@ func TestTheShareScreenIsScriptless(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// The share screen's default role is the least able one, and this test |
| +// is the whole enforcement of that. |
| +// |
| +// The screen renders shareableRoles straight into a <select> with no |
| +// option marked selected, so the browser selects the first. That makes |
| +// the ORDER of this slice a security control rather than a |
| +// presentational choice: whatever leads it is what a person grants when |
| +// they fill in an address, press Share, and never touch the dropdown — |
| +// which is the fastest and therefore most common path through the |
| +// screen. |
| +// |
| +// It led with owner once. The quickest route through the sharing screen |
| +// handed the sheet away, and the only repair is the new owner giving it |
| +// back. A reordering that reads like tidying restores that, silently and |
| +// with every test still green, which is exactly why the assertion is on |
| +// the first element and not on the set. |
| +func TestSharingDefaultsToTheLeastAbleRole(t *testing.T) { |
| + if shareableRoles[0] != SheetViewer { |
| + t.Fatalf("shareableRoles[0] = %q, want %q: the first entry is the "+ |
| + "<select>'s default, so it is what somebody grants without "+ |
| + "choosing", shareableRoles[0], SheetViewer) |
| + } |
| + if shareableRoles[len(shareableRoles)-1] != SheetOwner { |
| + t.Errorf("shareableRoles ends with %q, want %q: owner is offered, "+ |
| + "but it belongs furthest from the default", |
| + shareableRoles[len(shareableRoles)-1], SheetOwner) |
| + } |
| +} |
| + |
| +// The rendered markup has to agree with the slice above: a <select> |
| +// whose first <option> is owner is the bug, however the slice is |
| +// ordered. |
| +func TestTheShareFormOffersViewerFirst(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + body := do(t, app, |
| + as(httptest.NewRequest(http.MethodGet, sharePath(s), nil), cookies)).Body.String() |
| + |
| + i := strings.Index(body, `name="role"`) |
| + if i < 0 { |
| + t.Fatal("no role select on the share screen") |
| + } |
| + first := strings.Index(body[i:], "<option") |
| + if first < 0 { |
| + t.Fatal("the role select has no options") |
| + } |
| + opt := body[i+first:] |
| + if end := strings.Index(opt, "</option>"); end >= 0 { |
| + opt = opt[:end] |
| + } |
| + if !strings.Contains(opt, string(SheetViewer)) { |
| + t.Errorf("the first role option is %q, want the viewer one: the "+ |
| + "browser selects the first option, so this is the default grant", opt) |
| + } |
| +} |
diff --git a/internal/sheets/handlers_table.go b/internal/sheets/handlers_table.go| index d570e05..7f2e9b8 100644 |
| --- a/internal/sheets/handlers_table.go |
| +++ b/internal/sheets/handlers_table.go |
| @@ -5,6 +5,7 @@ import ( |
| "strconv" |
| "strings" |
| + "amadan.net/oficina/sheets/internal/sheets/calc" |
| "amadan.net/oficina/sheets/internal/sheets/store" |
| ) |
| @@ -31,6 +32,16 @@ type tableCell struct { |
| Raw string |
| Display string |
| Editing bool |
| + // Number is true when the COMPUTED value is numeric, which is what |
| + // lets the view right-align it. Alignment is the type signal in a |
| + // spreadsheet, and the grid gets it from Jspreadsheet for free while |
| + // this view has to be told. |
| + // |
| + // It is taken from the computed kind rather than from Raw, because |
| + // `=SUM(B2:B4)` is a formula whose value is a number, and sniffing |
| + // the input string would align the two differently. The server owns |
| + // every string on screen here; it owns this one bit too. |
| + Number bool |
| } |
| type tableRow struct { |
| @@ -253,8 +264,10 @@ func (a *App) tableRowsIn(h *store.Handle, order []int, editR, editC int) []tabl |
| } |
| if rowID, okR := idx.rowID[rowIdx]; okR { |
| if colID, okC := idx.colID[c]; okC { |
| + v := h.Value(rowID, colID) |
| cell.Raw, _ = h.RawInput(rowID, colID) |
| - cell.Display = display(h.Value(rowID, colID), h.Format(rowID, colID)) |
| + cell.Display = display(v, h.Format(rowID, colID)) |
| + cell.Number = v.Kind == calc.KindNum |
| } |
| } |
| row.Cells = append(row.Cells, cell) |
diff --git a/internal/sheets/static/sheets.css b/internal/sheets/static/sheets.css| index c2dc2d4..132bd44 100644 |
| --- a/internal/sheets/static/sheets.css |
| +++ b/internal/sheets/static/sheets.css |
| @@ -192,6 +192,52 @@ |
| font-weight: 600; |
| } |
| +/* The cell link, which is every cell. |
| + * |
| + * Each cell is an anchor, because the link IS the edit affordance in a |
| + * view with no script. Left at the browser's default that meant roughly |
| + * 650 blue underlined links per page — data and emptiness in the same |
| + * colour and the same weight, so `94200` and a blank cell differed only |
| + * in glyph width. A spreadsheet you can only read cell by cell is not a |
| + * spreadsheet, and this is the view the spec calls the product. |
| + * |
| + * So the cell reads as a cell and the affordance shows on approach. The |
| + * anchor still fills the cell, so the target is the whole box rather |
| + * than the few pixels of the glyph — which is also what lets an empty |
| + * cell be clickable without a standing in for content. |
| + * |
| + * :focus-visible carries the same treatment as :hover deliberately. |
| + * Keyboard is the primary way through this view, and a focus ring alone |
| + * on a 26-column grid is easy to lose. */ |
| +[data-cell-table] td > a { |
| + color: inherit; |
| + text-decoration: none; |
| + display: block; |
| + min-block-size: 1.25rem; |
| + border-radius: var(--rst-radius-sm); |
| +} |
| + |
| +[data-cell-table] td > a:hover, |
| +[data-cell-table] td > a:focus-visible { |
| + background: var(--rst-accent-soft); |
| + color: var(--rst-accent); |
| +} |
| + |
| +/* Numbers right, text left — the alignment IS the type signal in a |
| + * spreadsheet, and it is the one the grid gives for free and this view |
| + * had to be told. |
| + * |
| + * Both spellings, because there are two of these tables. The editable |
| + * one wraps every cell in an anchor, so the alignment has to reach the |
| + * block anchor rather than the cell around it; the anonymous link view |
| + * has no anchors, because there is nothing on it to edit. One rule for |
| + * each, rather than one selector that quietly covers only the surface |
| + * that happened to get tested. */ |
| +[data-cell-table] td[data-kind="number"], |
| +[data-cell-table] td[data-kind="number"] > a { |
| + text-align: end; |
| +} |
| + |
| [data-cell-table] td form { |
| display: flex; |
| gap: var(--rst-sp-1); |
diff --git a/internal/sheets/templates/layout.html b/internal/sheets/templates/layout.html| index cf850d2..7d2e7c5 100644 |
| --- a/internal/sheets/templates/layout.html |
| +++ b/internal/sheets/templates/layout.html |
| @@ -35,6 +35,34 @@ |
| <a rst-skip href="#main">Skip to content</a> |
| <header rst-shell-bar> |
| <a rst-shell-brand href="/">Sheets</a> |
| +{{if .SignedIn}} |
| +{{/* The narrow-viewport disclosure, and without it there is no way to |
| + sign out on a phone. |
| + |
| + rastrillo's topbar sets [rst-shell-tail] { display: none } and |
| + reveals it from exactly two places: an [rst-shell-menu][open] that |
| + IMMEDIATELY PRECEDES the tail, and the 800px media query. An app |
| + that ships no menu therefore has no navigation and no account below |
| + 800px — not collapsed behind something, simply absent. So the |
| + position of this element is load-bearing: it is the tail's previous |
| + sibling because the framework's rule is a `+` combinator, and |
| + moving it anywhere else silently restores the bug. |
| + |
| + It deliberately does NOT join the account dropdown's "rst-menus" |
| + group. `name` on <details> makes an exclusive accordion, and the |
| + account dropdown lives INSIDE the tail this menu reveals — grouped, |
| + opening the account would close this one, collapse the tail, and |
| + take the dropdown down with it. They nest, so they must not be |
| + exclusive. |
| + |
| + Rendered only when signed in: signed out the tail holds an empty |
| + nav, and a control that discloses nothing is worse than no control. |
| + rastrillo hides it itself above 800px, so there is no media query |
| + of ours here. */}} |
| +<details rst-shell-menu> |
| +<summary>Menu</summary> |
| +</details> |
| +{{end}} |
| <div rst-shell-tail> |
| <nav rst-shell-nav> |
| {{if .SignedIn}}<a href="/sheets">Your sheets</a>{{end}} |
diff --git a/internal/sheets/templates/link.html b/internal/sheets/templates/link.html| index 0c941d9..4f9090e 100644 |
| --- a/internal/sheets/templates/link.html |
| +++ b/internal/sheets/templates/link.html |
| @@ -33,7 +33,12 @@ is for.</p> |
| column that cannot match any cell, so .Editing is false |
| everywhere — read-only expressed in the data rather than trusted |
| to this template. */}} |
| -{{range .Cells}}<td{{if .Raw}} title="{{.Raw}}"{{end}}>{{if .Display}}{{.Display}}{{else}} {{end}}</td>{{end}} |
| +{{/* data-kind carries the same numeric flag the editable table uses, so |
| + a link viewer gets the same right-aligned numbers. The cells here |
| + are plain <td> rather than anchors — there is nothing to edit — and |
| + with no anchor there is nothing to keep an empty cell open, so the |
| + stays on this view and only on this view. */}} |
| +{{range .Cells}}<td{{if .Number}} data-kind="number"{{end}}{{if .Raw}} title="{{.Raw}}"{{end}}>{{if .Display}}{{.Display}}{{else}} {{end}}</td>{{end}} |
| </tr> |
| {{end}} |
| </tbody> |
diff --git a/internal/sheets/templates/signin.html b/internal/sheets/templates/signin.html| index 0732626..8b4fce6 100644 |
| --- a/internal/sheets/templates/signin.html |
| +++ b/internal/sheets/templates/signin.html |
| @@ -16,6 +16,19 @@ |
| <div rst-field> |
| <label rst-field-label for="address">Email address</label> |
| <input rst-input id="address" name="address" type="email" autocomplete="email" required> |
| +{{/* rst-field-help, not rst-help. rst-help is a 28x28 icon BADGE — |
| + inline-flex, a fixed width and height, a border and a radius — for |
| + the little "?" beside a label. Given a sentence it keeps the 28px |
| + box and lets the text overflow it, which rendered this paragraph |
| + one or two words per line with the badge's border struck through |
| + the middle of it, and pushed it off the left edge of the viewport |
| + on a phone. rst-field-help is the prose one: block, muted, sized |
| + down, spaced off the control above it. |
| + |
| + It lives inside the field, under the input, because that is what |
| + the token is for and where guidance about a control belongs. */}} |
| +<p rst-field-help>We email you a link instead of asking for a password. |
| +It signs you in once and then stops working.</p> |
| </div> |
| {{/* Force is the affordance auth asks for. When a keymail sign-in |
| fails, auth redirects to ?force=1&err=keymail specifically so this |
| @@ -28,7 +41,4 @@ |
| <button rst-btn="primary" type="submit">Email me a link</button> |
| </div> |
| </form> |
| - |
| -<p rst-help>We email you a link instead of asking for a password. It signs |
| -you in once and then stops working.</p> |
| {{end}} |
diff --git a/internal/sheets/templates/table.html b/internal/sheets/templates/table.html| index 31fc509..ab34b2b 100644 |
| --- a/internal/sheets/templates/table.html |
| +++ b/internal/sheets/templates/table.html |
| @@ -2,8 +2,13 @@ |
| <header rst-page-header> |
| <div rst-page-header-titles> |
| <h1>{{.Data.Sheet.Title}}</h1> |
| -<p rst-page-header-sub>Table view — rows {{.Data.Page}} of {{.Data.Pages}} pages. |
| -Every cell is editable here, with no JavaScript at all.</p> |
| +{{/* Was "rows {{.Data.Page}} of {{.Data.Pages}} pages", which spliced a |
| + row count and a page count into one phrase that counted neither. |
| + The trailing "with no JavaScript at all" went with it: this view is |
| + the product rather than the fallback, and a subtitle that leads with |
| + what the page lacks argues the opposite. */}} |
| +<p rst-page-header-sub>Page {{.Data.Page}} of {{.Data.Pages}}. Every cell |
| +here is editable.</p> |
| </div> |
| <a rst-btn="ghost" href="/sheets/{{.Data.Sheet.ID}}/history">History</a> |
| <a rst-btn="ghost" href="/sheets/{{.Data.Sheet.ID}}/comments">Comments</a> |
| @@ -72,7 +77,7 @@ rows are where they always were.</p> |
| <tr> |
| <th scope="row">{{.Number}}</th> |
| {{range .Cells}} |
| -<td> |
| +<td{{if .Number}} data-kind="number"{{end}}> |
| {{if .Editing}} |
| {{/* One cell at a time carries a form. Rendering an input in all 650 |
| cells of a page would make the scriptless view the heaviest page |
| @@ -90,7 +95,7 @@ rows are where they always were.</p> |
| above. The cell shows its computed value, and the title carries |
| the input, so a formula is readable without opening it. */}} |
| <a href="/sheets/{{$sheet.ID}}/table?page={{$page}}&edit={{.R}},{{.C}}" |
| - aria-label="Edit {{.Ref}}"{{if .Raw}} title="{{.Raw}}"{{end}}>{{if .Display}}{{.Display}}{{else}} {{end}}</a> |
| + aria-label="Edit {{.Ref}}"{{if .Raw}} title="{{.Raw}}"{{end}}>{{.Display}}</a> |
| {{end}} |
| </td> |
| {{end}} |