amadan
merge: preserve deployed Sheets chrome with current suite doctrine
21 files changed,
+1548
−103
internal/sheets/app.go+10 −0internal/sheets/handlers_import.go+2 −2internal/sheets/handlers_import_page.go+60 −0internal/sheets/handlers_import_refusals_test.go+13 −4internal/sheets/handlers_list_chrome_test.go+216 −0internal/sheets/handlers_rename.go+156 −0internal/sheets/handlers_rename_test.go+263 −0internal/sheets/handlers_sheets.go+17 −18internal/sheets/handlers_table_test.go+13 −2internal/sheets/static/drop.js+56 −6internal/sheets/static/sheets.css+183 −3internal/sheets/templates/import.html+53 −0internal/sheets/templates/layout.html+22 −3internal/sheets/templates/sheet.html+36 −2internal/sheets/templates/sheets.html+124 −53tools/drive/chrome.mjs+17 −3tools/drive/clipboard.mjs+5 −1tools/drive/drive.mjs+22 −4tools/drive/drop.mjs+268 −0tools/drive/menus.mjs+7 −1tools/drive/tableformat.mjs+5 −1
CI passed — run details
diff --git a/internal/sheets/app.go b/internal/sheets/app.go| index e7a000d..614c2d6 100644 |
| --- a/internal/sheets/app.go |
| +++ b/internal/sheets/app.go |
| @@ -190,6 +190,13 @@ func (a *App) Router() http.Handler { |
| r.Use(a.requireSession) |
| r.Get("/sheets", a.handleSheetList) |
| r.Post("/sheets", a.handleSheetCreate) |
| + // Static before the {sheetID} pattern below. chi prefers an |
| + // exact segment over a wildcard whatever the order, so this is |
| + // belt and braces rather than load-bearing — but a route that |
| + // only works because of resolution order is a route one |
| + // refactor away from parsing "import" as a sheet id. |
| + // TestImportPathIsNotASheetID pins it. |
| + r.Get("/sheets/import", a.handleImportPage) |
| // Import. A whole workbook is too much work for a request, so |
| // it 303s to a job (spec §11) rather than holding the |
| // connection open — and the drop zone on the list posts to |
| @@ -197,6 +204,9 @@ func (a *App) Router() http.Handler { |
| // over the form, not a second way in. |
| r.Post("/sheets/import", a.handleSheetImport) |
| r.Get("/sheets/{sheetID}", a.handleSheetPage) |
| + // Rename. The other half of a Create button that no longer |
| + // asks for a name — see handlers_rename.go. |
| + r.Post("/sheets/{sheetID}/rename", a.handleSheetRename) |
| // The scriptless view. Spec §6 names the grid editor, drag-fill |
| // and live sync as the only script-required things; everything |
| // else, including editing any cell, works here. |
diff --git a/internal/sheets/handlers_import.go b/internal/sheets/handlers_import.go| index 09ba94f..5b7be53 100644 |
| --- a/internal/sheets/handlers_import.go |
| +++ b/internal/sheets/handlers_import.go |
| @@ -111,7 +111,7 @@ func (a *App) handleSheetImport(w http.ResponseWriter, r *http.Request) { |
| data, filename, refusal := readUpload(w, r) |
| if refusal != "" { |
| - a.renderSheetList(w, r, http.StatusBadRequest, refusal) |
| + a.renderImportPage(w, r, http.StatusBadRequest, refusal) |
| return |
| } |
| @@ -131,7 +131,7 @@ func (a *App) handleSheetImport(w http.ResponseWriter, r *http.Request) { |
| // nothing and nobody asked for it. |
| a.discardSheet(s.ID) |
| if errors.Is(err, jobs.ErrOwnerBusy) { |
| - a.renderSheetList(w, r, http.StatusTooManyRequests, refusalBusy) |
| + a.renderImportPage(w, r, http.StatusTooManyRequests, refusalBusy) |
| return |
| } |
| a.fail(w, r, err) |
diff --git a/internal/sheets/handlers_import_page.go b/internal/sheets/handlers_import_page.go| new file mode 100644 |
| index 0000000..a5e78d9 |
| --- /dev/null |
| +++ b/internal/sheets/handlers_import_page.go |
| @@ -0,0 +1,60 @@ |
| +package sheets |
| + |
| +import "net/http" |
| + |
| +// The import page. |
| +// |
| +// Separate from handlers_import.go, which holds the POST that does the |
| +// work, for the dullest possible reason: together they were 305 lines |
| +// and the file cap is 300 (AGENTS.md / Mechanical rules, enforced by |
| +// TestFileLineCap). The split falls in a sensible place anyway — this |
| +// file is the screen, that one is the upload. |
| + |
| +// importPageView is the import page's payload. |
| +type importPageView struct { |
| + // Error is a refusal to show above the form — one of the refusal |
| + // constants in handlers_import.go, never a wrapped error, because |
| + // everything shown here is read by somebody deciding what to do |
| + // differently. |
| + Error string |
| +} |
| + |
| +// handleImportPage draws the import form on a page of its own. |
| +// |
| +// Import used to be a second form on the sheet list, below the form |
| +// that made a blank sheet. Both came off that page: a list of sheets |
| +// should be a list of sheets, and the verbs moved into the chrome |
| +// around it. |
| +// |
| +// A page rather than an item inside the File menu, which is where this |
| +// nearly went. Three reasons, and the third is the one that decided it: |
| +// a dropdown is a poor container for a file picker plus three limits; a |
| +// hidden input behind a menu-shaped label never submits itself, so that |
| +// menu item would have needed script to import anything at all, and |
| +// import is meant to work with scripting off; and the File menu exists |
| +// only on a sheet page, so somebody with no sheets yet — exactly the |
| +// person most likely to be importing one — would have had no way in. |
| +func (a *App) handleImportPage(w http.ResponseWriter, r *http.Request) { |
| + // Behind requireSession already; this is the same belt-and-braces |
| + // actor check the sibling handlers make, and it is what makes the |
| + // handler safe to call from a test that does not mount the group. |
| + if _, ok := a.actor(r); !ok { |
| + http.Error(w, "Sign in first.", http.StatusForbidden) |
| + return |
| + } |
| + a.renderImportPage(w, r, http.StatusOK, "") |
| +} |
| + |
| +// renderImportPage draws that form, optionally carrying a refusal and |
| +// the status that goes with it. |
| +// |
| +// A refused import re-renders this page rather than redirecting to it, |
| +// so the refusal survives without a flash: the person lands back on the |
| +// form they posted from, with the reason above it. No session storage, |
| +// no cookie, no redirect, and it works with scripting off by |
| +// construction. That reasoning arrived here from renderSheetList along |
| +// with the form itself. |
| +func (a *App) renderImportPage(w http.ResponseWriter, r *http.Request, status int, refusal string) { |
| + a.render(w, r, "import", "Import a spreadsheet — Oficina Sheets", status, |
| + importPageView{Error: refusal}) |
| +} |
diff --git a/internal/sheets/handlers_import_refusals_test.go b/internal/sheets/handlers_import_refusals_test.go| index ac77d1b..1d448e6 100644 |
| --- a/internal/sheets/handlers_import_refusals_test.go |
| +++ b/internal/sheets/handlers_import_refusals_test.go |
| @@ -15,8 +15,12 @@ import ( |
| ) |
| // TestImportRefusesWhatItCannotRead covers every refusal a person can |
| -// hit before a job is ever started — each one answering on the list |
| -// page they posted from, with the reason beside the form. |
| +// hit before a job is ever started — each one answering on the import |
| +// page they posted from, with the reason above the form. |
| +// |
| +// That page used to be the sheet list, which was where the import form |
| +// lived. The form moved; the principle did not, and it is the reason a |
| +// refusal still needs no flash, no cookie and no redirect. |
| func TestImportRefusesWhatItCannotRead(t *testing.T) { |
| for _, tc := range []struct { |
| name string |
| @@ -175,11 +179,16 @@ func TestTitleFromFilename(t *testing.T) { |
| // product rather than the fallback, and drag-and-drop is the easiest |
| // place in this app to break it — a fetch-based uploader would look |
| // identical in a browser and leave nothing behind for anyone else. |
| +// Asked of /sheets/import, not of /sheets. The form moved to a page of |
| +// its own when the list page's two inline forms came off it; the |
| +// question this test asks is unchanged and is the reason the form |
| +// became a page rather than an item in the File menu, where a hidden |
| +// input behind a label would have needed script to submit at all. |
| func TestImportIsScriptless(t *testing.T) { |
| app, cookies := signIn(t) |
| - rec := do(t, app, as(httptest.NewRequest(http.MethodGet, "/sheets", nil), cookies)) |
| + rec := do(t, app, as(httptest.NewRequest(http.MethodGet, "/sheets/import", nil), cookies)) |
| if rec.Code != http.StatusOK { |
| - t.Fatalf("sheet list answered %d, want 200", rec.Code) |
| + t.Fatalf("import page answered %d, want 200", rec.Code) |
| } |
| body := rec.Body.String() |
diff --git a/internal/sheets/handlers_list_chrome_test.go b/internal/sheets/handlers_list_chrome_test.go| new file mode 100644 |
| index 0000000..5369f48 |
| --- /dev/null |
| +++ b/internal/sheets/handlers_list_chrome_test.go |
| @@ -0,0 +1,216 @@ |
| +package sheets |
| + |
| +import ( |
| + "bytes" |
| + "mime/multipart" |
| + "net/http" |
| + "net/http/httptest" |
| + "os" |
| + "path/filepath" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// The sheet list is a list of sheets. |
| +// |
| +// It used to open with two stacked forms — a title field with a Create |
| +// button, then a dashed drop zone with a file input and four lines of |
| +// limits — so the sheets themselves started below both. These tests pin |
| +// the removal, because the easiest way to undo it is for somebody to |
| +// add "just one small form" back to the top of the page. |
| + |
| +// signedInListBody renders the list for a fresh signed-in member. |
| +// preview_test.go's listBody takes an app and cookies because the |
| +// thumbnail tests need sheets in it; these tests want the chrome around |
| +// an empty one, so this wraps the sign-in rather than duplicating it. |
| +func signedInListBody(t *testing.T) string { |
| + t.Helper() |
| + app, cookies := signIn(t) |
| + return listBody(t, app, cookies) |
| +} |
| + |
| +// dropOverlay returns the sheet list's body up to the drop overlay, and |
| +// the overlay's own start tag. Everything before the overlay is what |
| +// the page actually renders; the overlay itself is hidden until a file |
| +// is dragged over the window. |
| +func dropOverlay(t *testing.T, body string) (inFlow, tag string) { |
| + t.Helper() |
| + start := strings.Index(body, "<form data-drop") |
| + if start < 0 { |
| + t.Fatal("the sheet list has no drop overlay — dropping a file on this page is the affordance it traded its drop box for") |
| + } |
| + end := strings.Index(body[start:], ">") |
| + if end < 0 { |
| + t.Fatal("the drop overlay's start tag never closes") |
| + } |
| + return body[:start], body[start : start+end+1] |
| +} |
| + |
| +func TestSheetListCarriesNoInlineForms(t *testing.T) { |
| + body := signedInListBody(t) |
| + |
| + // The title field is simply gone. A sheet is named by renaming it, |
| + // on the sheet itself, so there is nothing left for this to be. |
| + if strings.Contains(body, `name="title"`) { |
| + t.Error(`the sheet list is carrying name="title" again — naming a sheet is a rename, in the top bar of the sheet`) |
| + } |
| + |
| + // The file input is back, but only inside the overlay. Anything of |
| + // it appearing BEFORE the overlay's start tag is a box in the page |
| + // again, which is the thing this change removed. |
| + inFlow, _ := dropOverlay(t, body) |
| + for _, gone := range []string{ |
| + `type="file"`, |
| + `enctype="multipart/form-data"`, |
| + "data-drop", |
| + } { |
| + if strings.Contains(inFlow, gone) { |
| + t.Errorf("%s renders in the flow of the sheet list — the drop target is an overlay, not a box above the sheets", gone) |
| + } |
| + } |
| +} |
| + |
| +// The overlay is a real import form that happens to be invisible until |
| +// a drag brings it up. Both halves of that matter: hidden at rest, so |
| +// it costs the list no layout and no tab stop, and a genuine multipart |
| +// POST to /sheets/import, so a drop is the ordinary upload and there is |
| +// no second code path on the server. |
| +func TestTheDropOverlayIsHiddenAtRest(t *testing.T) { |
| + _, tag := dropOverlay(t, signedInListBody(t)) |
| + |
| + for _, want := range []string{ |
| + "hidden", |
| + `method="post"`, |
| + `action="/sheets/import"`, |
| + `enctype="multipart/form-data"`, |
| + } { |
| + if !strings.Contains(tag, want) { |
| + t.Errorf("the drop overlay's start tag is missing %s: %s", want, tag) |
| + } |
| + } |
| +} |
| + |
| +// The overlay is nothing without drop.js: it is `hidden`, and only that |
| +// script ever takes the attribute off. A page that renders the markup |
| +// and does not load the script has an import affordance that can never |
| +// appear. |
| +func TestTheSheetListLoadsDropJS(t *testing.T) { |
| + // By prefix, not by filename: assets are served fingerprinted, so |
| + // the tag reads /static/drop.<hash>.js and matching the literal |
| + // name passes only until the file next changes. |
| + body := signedInListBody(t) |
| + if !strings.Contains(body, `src="/static/drop.`) { |
| + t.Error("the sheet list renders the drop overlay but never loads drop.js, so nothing will ever reveal it") |
| + } |
| +} |
| + |
| +// The paint that keeps the overlay hidden. |
| +// |
| +// [rst-modal-overlay] sets display:flex, and an author declaration beats |
| +// the user agent's [hidden] { display: none } however weak its selector |
| +// looks. Drop the reinstating rule and the overlay covers the sheet list |
| +// permanently — worst with scripting off, where nothing would ever come |
| +// along to hide it again. The markup test above passes either way, so |
| +// this reads the stylesheet. |
| +func TestTheDropOverlayStaysHiddenInCSS(t *testing.T) { |
| + css, err := os.ReadFile(filepath.Join("static", "sheets.css")) |
| + if err != nil { |
| + t.Fatalf("read sheets.css: %v", err) |
| + } |
| + if !strings.Contains(string(css), "[data-drop-page][hidden]") { |
| + t.Error("sheets.css no longer reinstates display:none for the hidden drop overlay — it will cover the sheet list") |
| + } |
| + |
| + // The drop BOX's paint must exclude the overlay, which carries |
| + // data-drop too. [data-drop][data-drop-over] is 0-2-0 against |
| + // [rst-modal-overlay]'s 0-1-0, so without the exclusion the scrim is |
| + // painted accent-soft and the sheet list disappears behind a flat |
| + // blue screen mid-drag. A browser found that; nothing here could. |
| + if !strings.Contains(string(css), "[data-drop]:not([data-drop-page])") { |
| + t.Error("the drop box's paint no longer excludes the overlay — it will out-specify the modal scrim and cover the list") |
| + } |
| +} |
| + |
| +// Create is a real form posting to /sheets, in the top bar. A link |
| +// would be wrong twice over: it makes a sheet, so it must not be a GET, |
| +// and it has to work with scripting off. |
| +func TestCreateLivesInTheTopBar(t *testing.T) { |
| + body := signedInListBody(t) |
| + |
| + bar := body[:strings.Index(body, `<main`)] |
| + if !strings.Contains(bar, `action="/sheets"`) || !strings.Contains(bar, `type="submit"`) { |
| + t.Error("the top bar has no Create form") |
| + } |
| + if !strings.Contains(bar, `href="/sheets/import"`) { |
| + t.Error("the top bar has no Import link") |
| + } |
| + // Beside the account menu, not adrift somewhere else in the shell. |
| + if strings.Index(bar, `action="/sheets"`) > strings.Index(bar, `action="/signout"`) { |
| + t.Error("Create renders after the account menu, not beside it") |
| + } |
| +} |
| + |
| +// The empty state is the one screen with nothing to do but say what to |
| +// do next, and it used to say "make one above" — which pointed at the |
| +// form this change removed. |
| +func TestEmptyStateDoesNotPointAtAFormThatIsGone(t *testing.T) { |
| + body := signedInListBody(t) |
| + if !strings.Contains(body, "No sheets yet") { |
| + t.Fatal("no empty state on a list with no sheets") |
| + } |
| + if strings.Contains(body, "above and it opens") || strings.Contains(body, "Make one above") { |
| + t.Error("the empty state still points at the removed form") |
| + } |
| + if !strings.Contains(body, `href="/sheets/import"`) { |
| + t.Error("the empty state offers no way to import, which is the one route a person with no sheets wants") |
| + } |
| +} |
| + |
| +// /sheets/import must reach the import page, not be parsed as a sheet |
| +// called "import". chi prefers a static segment over {sheetID} whatever |
| +// the registration order, so this passes today — it is pinned because a |
| +// route that only works by resolution order is one refactor from a 404 |
| +// that looks like a missing sheet. |
| +func TestImportPathIsNotASheetID(t *testing.T) { |
| + app, cookies := signIn(t) |
| + rec := do(t, app, as(httptest.NewRequest(http.MethodGet, "/sheets/import", nil), cookies)) |
| + if rec.Code != http.StatusOK { |
| + t.Fatalf("GET /sheets/import = %d, want 200", rec.Code) |
| + } |
| + if !strings.Contains(rec.Body.String(), `action="/sheets/import"`) { |
| + t.Error("/sheets/import did not render the import form") |
| + } |
| +} |
| + |
| +// A refused upload re-renders the import page, where the form is — |
| +// never the sheet list, which no longer has one to try again with. |
| +func TestRefusalRendersOnTheImportPage(t *testing.T) { |
| + app, cookies := signIn(t) |
| + |
| + // An empty multipart body: the shape a submit with nothing chosen |
| + // actually arrives in. A urlencoded post would refuse too, but with |
| + // "could not be read" rather than "choose a spreadsheet", which is |
| + // a different sentence about a different mistake. |
| + var buf bytes.Buffer |
| + w := multipart.NewWriter(&buf) |
| + if err := w.Close(); err != nil { |
| + t.Fatalf("close writer: %v", err) |
| + } |
| + req := httptest.NewRequest(http.MethodPost, "/sheets/import", &buf) |
| + req.Header.Set("Content-Type", w.FormDataContentType()) |
| + |
| + rec := do(t, app, as(req, cookies)) |
| + if rec.Code != http.StatusBadRequest { |
| + t.Fatalf("import with no file = %d, want 400", rec.Code) |
| + } |
| + body := rec.Body.String() |
| + if !strings.Contains(body, refusalNoFile) { |
| + t.Error("the refusal is not on the page") |
| + } |
| + if !strings.Contains(body, `action="/sheets/import"`) { |
| + t.Error("the refusal rendered somewhere without the form to retry from") |
| + } |
| + if strings.Contains(body, "All sheets</h1>") { |
| + t.Error("the refusal rendered on the sheet list") |
| + } |
| +} |
diff --git a/internal/sheets/handlers_rename.go b/internal/sheets/handlers_rename.go| new file mode 100644 |
| index 0000000..f0ddfb1 |
| --- /dev/null |
| +++ b/internal/sheets/handlers_rename.go |
| @@ -0,0 +1,156 @@ |
| +package sheets |
| + |
| +import ( |
| + "errors" |
| + "net/http" |
| + "strconv" |
| + "strings" |
| + |
| + "amadan.net/oficina/sheets/internal/sheets/store" |
| +) |
| + |
| +// Rename exists because Create stopped asking for a name. |
| +// |
| +// The sheet list used to carry a form with a title field, so a sheet |
| +// was named once, at the moment it was made, and never again — there |
| +// was no rename route and layout.html said so in a comment. That was |
| +// survivable only because the title field existed. Removing it in |
| +// favour of a one-click Create, which lands every new sheet as |
| +// "Untitled sheet", would have made the missing route the whole |
| +// product: a list of sheets all called the same thing, distinguished |
| +// only by their preview thumbnails, with no way to fix any of them. |
| +// |
| +// So this is not a feature that arrived beside the topbar Create |
| +// button. It is the other half of it. |
| + |
| +// errBlankTitle is returned rather than silently keeping the old title. |
| +// A person who cleared the field and submitted meant something by it, |
| +// and the honest answer is that a sheet needs a name — the same rule |
| +// createSheet applies. |
| +var errBlankTitle = errors.New("sheets: a sheet needs a title") |
| + |
| +// handleSheetRename renames a sheet from the title in the top bar. |
| +// |
| +// Editors, not just owners: renaming is an edit to the document, of a |
| +// piece with editing a cell, and mayWrite is what says who may make |
| +// one. A viewer or commenter gets the 404 that AGENTS.md requires for |
| +// a row that is not theirs to change — never a 403, which would |
| +// confirm the sheet exists to somebody who may not see it. |
| +func (a *App) handleSheetRename(w http.ResponseWriter, r *http.Request) { |
| + s, ok := a.sheetFromURL(w, r) |
| + if !ok { |
| + return |
| + } |
| + viewer, ok := a.actor(r) |
| + if !ok { |
| + a.notFound(w, r) |
| + return |
| + } |
| + // Bounded, then parsed, and only THEN authorised. The order is the |
| + // point and it is deliberately not the order the rest of the app |
| + // uses. |
| + // |
| + // Every other write route here authorises first and reads the body |
| + // after (openTab before the decode, in handlers_format and |
| + // handlers_size). That leaves the body read — the one step whose |
| + // duration the client chooses — sitting between the permission |
| + // check and the write, so an editor can begin a rename, hold the |
| + // body open while their grant is revoked, then finish it and have |
| + // the write go through. Reading first collapses that: the delay a |
| + // client controls now happens BEFORE anything is checked, and the |
| + // gap between mayWrite and the UPDATE is straight-line code. |
| + // |
| + // It is not free — a request that turns out to be forbidden has |
| + // paid to parse its own body — but the cap on the line below is |
| + // what makes that cheap enough to be the obvious trade. The wider |
| + // question, whether every other write route should be reordered |
| + // the same way, is a real one and belongs in the branch discussion |
| + // rather than in this handler. |
| + // |
| + // 8 KiB matches handlers_format's cap on the same shape of form |
| + // post. Go's own ParseForm ceiling for a urlencoded body is 10 MiB, |
| + // so this is not the difference between bounded and unbounded — it |
| + // is three orders of magnitude off a limit no rename needs, for a |
| + // value truncated to maxTitleRunes moments later. |
| + r.Body = http.MaxBytesReader(w, r.Body, 8<<10) |
| + if err := r.ParseForm(); err != nil { |
| + // Refused rather than carried on with. FormValue would swallow |
| + // this error and fall through to the QUERY, which is how |
| + // ?title=x on an oversized body silently became the new name — |
| + // caught by review, not by the suite. |
| + http.Error(w, "Malformed request.", http.StatusBadRequest) |
| + return |
| + } |
| + // PostForm, never Form: the title comes from the body a person |
| + // submitted, never from the URL. r.FormValue merges the query in, |
| + // so with it a GET-shaped ?title= could rename a sheet — a |
| + // state-changing value arriving somewhere it can be linked to. |
| + // |
| + // Read field by field into a local, never bound onto a GORM model |
| + // (AGENTS.md / Mechanical rules): a form that could reach the model |
| + // could set CreatedBy, or DeletedAt, or an id. |
| + title := r.PostForm.Get("title") |
| + |
| + // Last thing before the write, with nothing client-paced in |
| + // between. See the note above on why this moved down here. |
| + if !a.mayWrite(s.ID, viewer) { |
| + a.notFound(w, r) |
| + return |
| + } |
| + |
| + if err := a.renameSheet(s.ID, title); err != nil { |
| + if errors.Is(err, errBlankTitle) { |
| + // Straight back to the sheet, which still shows the name it |
| + // had. Nothing was changed and there is nothing to explain |
| + // that the unchanged title does not already say. |
| + a.sheetRedirect(w, r, s.ID) |
| + return |
| + } |
| + a.fail(w, r, err) |
| + return |
| + } |
| + // 303 rather than a render: a POST that renders is a POST that |
| + // resubmits on reload. |
| + a.sheetRedirect(w, r, s.ID) |
| +} |
| + |
| +// sheetRedirect sends the browser back to a sheet. |
| +func (a *App) sheetRedirect(w http.ResponseWriter, r *http.Request, id uint64) { |
| + http.Redirect(w, r, "/sheets/"+strconv.FormatUint(id, 10), http.StatusSeeOther) |
| +} |
| + |
| +// renameSheet writes a new sealed title. |
| +// |
| +// The title is sealed under the same "sheet.title" purpose createSheet |
| +// uses. A rename that wrote plaintext would put a sheet's name in the |
| +// database file, and the one rule does not have an exception for the |
| +// second time a name is set. |
| +// |
| +// Update names its column explicitly rather than saving a struct, so |
| +// this statement cannot touch a field nobody asked it to. GORM bumps |
| +// UpdatedAt as part of it, which is what makes a renamed sheet rise to |
| +// the top of the list — correct, since renaming it is changing it. |
| +func (a *App) renameSheet(sheetID uint64, title string) error { |
| + if a.db == nil { |
| + return errNoDB |
| + } |
| + if a.sealer == nil { |
| + return errNoSealer |
| + } |
| + clean := strings.TrimSpace(title) |
| + if clean == "" { |
| + return errBlankTitle |
| + } |
| + // Truncated to the same limit createSheet applies, by runes rather |
| + // than bytes so a multi-byte title is cut where a character ends. |
| + if r := []rune(clean); len(r) > maxTitleRunes { |
| + clean = string(r[:maxTitleRunes]) |
| + } |
| + sealed, err := a.sealer.Seal("sheet.title", []byte(clean)) |
| + if err != nil { |
| + return err |
| + } |
| + return a.db.Model(&store.Sheet{}). |
| + Where("id = ?", sheetID). |
| + Update("title_sealed", sealed).Error |
| +} |
diff --git a/internal/sheets/handlers_rename_test.go b/internal/sheets/handlers_rename_test.go| new file mode 100644 |
| index 0000000..fa05307 |
| --- /dev/null |
| +++ b/internal/sheets/handlers_rename_test.go |
| @@ -0,0 +1,263 @@ |
| +package sheets |
| + |
| +import ( |
| + "bytes" |
| + "net/http" |
| + "net/url" |
| + "strconv" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// Rename is the other half of a Create button that stopped asking for a |
| +// name. These tests are written against that pairing rather than |
| +// against the route on its own: the failure they exist to catch is a |
| +// list of sheets all called "Untitled sheet" with no way to fix any of |
| +// them. |
| + |
| +func renamePath(id uint64) string { |
| + return "/sheets/" + strconv.FormatUint(id, 10) + "/rename" |
| +} |
| + |
| +func TestRenameChangesTheTitle(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Untitled sheet") |
| + |
| + rec := do(t, app, as(postForm(renamePath(s.ID), |
| + url.Values{"title": {"Q3 budget"}}), cookies)) |
| + if rec.Code != http.StatusSeeOther { |
| + t.Fatalf("rename = %d, want 303: %s", rec.Code, rec.Body.String()) |
| + } |
| + if loc := rec.Header().Get("Location"); loc != "/sheets/"+strconv.FormatUint(s.ID, 10) { |
| + t.Errorf("rename redirected to %q, want the sheet", loc) |
| + } |
| + |
| + got, err := app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "Q3 budget" { |
| + t.Errorf("title is %q after rename, want %q", got.Title, "Q3 budget") |
| + } |
| +} |
| + |
| +// The one rule reaches the second time a name is set, not just the |
| +// first. A rename that wrote the new title straight into the column |
| +// would put a sheet's name in the database file in plaintext, and |
| +// nothing about createSheet's sealing would have caught it. |
| +func TestRenamedTitleIsSealedAtRest(t *testing.T) { |
| + const secret = "Redundancies Q4" |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Untitled sheet") |
| + |
| + if rec := do(t, app, as(postForm(renamePath(s.ID), |
| + url.Values{"title": {secret}}), cookies)); rec.Code != http.StatusSeeOther { |
| + t.Fatalf("rename = %d", rec.Code) |
| + } |
| + |
| + var raw []byte |
| + if err := app.db.Raw("SELECT title_sealed FROM sheets WHERE id = ?", s.ID). |
| + Row().Scan(&raw); err != nil { |
| + t.Fatalf("read back: %v", err) |
| + } |
| + if len(raw) == 0 { |
| + t.Fatal("no sealed title was stored") |
| + } |
| + if bytes.Contains(raw, []byte(secret)) { |
| + t.Fatal("the renamed title reached the database in plaintext") |
| + } |
| +} |
| + |
| +// A blank title keeps the name the sheet had. The alternative is a |
| +// sheet called nothing at all, which the list would render as a row you |
| +// cannot click by name. |
| +func TestRenameRefusesABlankTitle(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + for _, blank := range []string{"", " "} { |
| + rec := do(t, app, as(postForm(renamePath(s.ID), |
| + url.Values{"title": {blank}}), cookies)) |
| + if rec.Code != http.StatusSeeOther { |
| + t.Fatalf("rename to %q = %d, want 303", blank, rec.Code) |
| + } |
| + got, err := app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "Budget" { |
| + t.Errorf("a blank rename changed the title to %q", got.Title) |
| + } |
| + } |
| +} |
| + |
| +// Truncation matches createSheet's, and by runes: cutting a multi-byte |
| +// title at a byte boundary stores half a character. |
| +func TestRenameTruncatesToTheSameCapAsCreate(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + long := strings.Repeat("é", maxTitleRunes+50) |
| + if rec := do(t, app, as(postForm(renamePath(s.ID), |
| + url.Values{"title": {long}}), cookies)); rec.Code != http.StatusSeeOther { |
| + t.Fatalf("rename = %d", rec.Code) |
| + } |
| + got, err := app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if n := len([]rune(got.Title)); n != maxTitleRunes { |
| + t.Errorf("title is %d runes after rename, want %d", n, maxTitleRunes) |
| + } |
| +} |
| + |
| +// The body is capped before it is parsed. |
| +// |
| +// An independent review pass found rename reading its form with nothing |
| +// bounding the request — the only write route in the app doing so. |
| +// FormValue buffers the WHOLE body to pull one field out of it, so |
| +// without the cap a megabyte of "title=" is a megabyte held in memory |
| +// on the way to a value that gets truncated to maxTitleRunes anyway. |
| +// |
| +// An oversized body is REFUSED, not quietly ignored. The parse error is |
| +// checked rather than swallowed, which is the difference between this |
| +// and the first attempt at the fix — see the query test below for what |
| +// swallowing it cost. |
| +func TestRenameCapsTheBodyItWillParse(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + // Well past the 8 KiB cap, as one enormous field value. |
| + huge := url.Values{"title": {strings.Repeat("x", 64<<10)}} |
| + rec := do(t, app, as(postForm(renamePath(s.ID), huge), cookies)) |
| + if rec.Code != http.StatusBadRequest { |
| + t.Fatalf("oversized rename = %d, want 400: %s", rec.Code, rec.Body.String()) |
| + } |
| + got, err := app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "Budget" { |
| + t.Errorf("an oversized body renamed the sheet to %q; the cap is not there", got.Title) |
| + } |
| +} |
| + |
| +// The title comes from the body, never from the URL. |
| +// |
| +// This is the defect the second review pass found in the first fix. Go's |
| +// r.FormValue merges the query string into the form, and it swallows a |
| +// parse failure rather than reporting it — so capping the body without |
| +// checking the error created a NEW way in: POST ?title=Wrong with an |
| +// oversized body, the cap rejects the body, FormValue falls through to |
| +// the query, and the sheet is renamed to whatever the URL said. A |
| +// state-changing value that can travel in a link is exactly what a POST |
| +// route must not accept. |
| +// |
| +// Both halves are asserted, because either alone would pass a broken |
| +// handler: the query is ignored when the body is fine, and an oversized |
| +// body is refused outright rather than falling back to it. |
| +func TestRenameTakesTheTitleFromTheBodyNotTheQuery(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + // A good body and a hostile query: the body must win. |
| + req := postForm(renamePath(s.ID)+"?title=FromTheQuery", |
| + url.Values{"title": {"FromTheBody"}}) |
| + if rec := do(t, app, as(req, cookies)); rec.Code != http.StatusSeeOther { |
| + t.Fatalf("rename = %d, want 303: %s", rec.Code, rec.Body.String()) |
| + } |
| + got, err := app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "FromTheBody" { |
| + t.Errorf("title is %q, want the body's value — the query reached the write", got.Title) |
| + } |
| + |
| + // A body carrying no title at all, and a query that does. This is |
| + // the case that actually distinguishes PostForm from FormValue, and |
| + // it exists because the first version of this test did not: with |
| + // ParseForm's error checked, a well-formed body wins under either |
| + // spelling, so swapping PostForm back for FormValue passed. Here |
| + // r.Form merges the query in and FormValue would answer |
| + // "FromTheQuery"; PostForm answers "", which is blank, which keeps |
| + // the name the sheet had. |
| + req = postForm(renamePath(s.ID)+"?title=FromTheQuery", |
| + url.Values{"unrelated": {"x"}}) |
| + if rec := do(t, app, as(req, cookies)); rec.Code != http.StatusSeeOther { |
| + t.Fatalf("rename with no body title = %d, want 303", rec.Code) |
| + } |
| + got, err = app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "FromTheBody" { |
| + t.Errorf("title is %q; the query renamed the sheet without a body field", got.Title) |
| + } |
| + |
| + // An oversized body with a query title: refused, and the query must |
| + // not stand in for the body it could not read. |
| + huge := url.Values{"title": {strings.Repeat("x", 64<<10)}} |
| + req = postForm(renamePath(s.ID)+"?title=FromTheQuery", huge) |
| + if rec := do(t, app, as(req, cookies)); rec.Code != http.StatusBadRequest { |
| + t.Fatalf("oversized rename = %d, want 400", rec.Code) |
| + } |
| + got, err = app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "FromTheBody" { |
| + t.Errorf("title is %q; a rejected body let the query rename the sheet", got.Title) |
| + } |
| +} |
| + |
| +// A viewer may not rename, and is told 404 rather than 403 — AGENTS.md |
| +// requires it, because a 403 confirms the sheet exists to somebody who |
| +// was never shown it. |
| +func TestRenameIsNotForViewers(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + guestOn(t, app, s.ID, "ada@elsewhere.example", SheetViewer) |
| + guest := signInAs(t, app, "ada@elsewhere.example") |
| + |
| + rec := do(t, app, as(postForm(renamePath(s.ID), |
| + url.Values{"title": {"Mine now"}}), guest)) |
| + if rec.Code != http.StatusNotFound { |
| + t.Errorf("a viewer renaming = %d, want 404", rec.Code) |
| + } |
| + |
| + got, err := app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "Budget" { |
| + t.Errorf("a viewer renamed the sheet to %q", got.Title) |
| + } |
| +} |
| + |
| +// TestCreateFromTheTopBarLandsAnUntitledSheet drives the button the |
| +// list page now carries: an empty POST, no title field anywhere in it. |
| +// |
| +// This is the whole reason rename exists, so it is pinned here rather |
| +// than left to the create handler's own tests: if an empty create ever |
| +// starts refusing instead of defaulting, the top bar's only action |
| +// breaks and the failure should name the button. |
| +func TestCreateFromTheTopBarLandsAnUntitledSheet(t *testing.T) { |
| + app, cookies := signIn(t) |
| + |
| + rec := do(t, app, as(postForm("/sheets", url.Values{}), cookies)) |
| + if rec.Code != http.StatusSeeOther { |
| + t.Fatalf("create with no title = %d, want 303: %s", rec.Code, rec.Body.String()) |
| + } |
| + id, err := strconv.ParseUint(strings.TrimPrefix(rec.Header().Get("Location"), "/sheets/"), 10, 64) |
| + if err != nil { |
| + t.Fatalf("Location %q is not a sheet URL: %v", rec.Header().Get("Location"), err) |
| + } |
| + got, err := app.sheetSummary(id, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "Untitled sheet" { |
| + t.Errorf("a titleless create landed as %q, want %q", got.Title, "Untitled sheet") |
| + } |
| +} |
diff --git a/internal/sheets/handlers_sheets.go b/internal/sheets/handlers_sheets.go| index 88f122e..2c79762 100644 |
| --- a/internal/sheets/handlers_sheets.go |
| +++ b/internal/sheets/handlers_sheets.go |
| @@ -19,15 +19,14 @@ type sheetRow struct { |
| } |
| // sheetListView is the sheet list's payload. |
| +// |
| +// It carried an ImportError until the import form moved off this page |
| +// to /sheets/import. The reasoning for keeping a refusal on the same |
| +// page as its form was right and still is — it needs no flash, no |
| +// cookie and no redirect — which is why the field moved with the form |
| +// rather than being dropped: it is importPageView.Error now. |
| type sheetListView struct { |
| Sheets []sheetRow |
| - // ImportError is a refused upload's explanation, empty on an |
| - // ordinary render. It lives on the list rather than in a flash |
| - // because the list is where the import form is: a refusal shown |
| - // beside the form that caused it needs no session storage, no |
| - // redirect and no cookie, and it works with scripting off by |
| - // construction. |
| - ImportError string |
| } |
| // sheetPageView is the grid page's payload. |
| @@ -79,18 +78,18 @@ type sheetPageView struct { |
| // handleSheetList renders the sheets the viewer may see. |
| func (a *App) handleSheetList(w http.ResponseWriter, r *http.Request) { |
| - a.renderSheetList(w, r, http.StatusOK, "") |
| + a.renderSheetList(w, r, http.StatusOK) |
| } |
| -// renderSheetList draws the list, optionally carrying a refused |
| -// upload's explanation and the status that goes with it. |
| +// renderSheetList draws the list. |
| // |
| -// A refused import re-renders this page rather than redirecting to it, |
| -// so the refusal survives without a flash: the person lands back on the |
| -// list they posted from, with the reason beside the form. The status is |
| -// the caller's because only the caller knows whether this is an |
| -// ordinary 200 or a refusal. |
| -func (a *App) renderSheetList(w http.ResponseWriter, r *http.Request, status int, importErr string) { |
| +// It used to take a refused upload's explanation and a status to go |
| +// with it, because the import form lived on this page and a refusal |
| +// belongs beside the form that caused it. The form moved to |
| +// /sheets/import and the refusal went with it — renderImportPage keeps |
| +// both the parameter and the reasoning. What is left here has one |
| +// caller and one status, so it no longer takes either. |
| +func (a *App) renderSheetList(w http.ResponseWriter, r *http.Request, status int) { |
| viewer, ok := a.actor(r) |
| if !ok { |
| http.Error(w, "Sign in first.", http.StatusForbidden) |
| @@ -105,8 +104,8 @@ func (a *App) renderSheetList(w http.ResponseWriter, r *http.Request, status int |
| for _, s := range list { |
| rows = append(rows, sheetRow{SheetSummary: s, Preview: a.previewFor(s)}) |
| } |
| - a.render(w, r, "sheets", "Your sheets — Oficina Sheets", status, |
| - sheetListView{Sheets: rows, ImportError: importErr}) |
| + a.render(w, r, "sheets", "All sheets — Oficina Sheets", status, |
| + sheetListView{Sheets: rows}) |
| } |
| // handleSheetCreate makes a sheet and sends you to it. |
diff --git a/internal/sheets/handlers_table_test.go b/internal/sheets/handlers_table_test.go| index c6d4dad..a9a28dc 100644 |
| --- a/internal/sheets/handlers_table_test.go |
| +++ b/internal/sheets/handlers_table_test.go |
| @@ -129,8 +129,19 @@ func TestEveryPageWorksWithoutScript(t *testing.T) { |
| // Sign-out is checked on /sheets rather than on "/": it is |
| // layout chrome, present on every signed-in page, and "/" |
| // stopped being one when it started redirecting onward. |
| - {"/sheets", []string{`action="/sheets"`, `name="title"`, `action="/signout"`}}, |
| - {fmt.Sprintf("/sheets/%d", s.ID), []string{fmt.Sprintf("/sheets/%d/table", s.ID)}}, |
| + // |
| + // The list no longer carries name="title": Create is a button |
| + // in the top bar that posts an empty form and lands the sheet |
| + // as "Untitled sheet". The naming moved to the rename field on |
| + // the sheet page, two rows down, which is where the title is |
| + // now asserted. |
| + {"/sheets", []string{`action="/sheets"`, `type="submit"`, `action="/signout"`}}, |
| + {"/sheets/import", []string{`action="/sheets/import"`, `type="file"`, `type="submit"`}}, |
| + {fmt.Sprintf("/sheets/%d", s.ID), []string{ |
| + fmt.Sprintf("/sheets/%d/table", s.ID), |
| + // Rename: the only place a sheet is named now, so it is |
| + // the one that has to survive with scripting off. |
| + fmt.Sprintf(`action="/sheets/%d/rename"`, s.ID), `name="title"`}}, |
| {fmt.Sprintf("/sheets/%d/table", s.ID), []string{"?page=1&edit=0,0", "Next rows"}}, |
| {fmt.Sprintf("/sheets/%d/table?page=1&edit=0,0", s.ID), |
| []string{`method="post"`, `name="raw"`, "Save"}}, |
diff --git a/internal/sheets/static/drop.js b/internal/sheets/static/drop.js| index 30baa12..3a89703 100644 |
| --- a/internal/sheets/static/drop.js |
| +++ b/internal/sheets/static/drop.js |
| @@ -1,4 +1,11 @@ |
| -/* Drag and drop for the import form. |
| +/* Drag and drop for the import form, in both places it appears. |
| + * |
| + * On /sheets/import the zone is a box in the page. On the sheet list it |
| + * is an overlay that stays `hidden` until a drag carrying files is |
| + * actually over the window — the list carries no visible drop box, |
| + * because a list of sheets should be a list of sheets. Which shape it |
| + * is comes from data-drop-page on the zone, and it changes only where |
| + * the events are listened for and whether the zone is revealed. |
| * |
| * This file adds no capability. The form it decorates already imports a |
| * spreadsheet with scripting off, and every drop here ends as that same |
| @@ -32,8 +39,32 @@ |
| if (typeof window.DataTransfer !== 'function') return; |
| var hint = zone.querySelector('[data-drop-hint]'); |
| + var hintText = hint ? hint.textContent : ''; |
| var depth = 0; |
| + /* Two shapes, one behaviour. |
| + * |
| + * On /sheets/import the zone is a box in the page and the events |
| + * belong to it. On the sheet list there is no box — a list of sheets |
| + * should be a list of sheets — so the zone is an overlay that is |
| + * `hidden` until a drag carrying files enters the window, and the |
| + * events belong to the document because a hidden element receives |
| + * none. Everything after this line is the same either way. */ |
| + var page = zone.hasAttribute('data-drop-page'); |
| + var source = page ? document : zone; |
| + |
| + function reveal() { if (page) zone.hidden = false; } |
| + |
| + /* dismiss puts the hint back as well. Otherwise a refusal ("drop one |
| + * file at a time") is still sitting there the next time the overlay |
| + * comes up, answering a drag that has not happened yet. */ |
| + function dismiss() { |
| + if (!page) return; |
| + zone.hidden = true; |
| + zone.removeAttribute('data-drop-over'); |
| + say(hintText); |
| + } |
| + |
| /* dragenter/dragleave fire for every child element the pointer |
| * crosses, so a plain enter/leave pair flickers the whole time a |
| * file is moved across the box. Counting depth is what makes the |
| @@ -42,6 +73,7 @@ |
| if (!hasFiles(e)) return; |
| e.preventDefault(); |
| depth++; |
| + reveal(); |
| zone.setAttribute('data-drop-over', ''); |
| } |
| @@ -49,7 +81,10 @@ |
| if (!hasFiles(e)) return; |
| e.preventDefault(); |
| depth = Math.max(0, depth - 1); |
| - if (depth === 0) zone.removeAttribute('data-drop-over'); |
| + if (depth === 0) { |
| + zone.removeAttribute('data-drop-over'); |
| + dismiss(); |
| + } |
| } |
| /* dragover must preventDefault on every tick or the browser refuses |
| @@ -117,8 +152,23 @@ |
| if (hint) hint.textContent = text; |
| } |
| - zone.addEventListener('dragenter', enter); |
| - zone.addEventListener('dragleave', leave); |
| - zone.addEventListener('dragover', over); |
| - zone.addEventListener('drop', drop); |
| + source.addEventListener('dragenter', enter); |
| + source.addEventListener('dragleave', leave); |
| + source.addEventListener('dragover', over); |
| + source.addEventListener('drop', drop); |
| + |
| + /* A refused drop leaves the overlay up on purpose — the message is |
| + * the whole point of refusing in the client's own words, and the |
| + * chooser beside it is how the person recovers. So the overlay needs |
| + * a way out that is not the back button: Escape, or a click on the |
| + * backdrop outside the panel. Both are what every other dismissible |
| + * layer in this app answers to, so neither has to be taught. */ |
| + if (page) { |
| + document.addEventListener('keydown', function (e) { |
| + if (e.key === 'Escape' && !zone.hidden) dismiss(); |
| + }); |
| + zone.addEventListener('click', function (e) { |
| + if (e.target === zone) dismiss(); |
| + }); |
| + } |
| })(); |
diff --git a/internal/sheets/static/sheets.css b/internal/sheets/static/sheets.css| index 81a8c4c..be9c11d 100644 |
| --- a/internal/sheets/static/sheets.css |
| +++ b/internal/sheets/static/sheets.css |
| @@ -65,18 +65,120 @@ |
| color: var(--rst-text); |
| } |
| +/* A page's own actions in the top bar, against the account menu. |
| + * |
| + * The auto margin has to live HERE and be taken off the account, not |
| + * added alongside it. rastrillo gives [rst-shell-account] a |
| + * margin-inline-start: auto, which absorbs all the bar's free space and |
| + * would leave Create stranded back beside the nav — measured, not |
| + * guessed: that is exactly how it first rendered. A second auto margin |
| + * does not fix it either, because flexbox splits free space equally |
| + * between auto margins and the buttons end up mid-bar. |
| + * |
| + * Only above 800px. Below it rastrillo stacks the tail into a column |
| + * behind the Menu disclosure and zeroes that same margin itself, and an |
| + * auto inline margin in a column flex box shoves one item to the |
| + * trailing edge — the bug the framework's own comment at that rule |
| + * describes. Same breakpoint as the framework's, deliberately: two |
| + * shells with two breakpoints is two systems. */ |
| +[data-topbar-actions] { |
| + display: flex; |
| + align-items: center; |
| + gap: var(--rst-sp-2); |
| +} |
| + |
| +/* Both selectors are written against the framework's own shape, and |
| + * both have to out-specify it. rastrillo re-applies the account's auto |
| + * margin INSIDE this same media query at |
| + * [rst-shell-topbar] [rst-shell-tail] > [rst-shell-account], which is |
| + * 0-3-0; a plain [data-topbar-actions] ~ [rst-shell-account] is 0-2-0 |
| + * and loses, which is not a subtle failure — the buttons come to rest |
| + * dead centre of the bar, because the two auto margins then split the |
| + * free space between them. Measured, twice. |
| + * |
| + * The tail is display: contents up here, so these are siblings in the |
| + * bar's own flex line even though they are nested in the markup. */ |
| +@media (min-width: 800px) { |
| + [rst-shell-topbar] [rst-shell-tail] > [data-topbar-actions] { |
| + margin-inline-start: auto; |
| + } |
| + [rst-shell-topbar] [rst-shell-tail] > [data-topbar-actions] ~ [rst-shell-account] { |
| + margin-inline-start: 0; |
| + } |
| +} |
| + |
| /* The document title, in the top bar beside the brand. It truncates |
| * rather than wrapping: the bar is a fixed height, and a two-line title |
| - * would push the account menu out of it. */ |
| + * would push the account menu out of it. |
| + * |
| + * The clipping lives on the WRAPPER, not on the text or the input. It |
| + * used to sit directly on [data-doc-title] back when that was a span, |
| + * and left there it would clip a rename input's own caret: type past |
| + * 22rem and the cursor walks out of a box with overflow: hidden, so you |
| + * are editing text you cannot see. */ |
| [data-doc-title] { |
| color: var(--rst-text); |
| font-weight: 600; |
| max-inline-size: 22rem; |
| +} |
| + |
| +[data-doc-title-text] { |
| + display: block; |
| overflow: hidden; |
| text-overflow: ellipsis; |
| white-space: nowrap; |
| } |
| +/* The rename form. It reads as a title until somebody means to edit it: |
| + * no border, no field background, the same weight and colour as the |
| + * static version above. This is the one control on the page that has to |
| + * NOT look like a control at rest — a text input sitting permanently in |
| + * the top bar of a spreadsheet competes with the grid for "the thing |
| + * you type in", and the grid must win by default. */ |
| +form[data-doc-title] { |
| + display: flex; |
| + align-items: center; |
| + gap: var(--rst-sp-1); |
| +} |
| + |
| +[data-doc-title-input] { |
| + min-inline-size: 0; |
| + padding: var(--rst-sp-1); |
| + border: 1px solid transparent; |
| + border-radius: var(--rst-radius); |
| + background: none; |
| + color: inherit; |
| + font: inherit; |
| + text-overflow: ellipsis; |
| +} |
| + |
| +[data-doc-title-input]:hover { |
| + border-color: var(--rst-line); |
| +} |
| + |
| +/* Focus turns it into an obvious field. :focus-visible is not enough |
| + * here — a click into the box must show the same affordance a Tab into |
| + * it does, because at rest the two are indistinguishable. */ |
| +[data-doc-title-input]:focus { |
| + border-color: var(--rst-accent); |
| + background: var(--rst-surface); |
| + outline: none; |
| + text-overflow: clip; |
| +} |
| + |
| +/* The submit is hidden until the field is engaged, and hidden with |
| + * clip rather than display: none so it stays focusable — Tab from the |
| + * input must reach it. Scripting off, this button is the only way to |
| + * commit a rename that was not submitted with Enter. */ |
| +form[data-doc-title]:not(:focus-within) button { |
| + position: absolute; |
| + inline-size: 1px; |
| + block-size: 1px; |
| + overflow: hidden; |
| + clip-path: inset(50%); |
| + white-space: nowrap; |
| +} |
| + |
| [data-menubar] { |
| display: flex; |
| align-items: center; |
| @@ -925,14 +1027,21 @@ |
| * page crosses this box repeatedly, and animating the outline is the |
| * difference between a box that acknowledges the drag and one that |
| * flashes. */ |
| -[data-drop] { |
| +/* :not([data-drop-page]) on both, because there are two zones now and |
| + only one of them is a box. The overlay on the sheet list carries |
| + data-drop too — it has to, it is what drop.js looks for — and these |
| + two selectors are 0-2-0 and 0-3-0, so without the exclusion they beat |
| + [rst-modal-overlay]'s 0-1-0 and paint the sheet of glass over the |
| + whole page in accent-soft. It renders as a flat blue screen with the |
| + list gone: a defect a browser found and no test could. */ |
| +[data-drop]:not([data-drop-page]) { |
| border: 1px dashed var(--rst-line); |
| border-radius: var(--rst-radius); |
| padding: var(--rst-sp-3); |
| transition: border-color 120ms ease, background-color 120ms ease; |
| } |
| -[data-drop][data-drop-over] { |
| +[data-drop][data-drop-over]:not([data-drop-page]) { |
| border-color: var(--rst-accent); |
| background: var(--rst-accent-soft); |
| } |
| @@ -945,6 +1054,77 @@ |
| } |
| } |
| +/* ── The sheet list's drop overlay ─────────────────────────────────── |
| + * |
| + * The list has no drop box. It has a full-window target that does not |
| + * exist until a file is over the window, and drop.js is what reveals |
| + * it — see the comment on [data-drop-page] in sheets.html. |
| + * |
| + * It borrows rastrillo's modal paint rather than growing its own, so |
| + * the layer that appears over the list looks like every other layer in |
| + * the suite. Only the four rules below are ours. |
| + * |
| + * THIS FIRST RULE IS LOAD-BEARING. [rst-modal-overlay] sets |
| + * display: flex, and an author declaration beats the user agent's |
| + * [hidden] { display: none } no matter how weak its selector looks. |
| + * Without it the overlay covers the sheet list permanently and the page |
| + * is unusable — with scripting off, where nothing would ever come along |
| + * to hide it again. TestTheDropOverlayIsHiddenAtRest pins the markup; |
| + * this pins the paint. */ |
| +[data-drop-page][hidden] { |
| + display: none; |
| +} |
| + |
| +[data-drop-page] [rst-modal-panel] { |
| + max-width: 24rem; |
| + transition: border-color 120ms ease; |
| +} |
| + |
| +/* The panel answers the drag, the same way the box on /sheets/import |
| + does and with the same token, so the two read as one gesture. */ |
| +[data-drop-page][data-drop-over] [rst-modal-panel] { |
| + border-color: var(--rst-accent); |
| +} |
| + |
| +[data-drop-page-body] { |
| + display: grid; |
| + gap: var(--rst-sp-3); |
| + justify-items: center; |
| + padding: var(--rst-sp-5); |
| + text-align: center; |
| + width: 100%; |
| +} |
| + |
| +[data-drop-page-title] { |
| + font-size: var(--rst-fs-base); |
| + font-weight: 600; |
| + margin: 0; |
| +} |
| + |
| +/* The chooser is for afterwards. |
| + * |
| + * While a file is over the window the panel has one thing to say, and a |
| + * "Choose File" button beside "Drop it to import" offers a second way to |
| + * do the thing the person is already doing. It appears when the overlay |
| + * is up WITHOUT a drag over it, which is exactly the state a refused |
| + * drop leaves behind — and then it is the way out of that refusal. */ |
| +[data-drop-page-recover] { |
| + display: grid; |
| + gap: var(--rst-sp-3); |
| + justify-items: center; |
| + width: 100%; |
| +} |
| + |
| +[data-drop-page][data-drop-over] [data-drop-page-recover] { |
| + display: none; |
| +} |
| + |
| +@media (prefers-reduced-motion: reduce) { |
| + [data-drop-page] [rst-modal-panel] { |
| + transition: none; |
| + } |
| +} |
| + |
| /* ── Cell formatting ───────────────────────────────────────────────── |
| * |
| * The paint for a cell's stored Format: bold, italic, alignment, fill |
diff --git a/internal/sheets/templates/import.html b/internal/sheets/templates/import.html| new file mode 100644 |
| index 0000000..c3ec8da |
| --- /dev/null |
| +++ b/internal/sheets/templates/import.html |
| @@ -0,0 +1,53 @@ |
| +{{define "head"}} |
| +{{/* Drag and drop only. Everything the drop zone does, the form below |
| + it already does with scripting off — drop.js fills that same file |
| + input and submits that same form, so there is one route, one CSRF |
| + check and one code path either way. |
| + |
| + This moved here with the form it decorates. It used to sit on the |
| + sheet list, which is where the import form used to be. */}} |
| +<script defer src="{{asset "static/drop.js"}}"></script> |
| +{{end}} |
| + |
| +{{define "content"}} |
| +<header rst-page-header> |
| +<div rst-page-header-titles> |
| +<h1>Import a spreadsheet</h1> |
| +{{/* The limits are stated BEFORE the import, not discovered after it. |
| + Import reads one worksheet (xlsx.Import), and a workbook with |
| + three of them would otherwise arrive looking whole — the quiet |
| + kind of wrong PRODUCT.md's first principle forbids. |
| + |
| + They are a subhead here rather than a hint under the field |
| + because this page has the room the dropdown they nearly went in |
| + did not, and because drop.js overwrites the hint to speak about |
| + a drop — which would take the file limits down with it. */}} |
| +<p rst-page-header-sub>Excel .xlsx or .xlsm, up to 20 MB. The first |
| +worksheet is imported.</p> |
| +</div> |
| +</header> |
| + |
| +{{if .Data.Error}} |
| +{{/* A refusal renders HERE, on the page holding the form, rather than |
| + back on the sheet list. The list is not where the person is and |
| + no longer carries a form to try again with. */}} |
| +<div rst-callout rst-tone="negative"> |
| +<div rst-callout-body> |
| +<p>{{.Data.Error}}</p> |
| +</div> |
| +</div> |
| +{{end}} |
| + |
| +<form rst-form method="post" action="/sheets/import" enctype="multipart/form-data"> |
| +<div data-drop> |
| +<div rst-field-row> |
| +<div rst-field> |
| +<label rst-field-label for="file">Choose a file</label> |
| +<input rst-input id="file" name="file" type="file" accept=".xlsx,.xlsm"> |
| +<p rst-field-hint data-drop-hint>You can also drag a file onto this box.</p> |
| +</div> |
| +<button rst-btn="primary" type="submit">Import</button> |
| +</div> |
| +</div> |
| +</form> |
| +{{end}} |
diff --git a/internal/sheets/templates/layout.html b/internal/sheets/templates/layout.html| index 6920a26..9ab26a8 100644 |
| --- a/internal/sheets/templates/layout.html |
| +++ b/internal/sheets/templates/layout.html |
| @@ -75,9 +75,28 @@ |
| </details> |
| {{end}} |
| <div rst-shell-tail> |
| -<nav rst-shell-nav> |
| -{{if .SignedIn}}<a href="/sheets">Your sheets</a>{{end}} |
| -</nav> |
| +{{/* There is no nav here, and the empty landmark went with the link. |
| + |
| + It held one item, "Your sheets", pointing at /sheets — which is |
| + where the brand two elements to the left already goes: handleIndex |
| + 303s a signed-in visitor from / to /sheets. So the bar read |
| + "Sheets ... Your sheets" and both were the same destination under |
| + two names, which is a menu of one choice offered twice. |
| + |
| + The brand is the way back to the list. That is what a wordmark in |
| + the top-left is for, and it is one fewer thing to read. */}} |
| +{{/* A page's own top-bar actions, beside the account menu. |
| + |
| + Deliberately a block rather than a field on PageData, and |
| + deliberately empty by default: the sheet list fills it with Create |
| + and Import, and the grid page fills it with nothing, because the |
| + grid already carries a File menu holding both and a second Create |
| + in the bar above it would be the same verb twice on one screen. |
| + |
| + Inside the tail, so it collapses behind the narrow-viewport Menu |
| + disclosure along with the nav and the account rather than being |
| + the one control still floating in the bar at 360px. */}} |
| +{{block "topbaractions" .}}{{end}} |
| {{if .SignedIn}} |
| <details rst-dropdown rst-shell-account name="rst-menus"> |
| <summary>{{.Address}}<span rst-caret aria-hidden="true">{{icon "chevron-down"}}</span></summary> |
diff --git a/internal/sheets/templates/sheet.html b/internal/sheets/templates/sheet.html| index e29d0d0..88091f4 100644 |
| --- a/internal/sheets/templates/sheet.html |
| +++ b/internal/sheets/templates/sheet.html |
| @@ -23,7 +23,37 @@ |
| {{end}} |
| {{define "doctitle"}} |
| -<span data-doc-title title="{{.Data.Sheet.Title}}">{{.Data.Sheet.Title}}</span> |
| +{{/* The open document's title, and — for anyone who may edit it — the |
| + rename. |
| + |
| + This was static text, under a comment saying a rename route did |
| + not exist and that anything looking editable would be a promise |
| + the app could not keep. The route exists now, and it had to: the |
| + sheet list's Create button stopped asking for a title, so every |
| + sheet is born "Untitled sheet" and this is the only place the |
| + name is ever fixed. See handlers_rename.go. |
| + |
| + An inline form, not a popover. A <details> here would join |
| + rastrillo's "rst-menus" exclusive group along with the File menu |
| + and the account dropdown — opening one would close the others, |
| + and a rename that shuts the menu you opened it from is the |
| + nested-accordion trap TestNoNestedExclusiveDetails exists to |
| + catch. An input has no such problem. |
| + |
| + Scriptless by construction: it is a real form with a real submit. |
| + The button is visually hidden until the input has focus, so the |
| + bar stays a title bar until somebody means to edit it, and Enter |
| + submits from the input either way. */}} |
| +{{if .Data.MayWrite}} |
| +<form data-doc-title method="post" action="/sheets/{{.Data.Sheet.ID}}/rename"> |
| +<input data-doc-title-input name="title" value="{{.Data.Sheet.Title}}" |
| + aria-label="Sheet title" autocomplete="off" spellcheck="false"> |
| +<button rst-btn="ghost" type="submit">Rename</button> |
| +</form> |
| +{{else}} |
| +<span data-doc-title><span data-doc-title-text |
| + title="{{.Data.Sheet.Title}}">{{.Data.Sheet.Title}}</span></span> |
| +{{end}} |
| {{end}} |
| {{define "main"}} |
| @@ -92,7 +122,11 @@ |
| <summary>File</summary> |
| <div rst-dropdown-menu> |
| <form method="post" action="/sheets"><button type="submit">New sheet</button></form> |
| -<a href="/sheets">Import a spreadsheet…</a> |
| +{{/* A page, not a file input in this dropdown. A hidden input behind a |
| + menu-shaped label never submits itself, so the menu item would |
| + have needed script to import anything at all — see |
| + handleImportPage. */}} |
| +<a href="/sheets/import">Import a spreadsheet…</a> |
| <a href="/sheets/{{.Data.Sheet.ID}}/export.xlsx">Download as .xlsx</a> |
| <hr> |
| <a href="/sheets/{{.Data.Sheet.ID}}/table">Open as table</a> |
diff --git a/internal/sheets/templates/sheets.html b/internal/sheets/templates/sheets.html| index 2dbef11..4844b61 100644 |
| --- a/internal/sheets/templates/sheets.html |
| +++ b/internal/sheets/templates/sheets.html |
| @@ -1,64 +1,66 @@ |
| +{{/* The verbs live in the top bar; the page is the list. |
| + |
| + This screen used to open with two stacked forms — a title field |
| + with a Create button, then a dashed drop zone with a file input and |
| + four lines of limits — and the sheets themselves began below the |
| + fold of both. A list of sheets should be a list of sheets. |
| + |
| + Create is a button beside the account menu now, and makes a sheet |
| + called "Untitled sheet" in one click; the name is fixed afterwards |
| + by renaming it in the top bar of the sheet itself, which is what |
| + handlers_rename.go is for. Import moved to a page of its own at |
| + /sheets/import — see handleImportPage for why a page and not an |
| + item in the File menu. */}} |
| {{define "head"}} |
| -{{/* Drag and drop only. Everything the drop zone does, the form below |
| - it already does with scripting off — drop.js fills that same file |
| - input and submits that same form, so there is one route, one CSRF |
| - check and one code path either way. */}} |
| +{{/* Drag and drop, and only that. The overlay at the foot of this file |
| + is hidden until a file is dragged over the window, and every drop |
| + ends as a submit of the same form /sheets/import serves — same |
| + route, same CSRF check, same job. |
| + |
| + With scripting off it never appears and the Import link in the top |
| + bar is the entire feature. That is what AGENTS.md's "every |
| + drag-only affordance needs a menu equivalent" asks for: the link |
| + is the floor, and the drag is a shortcut to it. */}} |
| <script defer src="{{asset "static/drop.js"}}"></script> |
| {{end}} |
| +{{define "topbaractions"}} |
| +{{if .SignedIn}} |
| +{{/* The wrapper is what carries the auto margin that puts these |
| + against the account menu — see sheets.css. It lives here rather |
| + than in layout.html so that a page which fills this block with |
| + nothing contributes no element at all, and the grid's bar is |
| + exactly what it was. */}} |
| +<div data-topbar-actions> |
| +{{/* Import first, Create last: Create is the primary action and sits |
| + closest to the account menu, which is the corner the eye returns |
| + to. A link rather than a form, because it opens a page rather |
| + than changing anything. */}} |
| +<a rst-btn href="/sheets/import">Import</a> |
| +{{/* No title field, and so no dialog and no second step: the button |
| + IS the create. handleSheetCreate defaults an empty title to |
| + "Untitled sheet" and 303s into the new sheet, so a click lands |
| + you in a grid you can type into. */}} |
| +<form method="post" action="/sheets"> |
| +<button rst-btn="primary" type="submit">Create</button> |
| +</form> |
| +</div> |
| +{{end}} |
| +{{end}} |
| + |
| {{define "content"}} |
| <header rst-page-header> |
| <div rst-page-header-titles> |
| -<h1>Your sheets</h1> |
| +{{/* "All sheets", not "Your sheets". The old heading was wrong twice: |
| + it repeated the brand in the bar directly above it, and it claimed |
| + ownership the line underneath immediately contradicts — everyone on |
| + the instance opens every sheet here, so almost none of these are |
| + yours. A heading its own subtitle corrects is worse than none. */}} |
| +<h1>All sheets</h1> |
| <p rst-page-header-sub>Everyone on this instance can open every sheet here.</p> |
| </div> |
| </header> |
| -{{if .Data.ImportError}} |
| -<div rst-callout rst-tone="negative"> |
| -<div rst-callout-body> |
| -<p>{{.Data.ImportError}}</p> |
| -</div> |
| -</div> |
| -{{end}} |
| - |
| -<form rst-form method="post" action="/sheets"> |
| -<div rst-field-row> |
| -<div rst-field> |
| -<label rst-field-label for="title">New sheet</label> |
| -<input rst-input id="title" name="title" type="text" placeholder="Untitled sheet" autocomplete="off"> |
| -</div> |
| -<button rst-btn="primary" type="submit">Create</button> |
| -</div> |
| -</form> |
| - |
| -{{/* Import. The file input and its button are the whole feature — they |
| - work with scripting off, and dropping a file onto the zone below |
| - does nothing this form does not already do. |
| - |
| - data-drop is drop.js's hook and is deliberately a data- attribute |
| - rather than an rst- one: the rst- namespace is rastrillo's, and an |
| - invented rst- attribute is a name tokens.css does not define, which |
| - is the mistake this app has already made once in CSS. */}} |
| -<form rst-form method="post" action="/sheets/import" enctype="multipart/form-data"> |
| -<div data-drop> |
| -<div rst-field-row> |
| -<div rst-field> |
| -<label rst-field-label for="file">Import a spreadsheet</label> |
| -<input rst-input id="file" name="file" type="file" accept=".xlsx,.xlsm"> |
| -{{/* The first-worksheet limit is stated BEFORE the import, not |
| - discovered after it. Import reads one worksheet (xlsx.Import), and |
| - a workbook with three of them would otherwise arrive looking whole |
| - — which is the quiet kind of wrong PRODUCT.md's first principle |
| - forbids. */}} |
| -<p rst-field-hint data-drop-hint>Excel .xlsx or .xlsm, up to 20 MB. The |
| -first worksheet is imported. You can also drag a file onto this box.</p> |
| -</div> |
| -<button rst-btn type="submit">Import</button> |
| -</div> |
| -</div> |
| -</form> |
| - |
| {{if .Data.Sheets}} |
| <ul rst-list data-sheet-list> |
| {{range .Data.Sheets}} |
| @@ -80,11 +82,80 @@ first worksheet is imported. You can also drag a file onto this box.</p> |
| {{end}} |
| </ul> |
| {{else}} |
| +{{/* The empty state points at the two controls in the top bar. It |
| + used to say "make one above", which was true when the form was |
| + directly above it and would now be a sentence pointing at a |
| + header. An empty screen is the one place in a product that has |
| + nothing to do but explain what to do next. */}} |
| <div rst-empty> |
| <p rst-empty-title>No sheets yet</p> |
| -<p rst-empty-body>Make one above and it opens with a blank grid you can type |
| -into straight away, or import an Excel file to start from what you already |
| -have.</p> |
| +<p rst-empty-body>Create one from the top bar and it opens with a blank |
| +grid you can type into straight away, or |
| +<a href="/sheets/import">import an Excel file</a> to start from what you |
| +already have.</p> |
| +</div> |
| +{{end}} |
| + |
| +{{if .SignedIn}} |
| +{{/* The drop target for the whole page. |
| + |
| + This screen used to carry a dashed box you could drop a file on, |
| + and taking the box away took the drop with it — so the drop comes |
| + back without the box. It is `hidden` at rest, contributes nothing |
| + to the layout and nothing to the tab order, and drop.js reveals it |
| + only while a drag carrying files is actually over the window. An |
| + affordance that appears exactly when it applies beats a permanent |
| + rectangle explaining that it exists. |
| + |
| + It is the same form as the one on /sheets/import, deliberately: |
| + drop.js fills this file input and submits this form, so a drop |
| + from here is the ordinary multipart POST and there is no second |
| + code path on the server. The file field is visible inside the |
| + panel rather than hidden, because a refused drop ("drop one file |
| + at a time") needs something to recover with that is not the back |
| + button. |
| + |
| + `hidden` is load-bearing and is asserted in a test: rst-modal-overlay |
| + sets display:flex, an author rule beats the UA's [hidden], and |
| + without the reinstating rule in sheets.css this would cover the |
| + list permanently. */}} |
| +<form data-drop data-drop-page hidden rst-modal-overlay |
| + method="post" action="/sheets/import" enctype="multipart/form-data"> |
| +<div rst-modal-panel> |
| +<div data-drop-page-body> |
| +<p data-drop-page-title>Drop it to import</p> |
| +{{/* The same limits /sheets/import states, and stated before the |
| + import for the same reason: one worksheet is read, and a |
| + three-sheet workbook would otherwise arrive looking whole. This |
| + is also the line drop.js rewrites to refuse a drop, which is |
| + why it is a sentence and not a list. */}} |
| +<p rst-field-hint data-drop-hint>Excel .xlsx or .xlsm, up to 20 MB. The |
| +first worksheet is imported.</p> |
| +{{/* Chooser and submit, so a refused drop has something to recover |
| + with, and so the panel is a working form rather than a picture |
| + of one. They are hidden while a drag is actually over the window |
| + — see [data-drop-page-recover] in sheets.css — because offering |
| + a file picker to somebody mid-drag is a second way to do the |
| + thing they are already doing. |
| + |
| + They carry no tabindex, and its absence is a fix rather than an |
| + omission. Both used to carry tabindex="-1", to "keep them out of |
| + the tab order while the overlay is up" — which is exactly backwards. |
| + The overlay stays up after a REFUSED drop precisely so these two |
| + controls can be used, and a negative tabindex meant a keyboard user |
| + tabbed straight past the recovery form into the list behind it. |
| + The other two states never needed the help: `hidden` on the form |
| + takes the whole subtree out of the tab order at rest, and |
| + [data-drop-page-recover] is display:none under [data-drop-over], |
| + which does the same mid-drag. So it was redundant where it was |
| + harmless and harmful where it was not. */}} |
| +<div data-drop-page-recover> |
| +<input rst-input name="file" type="file" accept=".xlsx,.xlsm" |
| + aria-label="Spreadsheet to import"> |
| +<button rst-btn="primary" type="submit">Import</button> |
| +</div> |
| </div> |
| +</div> |
| +</form> |
| {{end}} |
| {{end}} |
diff --git a/tools/drive/chrome.mjs b/tools/drive/chrome.mjs| index 65083a8..95aaa11 100644 |
| --- a/tools/drive/chrome.mjs |
| +++ b/tools/drive/chrome.mjs |
| @@ -63,9 +63,23 @@ if (!m) { console.error('no magic link in the log'); process.exit(1); } |
| await page.goto(m[0]); |
| await page.waitForURL(/\/sheets/); |
| -/* A sheet with something in it. */ |
| -await page.fill('#title', 'Commission Q2'); |
| -await Promise.all([page.waitForURL(/\/sheets\/\d+$/), page.click('form[action="/sheets"] button[type=submit]')]); |
| +/* A sheet with something in it. |
| + * |
| + * Created from the top bar's Create button, then named by renaming it. |
| + * The list page's #title field is gone: it carries no forms now, Create |
| + * posts an empty one and the sheet lands as "Untitled sheet", so the |
| + * only place a sheet is ever named is the title in the top bar. The |
| + * rename is a real form submit, so driving it here covers the |
| + * scriptless path as well as the button. */ |
| +await Promise.all([ |
| + page.waitForURL(/\/sheets\/\d+$/), |
| + page.click('[rst-shell-tail] form[action="/sheets"] button[type=submit]'), |
| +]); |
| +await page.fill('[data-doc-title-input]', 'Commission Q2'); |
| +await Promise.all([ |
| + page.waitForURL(/\/sheets\/\d+$/), |
| + page.press('[data-doc-title-input]', 'Enter'), |
| +]); |
| const url = page.url(); |
| await page.waitForFunction(() => window.SheetsGrid && window.SheetsGrid.worksheet()); |
diff --git a/tools/drive/clipboard.mjs b/tools/drive/clipboard.mjs| index 61d8f16..97d58ec 100644 |
| --- a/tools/drive/clipboard.mjs |
| +++ b/tools/drive/clipboard.mjs |
| @@ -33,7 +33,11 @@ await page.goto(`${base}/signin`); await page.fill('input[type=email]','paul@exa |
| await Promise.all([page.waitForLoadState('networkidle'),page.click('button[type=submit]')]); |
| const mm=[...fs.readFileSync(process.env.SHEETS_LOG,'utf8').matchAll(/http:\/\/127\.0\.0\.1:8799\/auth\/verify\?[^\s\\"]+/g)].pop(); |
| await page.goto(mm[0]); await page.waitForURL(/\/sheets/); |
| -await page.fill('#title','Clip2'); await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.click('form[action="/sheets"] button[type=submit]')]); |
| +// Create from the top bar, then name it by renaming: the list's #title |
| +// field is gone, so a sheet is born "Untitled sheet" and named after. |
| +await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.click('[rst-shell-tail] form[action="/sheets"] button[type=submit]')]); |
| +await page.fill('[data-doc-title-input]','Clip2'); |
| +await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.press('[data-doc-title-input]','Enter')]); |
| await page.waitForFunction(()=>window.SheetsGrid&&window.SheetsGrid.worksheet()); |
| const w=(r,c,v)=>page.evaluate(([r,c,v])=>window.SheetsGrid.write(r,c,v),[r,c,v]); |
| const sel=(x1,y1,x2,y2)=>page.evaluate(([a,b,c,d])=>window.SheetsGrid.worksheet().updateSelectionFromCoords(a,b,c,d),[x1,y1,x2,y2]); |
diff --git a/tools/drive/drive.mjs b/tools/drive/drive.mjs| index 3946dfb..01bb025 100644 |
| --- a/tools/drive/drive.mjs |
| +++ b/tools/drive/drive.mjs |
| @@ -38,7 +38,14 @@ |
| import net from 'node:net'; |
| import fs from 'node:fs'; |
| const PLAYWRIGHT = process.env.PLAYWRIGHT || 'playwright'; |
| -const { chromium } = await import(PLAYWRIGHT); |
| +/* A CJS build imported from ESM puts its exports on .default, so a |
| + * plain `const { chromium } = await import(...)` yields undefined and |
| + * dies one line later at chromium.launch — which is how this read until |
| + * the branch that added the checks below tried to run it. chrome.mjs |
| + * already spelled it this way; the two now agree. */ |
| +const pw = await import(PLAYWRIGHT); |
| +const chromium = pw.chromium || (pw.default && pw.default.chromium); |
| +if (!chromium) { console.error('playwright: no chromium export'); process.exit(1); } |
| const SOCK = process.env.SHEETS_SOCK || '/tmp/s.sock'; |
| const LOG = process.env.SHEETS_LOG || '/tmp/s.log'; |
| @@ -78,16 +85,27 @@ if (!m) { console.log('FAIL: no magic link in the log'); process.exit(1); } |
| await page.goto(`${base}/auth/verify?token=${m[1]}`); |
| console.log('signed in as:', (await page.textContent('body')).includes('Signed in as') ? 'yes' : 'NO'); |
| -// 2. Make a sheet. |
| +// 2. Make a sheet, from the top bar's Create button. |
| +// |
| +// This used to fill #title on the list page. That field is gone: the |
| +// list carries no forms now, Create posts an empty one, and the sheet |
| +// lands as "Untitled sheet" — so naming it is a rename, below. |
| await page.goto(base + '/sheets'); |
| -await page.fill('#title', 'Browser drive'); |
| // Not networkidle: the SSE stream is a long-lived connection, so the |
| // network on this page is never idle by design. |
| -await page.click('form[action="/sheets"] button[type=submit]'); |
| +await page.click('[rst-shell-tail] form[action="/sheets"] button[type=submit]'); |
| await page.waitForURL(/\/sheets\/\d+$/, { timeout: 15000 }); |
| const url = page.url(); |
| console.log('sheet url:', url); |
| +// 2b. Rename it from the top bar, which is now the only place a sheet |
| +// is ever named — and a real form submit, so this covers the |
| +// scriptless path too. |
| +await page.fill('[data-doc-title-input]', 'Browser drive'); |
| +await page.press('[data-doc-title-input]', 'Enter'); |
| +await page.waitForURL(/\/sheets\/\d+$/, { timeout: 15000 }); |
| +console.log('renamed to:', JSON.stringify(await page.inputValue('[data-doc-title-input]'))); |
| + |
| // 3. The grid must actually render. |
| await page.waitForSelector('.jexcel, .jss_worksheet, table', { timeout: 15000 }); |
| console.log('grid rendered:', await page.locator('[data-sheet-grid] table').count() > 0); |
diff --git a/tools/drive/drop.mjs b/tools/drive/drop.mjs| new file mode 100644 |
| index 0000000..c96deba |
| --- /dev/null |
| +++ b/tools/drive/drop.mjs |
| @@ -0,0 +1,268 @@ |
| +/* Drive the sheet list's drop overlay in a real browser. |
| + * |
| + * The third of the drives, after drive.mjs (live sync across two tabs) |
| + * and chrome.mjs (the editor chrome). Like both it is NOT part of |
| + * `make test`: it needs a browser and a running instance, neither of |
| + * which CI has. |
| + * |
| + * It is here because the Go suite can assert the markup and read the |
| + * stylesheet, and neither tells you whether the thing works. The list |
| + * has no visible drop box any more — a list of sheets should be a list |
| + * of sheets — so the whole affordance is an overlay that must be |
| + * invisible at rest, appear when a file is over the window, and go away |
| + * again. "Invisible" and "appears" are not things a Go test can see. |
| + * |
| + * It earned its place on the first run. The overlay carries data-drop, |
| + * because that is what drop.js looks for, and so it also matched |
| + * [data-drop][data-drop-over] — 0-2-0 against [rst-modal-overlay]'s |
| + * 0-1-0. The scrim was painted accent-soft and the sheet list vanished |
| + * behind a flat blue screen. Every Go test passed. |
| + * |
| + * Usage, from the repository root — see drive.mjs for the full recipe: |
| + * |
| + * SHEETS_SOCK=$D/s.sock SHEETS_LOG=$D/s.log OUT=$D \ |
| + * SHEETS_FIXTURE=testdata/corpus/xlsx/numbers.xlsx \ |
| + * node tools/drive/drop.mjs |
| + * |
| + * The drag events are synthesised rather than performed: Playwright |
| + * cannot drag a file in from the desktop, and a DragEvent carrying a |
| + * real DataTransfer of real File objects is what the browser delivers |
| + * when it does. The final drop is a genuine multipart upload — the |
| + * import at the end really imports. |
| + */ |
| +import net from 'node:net'; |
| +import fs from 'node:fs'; |
| +const PLAYWRIGHT = process.env.PLAYWRIGHT || 'playwright'; |
| +const pw = await import(PLAYWRIGHT); |
| +/* A CJS build imported from ESM puts its exports on .default. */ |
| +const chromium = pw.chromium || (pw.default && pw.default.chromium); |
| +if (!chromium) { console.error('playwright: no chromium export'); process.exit(1); } |
| + |
| +const SOCK = process.env.SHEETS_SOCK; |
| +const LOG = process.env.SHEETS_LOG; |
| +const PORT = parseInt(process.env.SHEETS_PORT || '8799', 10); |
| +const OUT = process.env.OUT || '.'; |
| +const FIXTURE = process.env.SHEETS_FIXTURE || 'testdata/corpus/xlsx/numbers.xlsx'; |
| + |
| +const proxy = net.createServer((c) => { |
| + const u = net.connect(SOCK); |
| + c.pipe(u); u.pipe(c); |
| + c.on('error', () => u.destroy()); |
| + u.on('error', () => c.destroy()); |
| +}); |
| +await new Promise((r) => proxy.listen(PORT, '127.0.0.1', r)); |
| + |
| +const base = `http://127.0.0.1:${PORT}`; |
| +const browser = await chromium.launch(); |
| +const ctx = await browser.newContext({ viewport: { width: 1280, height: 800 } }); |
| +const page = await ctx.newPage(); |
| + |
| +const errors = []; |
| +page.on('console', (m) => { if (m.type() === 'error') errors.push('console: ' + m.text()); }); |
| +page.on('pageerror', (e) => errors.push('pageerror: ' + e.message)); |
| + |
| +const failed = []; |
| +function check(name, ok, detail) { |
| + console.log((ok ? ' ok ' : ' FAIL ') + name + (detail === undefined ? '' : ' ' + JSON.stringify(detail))); |
| + if (!ok) failed.push(name); |
| +} |
| + |
| +/* Sign in. The magic link is read out of the log, which only works on a |
| + * loopback origin with no SMTP — see internal/sheets/auth.go. */ |
| +const before = fs.readFileSync(LOG, 'utf8').length; |
| +await page.goto(base + '/signin'); |
| +await page.fill('#address', 'paul@example.com'); |
| +await page.click('form[action="/signin"] button[type=submit]'); |
| +await page.waitForURL(/signin\?sent=1/, { timeout: 15000 }); |
| +const link = fs.readFileSync(LOG, 'utf8').slice(before).match(/auth\/verify\?token=([A-Za-z0-9_-]+)/); |
| +if (!link) { console.error('no magic link in the log'); process.exit(1); } |
| +await page.goto(`${base}/auth/verify?token=${link[1]}`); |
| +await page.goto(base + '/sheets'); |
| + |
| +console.log('\n— at rest'); |
| + |
| +const rest = await page.evaluate(() => { |
| + const z = document.querySelector('[data-drop-page]'); |
| + if (!z) return null; |
| + const r = z.getBoundingClientRect(); |
| + const input = z.querySelector('input[type=file]'); |
| + return { |
| + hidden: z.hidden, |
| + display: getComputedStyle(z).display, |
| + area: r.width * r.height, |
| + reachable: input ? input.offsetParent !== null : null, |
| + }; |
| +}); |
| +check('the overlay is in the page', rest !== null); |
| +check('...hidden', rest && rest.hidden === true && rest.display === 'none', rest); |
| +check('...costing no layout', rest && rest.area === 0, rest && rest.area); |
| +check('...and no tab stop', rest && rest.reachable === false, rest && rest.reachable); |
| +await page.screenshot({ path: OUT + '/list-at-rest.png' }); |
| + |
| +/* Synthesised drags. __mkdt builds a DataTransfer of n copies of the |
| + * fixture; __fire dispatches at the centre of the viewport, on whatever |
| + * element is actually there, so the event bubbles the way a real one |
| + * does rather than being handed straight to the listener. */ |
| +const helper = ` |
| + window.__mkdt = (b64, n) => { |
| + const bin = atob(b64); |
| + const bytes = new Uint8Array(bin.length); |
| + for (let i = 0; i < bin.length; i++) bytes[i] = bin.charCodeAt(i); |
| + const dt = new DataTransfer(); |
| + for (let i = 0; i < n; i++) { |
| + dt.items.add(new File([bytes], (i ? i + '-' : '') + 'drop.xlsx', |
| + { type: 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet' })); |
| + } |
| + return dt; |
| + }; |
| + window.__fire = (type, dt) => { |
| + const e = new DragEvent(type, { bubbles: true, cancelable: true, dataTransfer: dt }); |
| + (document.elementFromPoint(640, 400) || document.body).dispatchEvent(e); |
| + }; |
| +`; |
| +const b64 = fs.readFileSync(FIXTURE).toString('base64'); |
| +const drag = (n) => page.evaluate(([b64, n]) => { |
| + window.__dt = window.__mkdt(b64, n); |
| + window.__fire('dragenter', window.__dt); |
| + window.__fire('dragover', window.__dt); |
| +}, [b64, n]); |
| + |
| +console.log('\n— a file over the window'); |
| + |
| +await page.evaluate(helper); |
| +await drag(1); |
| +const dragging = await page.evaluate(() => { |
| + const z = document.querySelector('[data-drop-page]'); |
| + const r = z.getBoundingClientRect(); |
| + return { |
| + hidden: z.hidden, |
| + over: z.hasAttribute('data-drop-over'), |
| + w: Math.round(r.width), h: Math.round(r.height), |
| + scrim: getComputedStyle(z).backgroundColor, |
| + recover: getComputedStyle(z.querySelector('[data-drop-page-recover]')).display, |
| + }; |
| +}); |
| +check('the overlay appears', dragging.hidden === false && dragging.over === true, dragging); |
| +check('...covering the window', dragging.w >= 1200 && dragging.h >= 700, dragging); |
| +/* The list must still be visible behind it. This is the check that |
| + * caught the specificity bug in the header comment: an opaque backdrop |
| + * reads as the page having gone away. */ |
| +check('...over a translucent scrim, with the list still there', |
| + /^rgba\(/.test(dragging.scrim) && parseFloat(dragging.scrim.split(',')[3]) < 0.8, dragging.scrim); |
| +check('...and the chooser out of the way', dragging.recover === 'none', dragging.recover); |
| +await page.screenshot({ path: OUT + '/list-dragging-light.png' }); |
| + |
| +await page.evaluate(() => window.__fire('dragleave', window.__dt)); |
| +check('the overlay goes when the drag leaves', |
| + (await page.evaluate(() => document.querySelector('[data-drop-page]').hidden)) === true); |
| + |
| +console.log('\n— a drop it cannot take'); |
| + |
| +await page.evaluate(([b64]) => { |
| + const dt = window.__mkdt(b64, 2); |
| + window.__fire('dragenter', dt); |
| + window.__fire('drop', dt); |
| +}, [b64]); |
| +const refused = await page.evaluate(() => { |
| + const z = document.querySelector('[data-drop-page]'); |
| + return { |
| + hidden: z.hidden, |
| + hint: z.querySelector('[data-drop-hint]').textContent.trim(), |
| + recover: getComputedStyle(z.querySelector('[data-drop-page-recover]')).display, |
| + path: location.pathname, |
| + }; |
| +}); |
| +check('two files are refused in words', refused.hint === 'Drop one file at a time.', refused); |
| +check('...with the overlay left up to say so', refused.hidden === false, refused); |
| +check('...and the chooser back, to recover with', refused.recover !== 'none', refused.recover); |
| +check('...having submitted nothing', refused.path === '/sheets', refused.path); |
| + |
| +/* The recovery form must be reachable by KEYBOARD, not merely visible. |
| + * |
| + * An independent review found both controls carrying tabindex="-1", |
| + * which put them out of the tab order at the one moment they exist to |
| + * be used: the overlay stays up after a refusal so they can be used to |
| + * recover, and Tab went straight past them into the list behind. It |
| + * looked completely correct in a screenshot, which is why this check |
| + * presses Tab instead of reading a style — the same lesson the menu bar |
| + * taught when it worked from the keyboard and not from a mouse. |
| + * |
| + * Tabbing from the panel must land ON the panel's own controls. */ |
| +const reachable = await page.evaluate(() => { |
| + const rec = document.querySelector('[data-drop-page-recover]'); |
| + const controls = [...rec.querySelectorAll('input,button')]; |
| + return { |
| + negative: controls.filter((c) => c.tabIndex < 0).length, |
| + count: controls.length, |
| + }; |
| +}); |
| +check('the recovery controls are in the tab order', reachable.negative === 0 |
| + && reachable.count === 2, JSON.stringify(reachable)); |
| + |
| +/* And prove it by actually tabbing: focus the panel, press Tab, and the |
| + * focus must be inside the panel rather than on the list underneath. */ |
| +await page.evaluate(() => document.querySelector('[data-drop-page-recover] input').focus()); |
| +const focusedInPanel = await page.evaluate(() => |
| + !!document.activeElement.closest('[data-drop-page-recover]')); |
| +check('...and focus really lands in the panel', focusedInPanel, 'focus escaped the overlay'); |
| +await page.keyboard.press('Tab'); |
| +const nextInPanel = await page.evaluate(() => |
| + !!document.activeElement.closest('[data-drop-page-recover]')); |
| +check('...and Tab moves to Import, not past the overlay', nextInPanel, |
| + 'Tab left the recovery form'); |
| + |
| +await page.screenshot({ path: OUT + '/list-refused.png' }); |
| + |
| +await page.keyboard.press('Escape'); |
| +const dismissed = await page.evaluate(() => { |
| + const z = document.querySelector('[data-drop-page]'); |
| + return { hidden: z.hidden, hint: z.querySelector('[data-drop-hint]').textContent.trim() }; |
| +}); |
| +check('Escape dismisses it', dismissed.hidden === true, dismissed); |
| +check('...and the refusal does not outlive the dismissal', |
| + dismissed.hint.startsWith('Excel'), dismissed.hint); |
| + |
| +console.log('\n— dark'); |
| + |
| +await ctx.addCookies([{ name: 'sheets_mode', value: 'dark', url: base }]); |
| +await page.goto(base + '/sheets'); |
| +await page.evaluate(helper); |
| +await drag(1); |
| +/* The cookie is sheets_mode. Naming it "mode" renders the light page |
| + * and the screenshot proves nothing — which it once did, and the only |
| + * tell was that the two images were pixel-identical. */ |
| +check('dark really is dark', (await page.getAttribute('html', 'data-theme')) === 'dark'); |
| +await page.screenshot({ path: OUT + '/list-dragging-dark.png' }); |
| + |
| +console.log('\n— the drop imports'); |
| + |
| +await ctx.addCookies([{ name: 'sheets_mode', value: 'light', url: base }]); |
| +await page.goto(base + '/sheets'); |
| +await page.evaluate(helper); |
| +await page.evaluate(([b64]) => { |
| + const dt = window.__mkdt(b64, 1); |
| + window.__fire('dragenter', dt); |
| + window.__fire('drop', dt); |
| +}, [b64]); |
| +let landed = 'never left /sheets'; |
| +try { |
| + await page.waitForURL((u) => !u.pathname.endsWith('/sheets'), { timeout: 20000 }); |
| + landed = new URL(page.url()).pathname; |
| +} catch (e) { /* landed stays as it was */ } |
| +check('a drop starts a real import', /^\/(jobs|sheets)\//.test(landed), landed); |
| + |
| +let arrived = false; |
| +for (let i = 0; i < 60; i++) { |
| + await page.goto(base + '/sheets'); |
| + if (await page.locator('[data-sheet-list] li').count() > 0) { arrived = true; break; } |
| + await page.waitForTimeout(500); |
| +} |
| +check('...and the sheet lands in the list', arrived); |
| +await page.screenshot({ path: OUT + '/list-after-import.png' }); |
| + |
| +check('no console errors', errors.length === 0, errors.slice(0, 8)); |
| + |
| +console.log(failed.length ? '\nFAILED: ' + failed.join(', ') : '\nall checks passed'); |
| +await browser.close(); |
| +proxy.close(); |
| +process.exit(failed.length ? 1 : 0); |
diff --git a/tools/drive/menus.mjs b/tools/drive/menus.mjs| index 01c11ab..d508b85 100644 |
| --- a/tools/drive/menus.mjs |
| +++ b/tools/drive/menus.mjs |
| @@ -48,7 +48,13 @@ await page.goto(`${base}/signin`); await page.fill('input[type=email]','paul@exa |
| await Promise.all([page.waitForLoadState('networkidle'),page.click('button[type=submit]')]); |
| const mm=[...fs.readFileSync(LOG,'utf8').matchAll(/http:\/\/127\.0\.0\.1:8799\/auth\/verify\?[^\s\\"]+/g)].pop(); |
| await page.goto(mm[0]); await page.waitForURL(/\/sheets/); |
| -if(!(await page.$('[data-sheet-list]'))){await page.fill('#title','Audit');await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.click('form[action="/sheets"] button[type=submit]')]);} |
| +// No sheets yet: Create from the top bar, then name it by renaming. |
| +// The list's #title field is gone — a sheet is born "Untitled sheet". |
| +if(!(await page.$('[data-sheet-list]'))){ |
| + await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.click('[rst-shell-tail] form[action="/sheets"] button[type=submit]')]); |
| + await page.fill('[data-doc-title-input]','Audit'); |
| + await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.press('[data-doc-title-input]','Enter')]); |
| +} |
| else {await page.click('[data-sheet-list] [rst-row-main] a'); await page.waitForURL(/\/sheets\/\d+$/);} |
| const url=page.url(); |
| await page.waitForFunction(()=>window.SheetsGrid&&window.SheetsGrid.worksheet()); |
diff --git a/tools/drive/tableformat.mjs b/tools/drive/tableformat.mjs| index 31eb994..040da0f 100644 |
| --- a/tools/drive/tableformat.mjs |
| +++ b/tools/drive/tableformat.mjs |
| @@ -26,7 +26,11 @@ await page.goto(`${base}/signin`); await page.fill('input[type=email]','paul@exa |
| await Promise.all([page.waitForLoadState('networkidle'),page.click('button[type=submit]')]); |
| const mm=[...fs.readFileSync(process.env.SHEETS_LOG,'utf8').matchAll(/http:\/\/127\.0\.0\.1:8799\/auth\/verify\?[^\s\\"]+/g)].pop(); |
| await page.goto(mm[0]); await page.waitForURL(/\/sheets/); |
| -await page.fill('#title','Table format'); await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.click('form[action="/sheets"] button[type=submit]')]); |
| +// Create from the top bar, then name it by renaming: the list's #title |
| +// field is gone, so a sheet is born "Untitled sheet" and named after. |
| +await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.click('[rst-shell-tail] form[action="/sheets"] button[type=submit]')]); |
| +await page.fill('[data-doc-title-input]','Table format'); |
| +await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.press('[data-doc-title-input]','Enter')]); |
| const url=page.url(); |
| await page.waitForFunction(()=>window.SheetsGrid&&window.SheetsGrid.worksheet()); |
| for(const [r,c,v] of [[0,0,'Region'],[0,1,'Revenue'],[1,0,'North'],[1,1,'1200'],[2,0,'South'],[2,1,'900']]) |