OTTER-691: refactor the Study Proposal page and switch its fields to character limits - #988
Conversation
a659d09 to
7962e04
Compare
nathanstitt
left a comment
There was a problem hiding this comment.
seems good, but probably need to adjust the placeholder as suggested
| orgSlug={enclaveOrgSlug} | ||
| // The card removes placeholder text from every input field | ||
| // on this page. The resubmit page keeps its own. | ||
| placeholder="" |
There was a problem hiding this comment.
placeholder="" here has a side effect in Mantine that I think we want to avoid. MultiSelect picks the inner field's type with !searchable && !placeholder ? 'hidden' : 'visible' (MultiSelect.mjs L350), and DatasetMultiSelect hardcodes searchable={false}. An empty string is falsy, so #datasets renders with data-type="hidden", which PillsInput.css styles as:
height: 1px; width: 1px; top: 0; left: 0;
pointer-events: none; position: absolute; opacity: 0;That collides with the headline feature of this card. focusFirstInvalid resolves datasets via getElementById, and focusable() returns the node itself because an <input> already has tabIndex >= 0 — so "jump to the first flagged field" puts the caret and the focus ring on a 1px, opacity: 0 element, and scrollIntoView({ block: 'center' }) centers a 1px box. datasets is first in ORDERED_FIELD_IDS, so it's the most common landing spot.
I think the e2e change is the same bug surfacing: datasetsField() had to switch from clicking the input to clicking label[for="datasets"] precisely because the input stopped being a click target. That reads to me as a symptom rather than a locator preference.
Could we get the no-placeholder look without flipping the field to hidden — e.g. placeholder=" ", or a hidePlaceholder prop on DatasetMultiSelect that keeps the field visible? Worth a browser check on where focus actually lands after an empty-form Submit.
There was a problem hiding this comment.
Confirmed and fixed in 795b857. I checked the Mantine source rather than taking it on trust, and it is worse than the comment says in one respect: id: _id is applied to both the PillsInput wrapper and the PillsInput.Field (MultiSelect.mjs L330 and L348), so two nodes share datasets. Either way focusable() lands on the 1px input, so the outcome is the one you described.
Also worth noting the undefined branch had the same effect: placeholder={value.length === 0 ? placeholder : undefined} is falsy in both directions, so the field was hidden with pills present too. That part is pre-existing and correct, since a field with pills is not a focus target.
Fix is inside DatasetMultiSelect rather than at the call site, so both pages are covered:
management-app/src/components/dataset-multi-select.tsx
Lines 30 to 40 in 795b857
management-app/src/components/dataset-multi-select.tsx
Lines 55 to 59 in 795b857
On the e2e: you read it right, that was the bug surfacing, and my comment above the locator documented the hidden input as if it were intended. Corrected. I kept label[for="datasets"] though, for two reasons that are not the original one: getByPlaceholder cannot come back (Step 2 genuinely has no placeholder text), and the duplicate id means a bare #datasets is two matches and a strict-mode failure. The real guard is now a unit test: https://github.com/safeinsights/management-app/blob/795b85760ed04974047066626c0dbaf6ba1f4947/src/components/dataset-multi-select.test.tsx. I verified it fails with data-type="hidden" if the substitution is removed, so it is a guard rather than a green rubber stamp.
One thing I have not done: re-run the browser check to watch where focus lands after an empty-form Submit. The unit test pins the mechanism (field is not hidden, id resolves to it), but not the pixels.
There was a problem hiding this comment.
Correction to my reply above: the duplicate-id claim was wrong. I read id: _id twice in MultiSelect.mjs and concluded two nodes carry datasets. Only one does, the inner input. The wrapper's _id feeds Input.Wrapper's label and aria-describedby wiring rather than becoming a literal id attribute. Verified two ways: a querySelectorAll('#datasets') count in a scratch render returns 1, and Playwright resolves locator('#datasets') to a single element with no strict-mode complaint.
So your original reading of focusable() was simply right, with no extra wrinkle from me.
That also removes the reason I gave for keeping the label locator, so the e2e now clicks #datasets directly (042c599), which is what you were asking for in the first place.
The CI e2e failure was mine too, from the same fix: tests/study-flow.spec.ts asserted toHaveAttribute('placeholder', '') and the value is now a single space. Rather than hard-coding the space I assert /^\s*$/ for "no visible placeholder text", plus data-type="visible", which is the assertion that actually fails if the field ever collapses back to the hidden 1px box:
management-app/tests/study-flow.spec.ts
Lines 1025 to 1030 in 042c599
Full suite run locally three times, 32/32 each time, no flakes.
| websocketProvider={websocketProvider} | ||
| contentStyle={contentStyle} | ||
| placeholder={field.placeholder} | ||
| contentHeight={contentHeight} |
There was a problem hiding this comment.
We pass contentHeight but not skeletonHeight, and Editor defaults the latter to a flat 240 (editor.tsx L76) for the skeleton it shows at L94 while unmounted / waiting on the websocket.
So in collaborative mode the four fields all render a 240px skeleton first, then snap to 105 / 205 / 205 / 505. That's a visible jump on every load, and projectSummary moves by ~265px.
The interesting part is that this PR already fixed the inner skeleton for exactly this reason — collaborative-editor.tsx now does <Skeleton h={resolveContentHeight(contentHeight, contentStyle)} /> with the comment "so the pre-connect skeleton is the height the editor mounts at and the page does not jump." The outer Editor skeleton is the one that renders first though, so the fix only lands on the second of the two. Passing skeletonHeight={contentHeight} here (or defaulting skeletonHeight to resolveContentHeight(...) inside Editor) would close the gap.
Worth noting CI runs single-user mode, which skips both skeletons entirely — so this won't show up in the test suite.
There was a problem hiding this comment.
Right on all counts, fixed in 795b857. I took the second option you offered rather than passing skeletonHeight at the call site, since the call site fix would have left every other caller on the flat 240:
management-app/src/components/editable-text/editor.tsx
Lines 92 to 96 in 795b857
resolveContentHeight(contentHeight, contentStyle), the same function CollaborativeEditor uses for the inner one, so the two skeletons and the mounted editor are all the same height. skeletonHeight stays as an explicit override.
Your point about the inner fix landing on the wrong one of the two is the part I would have missed on my own. I fixed the inner skeleton because that was the one I could see in the code path I was editing, and never asked which renders first.
Importing resolveContentHeight into editor.tsx costs nothing at the bundle level, since editor.tsx already imports SingleUserEditor statically and that pulls in editor-surface (the dynamic boundary is CollaborativeEditor, which is separate).
And yes, no test covers this. Single-user mode returns before either skeleton, so there is no seam to assert against without faking the websocket context, which felt like more machinery than the assertion is worth. I have noted it in the code comment instead so the next person does not assume the suite is watching it.
| // The confirmation modal closes as the mutation settles, leaving the user back on the | ||
| // form with every value intact. Put the Submit button back in view so retrying does not | ||
| // start with a scroll. | ||
| window.scrollTo?.({ top: document.body.scrollHeight, behavior: 'smooth' }) |
There was a problem hiding this comment.
This is the only window.scrollTo in the app, and document.body.scrollHeight is "the very bottom of the document" rather than "the Submit button." Those coincide today only because the footer happens to be last; anything appended below it (a support link, a footer banner) silently moves the target, with nothing failing.
We already have a helper that expresses the real intent — focusFirstInvalid uses scrollIntoView({ block: 'center' }) against a specific node. A ref on the Submit button plus scrollIntoView would say what we mean and survive layout changes.
One behavioral note either way: this fires unconditionally on failure, so a user who deliberately scrolled up to inspect a field gets yanked to the bottom. Since the error toast is already the thing announcing the failure, we could also just skip the scroll — worth a quick check against what the card actually asks for.
(The test only asserts scrollTo was called with behavior: 'smooth', so it wouldn't catch the target drifting.)
There was a problem hiding this comment.
Changed to the ref-equivalent in 795b857, using an id rather than a ref because useSubmitProposal lives in the context and threading a ref out of ProposalFooter into it would have meant new context surface for a scroll target. SUBMIT_BUTTON_ID (
ORDERED_FIELD_IDS, since it is a scroll target and never a focus target. The call is now getElementById(...)?.scrollIntoView({ block: 'center', behavior: 'smooth' }): management-app/src/contexts/proposal/hooks/use-submit-proposal.ts
Lines 103 to 108 in 795b857
On whether to scroll at all, I went back to the card. It asks for it twice, in the AC ("On page load, scroll to the bottom of the page so user can click the submit button again easily") and again in the test requirements. So it stays. But both phrasings state the bottom of the page and then immediately justify it by the Submit button, which is exactly the drift you flagged: the letter and the intent agree only while the footer is last. Scrolling the button satisfies the intent permanently and the letter today.
I left it unconditional. The card asks for it on load, and I would rather not invent a "did the user scroll deliberately" heuristic that the card does not ask for. If that yanking turns out to annoy people in QA it is a one-line change now that the target is a specific node.
You were also right that the old test was not watching anything. It stubbed window.scrollTo and asserted only behavior: 'smooth', which would have stayed green if the target became the page header. It now appends a real node under the real id and asserts scrollIntoView was called on that node with { block: 'center', behavior: 'smooth' }.
| // Placeholder-free for the same reason the dataset field is: | ||
| // the card removes placeholder text from every input on this | ||
| // page. | ||
| placeholder="" |
There was a problem hiding this comment.
Different situation from the datasets field above — this Select has searchable, so the input stays visible and focusable and the hidden-field problem doesn't apply. Flagging it only so the two placeholder="" call sites don't get treated as the same fix if the datasets one gets changed: this one is fine as-is.
Small thought while we're here — with no placeholder and no value, the PI select renders as an empty box whose only affordance is the caret. aria-label covers assistive tech, but sighted users lose the "this is a dropdown you pick from" cue that the datasets field at least keeps via its pills area. If the card really does mandate zero placeholders that's fine, just worth confirming it was meant to include the Select and not only the text inputs.
There was a problem hiding this comment.
Thanks for separating these two, that saved a wrong fix. I left this one exactly as-is, and the fix on the datasets field is inside DatasetMultiSelect rather than at either call site, so it does not reach the Select at all.
On whether the card meant the Select: it does. The AC phrases it as a change "to be applied to all input fields" and lists "Remove placeholder text from all input fields", not text inputs specifically, and the Figma frames show the PI dropdown empty. So the empty box is the requested state rather than an oversight in how I read it.
Your point about the lost affordance is fair regardless of what the card says. Sighted users get only the caret, and the datasets field keeps its pills area as a cue while this one has nothing. That is a design question rather than an implementation one, though, and the same "no placeholders" rule is landing across Step 1, Step 2, and the resubmit page, so if it needs revisiting it should be revisited for all of them at once rather than quietly excepting the PI select here. I have not raised it with design; happy to if you think it is worth a card.
7962e04 to
60377ed
Compare
|
Pushed 795b857 with all three review items addressed, replies in the threads. Short version: the placeholder one was a real bug and it broke the headline feature of this card, not just the look. Removing placeholder text flipped Mantine's non-searchable The other two: The PI select |
795b857 to
4b529a2
Compare
042c599 to
14dda24
Compare
14dda24 to
92ccb6e
Compare
92ccb6e to
a1129bc
Compare
Total coverage
Detailed report6 files with a coverage regression
|
see OTTER-691
This branch rebuilds the Study Proposal page, which is Step 2 of the proposal flow.
Review #980 first
This branch is on top of #980 (OTTER-690). It is not on top of
main. PR #980 changed all of the files that this card changes, and it already did one rule from this card: the Study title must not show on Step 2. This branch cannot merge before #980 merges.Decisions where the sources disagree
The card, the Figma frames, and earlier Done cards do not agree in three places.
onBlurproperties.On the blur decision: the title of that part is "Button Logic - Shifting to always enabled", and each line in it ends with "button remains enabled". The subject of the part is therefore the button, not the errors. OTTER-690 has the same text, and the work on that card did not obey it.
The changes stay inside Step 2
The
edit-and-resubmitpage sharesfield-config.ts,schema.ts, and the text-field component with Step 2. This card does not mention that page. Each difference therefore lives in the properties that the two call sites supply. PR #980 used the same method for the study title.CHARACTER_LIMITSWORD_LIMITSdraftProposalFormSchemaproposalFormSchemaThe word limits in the table apply to this branch only. OTTER-737 (#991) is on top of this branch. That PR changes the
edit-and-resubmitpage to characters and deletesWORD_LIMITS.schema.test.tshas a test group that fails if the character rules or the new text move intoproposalFormSchema.The shared editor surface uses the same method. The
isResizableproperty is off by default, so the reviewer-feedback, code-review, and outputs-decision editors do not change. Only Step 2 gets a handle.How the height control works
The card gives two limits. A manual change must not make a field less high than its default height, and it must not make a field less high than its automatic height. Together the card calls this "cannot go below whichever is currently taller". One
min-heightvalue gives both limits.The
min-heightvalue and the draggedheightvalue operate together. The drag makes the browser write an inlineheighton the element. React writes onlyminHeight, so a new render does not remove the dragged value. New text raises the limit above the dragged height and opens the box again. A deletion lowers the limit, and the dragged height returns to control.Measurements from a browser check on the Impact field:
Two points for the reviewer:
contentRef, the editable surface, which does not stretch. If it measured the scroll container, the limit would take its own value after a manual increase, and the user could never make the field smaller again.resize: verticalagrees with the design. The Figma handle looks like a special component, but that layer is an image of the Chrome handle.The height applies to one user only. The code writes it to no Yjs key, form field, database column, or browser storage, so there is no data path to test.
The new file
editor-surface.tsxexists becauseCollaborativeEditorandSingleUserEditorhad almost the same markup. Without it, the height code would exist two times. CI uses single-user mode, so a defect in the collaborative copy could stay hidden.Defects that only a browser check found
loadingproperty, which puts the Loader on top of the label. The accessible name stayed "Submitting", so a test on the name passed.disabledand aleftSectionspinner, as the design shows. Theloadingproperty also setsdisabled, so the protection against a second submit does not change. The newsubmit-confirmation-modal.test.tsxexamines thedata-loadingattribute.focusFirstInvalidcould not move the focus to a Lexical editorContentEditableaccepts atabIndexproperty but sets no default value, and no caller supplied one. A browser moves the focus to a contenteditable element, but happy-dom does not.tabIndexwas-1, so the search of the child elements found nothing.tabIndex={0}on the surface, and a test. Four of the six fields on this page are editors, so the rule "move the cursor to the first flagged field" had tests that passed but examined nothing.One footer test was also unreliable. It examined a label that shows for a short time, so the result changed with the speed of the database. That test now examines the final state, and the modal test examines the loading state directly.
Review fixes
Three items from the review on
60377ed4, all of them consequences of the same thing: this card removes placeholder text and adds per-field heights, and two components read those properties in ways the card did not anticipate.MultiSelectwith!searchable && !placeholder, andDatasetMultiSelectis never searchable. The card's "no placeholder" rule therefore gave the fielddata-type="hidden", whichPillsInput.csscollapses to a 1px, transparent,pointer-events: nonebox. That field carries the DOM id, so the headline rule of this card put the caret on an invisible element, anddatasetsis the first entry inORDERED_FIELD_IDS.DatasetMultiSelectnow substitutes a single space when a caller asks for no placeholder, which is truthy for Mantine's test and renders as no visible text. Only while the field is empty, which is the only state that carries a required error. With pills present the field stays hidden and the pills box remains the click target, as on both pages today.EditordefaultedskeletonHeightto a flat 240, so each field rendered a 240px skeleton and then snapped to 105, 205, 205, or 505.projectSummarymoved by about 265px.CollaborativeEditoralready resolved its own pre-connect skeleton fromcontentHeight, but the outer skeleton is the one that renders first.skeletonHeightnow defaults toresolveContentHeight(contentHeight, contentStyle), the same value the editor mounts at. The fix is inEditorrather than at the call site, so every caller gets it.window.scrollTo({ top: document.body.scrollHeight })is the bottom of the document. The card asks for the bottom of the page, but only ever to explain "so user can click the submit button again easily". The two coincide only while the footer is last, and anything added below it would move the target with nothing failing.SUBMIT_BUTTON_IDand the failure path callsscrollIntoView({ block: 'center' })on it, matching whatfocusFirstInvalidalready does.The e2e locator for the dataset field goes back to
#datasets, which is what the field being visible again makes possible.getByPlaceholdercannot come back, because Step 2 genuinely has no placeholder text. The comment above the locator no longer describes the hidden field as intended behavior.The same fix broke one e2e assertion, which is the CI failure on
4b529a28: the test assertedtoHaveAttribute('placeholder', '')and the value is now a single space. It asserts/^\s*$/instead, which states the actual requirement (no visible placeholder text) without pinning the spelling, and addsdata-type="visible", which is the assertion that fails if the field ever collapses back to the hidden box. The unit testdataset-multi-select.test.tsxguards the same thing at the component level; it fails withdata-type="hidden"if the substitution is removed.The second
placeholder=""on this page, the Principal InvestigatorSelect, is left alone. That one is searchable, so the hidden-field rule does not reach it, and the card asks for the removal on "all input fields" rather than on the text inputs only.One duplicate component removed
The Step 2 form and the resubmit form each had a local
EditableTextFieldEntrycomponent. The two copies were almost the same, and they became more different during this card, because only the Step 2 copy received the live character-limit rule.collaborative-proposal-text-field.tsxnow holds oneProposalTextFieldEntrycomponent for both pages, and the differences are the properties at the two call sites. The line count does not decrease (+82, -79), because the shared component has documentation that neither copy had.Smaller decisions
FormField, so it applies to each input field, as the card requires. Step 1 and the resubmit page also move by 4 px. This effect is intentional.validateInputOnChangeis no longer in the proposal form. It showed errors while the user typed, and the card does not permit this. The MantineclearInputErrorOnChangeproperty still removes an error when the user changes the field.variant="primary", which supplies no styles, to thefilledvariant that the card requires.FormFieldnorReadOnlyField.FormFieldmakes a label for a control, and this row has no control.ReadOnlyFieldhas no asterisk and no help text, but Figma shows both. The asterisk is visual only, because the user cannot supply a value.isDraftCreatorfrom the session. The browser knows the Clerk id of the user but not the database id instudy.researcherId. The rule is "did this person make the draft", not "is this person a researcher", so a second author on the same proposal sees the name only.reportError, because that function adds a reference number to a message that the card specifies exactly. Sentry still receives the exception.One item that this branch does not correct
The field error messages show red text only. The Figma error frames also show a red circle icon. The repository has an
InputErrorcomponent that draws this icon, butFormFielduses the MantineInput.Errorcomponent. Each field on Step 1, Step 2, and the resubmit page therefore has the same condition. The condition is older than this card, and a correction changes each user ofFormFieldin the application, so a different card must do this work.Out of scope
Test status
pnpm run checksis successful. The unit tests are successful: 296 files and 3013 tests.A browser check examined each requirement: the header and body text; the dataset field at 60% of the internal width of the card, with chips and chip removal; the four counters, and the limit at 3000 and 3001, with the live error and its
aria-livearea; the two height limits, and the automatic growth after a manual drag; the five messages for empty fields together, with the focus on the first field in page sequence andaria-invalidon all five; the text in the modal and its loading state; the failure toast at the top right (984, 16) in a 1440 viewport, with the field values kept; and the Researcher row, with and without the creator of the draft.The end-to-end tests changed because they used interface items that this branch removes: the "Submit initial request" button, its confirmation text, the dataset placeholder, and the old messages for empty fields. They now run: the full suite passed three consecutive times locally at
retries: 0under parallelism, 32 of 32 each time.The browser check did not examine the success toast, because a successful submission needs a real session. The unit tests examine that path.