Repository navigation
Expand the Example drawing over the editor panel - #586
Disesfgewu wants to merge 1 commit into
Conversation
Alan52254
left a comment
There was a problem hiding this comment.
The measurements in the issue and the three-viewport test make the problem and the fix easy to check, and keeping one canvas so strokes, undo and Jim's image do not change is the right call. jserv's threads already cover tab order, text-box gestures and the z-index; I checked those against the diff and agree, so I have not repeated them. My comments are about two places where the tests would still pass if the layout broke, and one focus behavior. The most useful change is to build the test shell from the real interview.html overlays, because that is what would have caught the z-index problem.
|
Some review comments on this pull request are not settled yet. Please refine the change per the review comments listed below, reply on each thread with what you changed or why you did not, and mark each thread resolved yourself once it is settled (Resolve conversation, at the bottom of the thread). The pull request stays blocked while review threads are open. Not addressed at the current head:
Addressed at the current head; reply with what changed and resolve:
Checked at caff855. |
princelu1009
left a comment
There was a problem hiding this comment.
Keeping one canvas and only restyling its container is a clean way to keep strokes, undo and Jim's image identical. The existing threads cover tab order, text-box gestures, stacking and the test shell, so I have not repeated them. My comments are about one change I could not make any test fail without, and two places where the diff could be smaller.
Example drawing showed only a strip of its 16:10 canvas and scrolled the rest, about 30% of it at first and at most 65% with the divider dragged all the way up. A button on its toolbar now lays the board over the editor panel, whose code and tests are not needed while drawing, and shows the whole canvas while the sidebar stays usable. The strokes stay on the one canvas, so what Jim reads and what undo removes do not change on the way back, by the button, Escape or closing Example drawing. Neither the button nor Escape acts during a stroke or a text-box gesture, and Escape is left to a field, a dialog, an input method, a held key or a modifier chord. What the board covers cannot be reached from the keyboard while it is expanded: the rest of the panel is inert, Jim's full stage is hidden, and Cmd/Ctrl+Enter runs nothing, since its results would land in the covered drawer. The stage stays on top in the wait for the report, while audio is blocked, so Resume audio can be pressed, and while it holds the focus that button gives it; a narrow window keeps its caption card. Covering the full stage keeps the canvas's right column; his voice is unaffected and shrinking restores all of it. Close sysprog21#579 Change-Id: I981fb4638856bc2a9a4e07fde1035b311c424a32
caff855 to
d6e5200
Compare
| :focus-within, | ||
| :has(#audio-playback-notice:not([hidden])) | ||
| ) { | ||
| visibility: hidden; |
There was a problem hiding this comment.
Hiding the whole .jim-stage while expanded takes the Live CC captions and their live-region announcements away from a candidate who relies on them. The comment above .jim-stage.compact already makes the opposite call for narrow windows: captions are the accessible transcript, so the avatar goes and the stage stays. Consider the same split here: keep the stage in its compact card over a corner of the board while expanded, so only the avatar and self-view are lost. If the card's footprint on the canvas is the objection, this fits a follow-up with #512, which decides where the stage sits.
| const panel = root.closest(".editor-panel"); | ||
| // The panel can scroll a little on a short screen, and the pane would | ||
| // scroll with it; the stylesheet stops it scrolling while expanded. | ||
| if (on && panel) panel.scrollTop = 0; |
There was a problem hiding this comment.
Expanding sets the panel's scrollTop to 0, and shrinking never puts it back. On a short window, where the candidate had to scroll the panel down to reach Expand at all (the 1280x500 case in the description), Shrink jumps back to the top and leaves the focused button out of view. Keep the old value on expand and restore it on shrink (with let scrollBack = null; next to drag), and extend the scrolled-panel test in interview-layout.test.js to check the round trip.
| if (on && panel) panel.scrollTop = 0; | |
| if (on && panel) { | |
| scrollBack = panel.scrollTop; | |
| panel.scrollTop = 0; | |
| } else if (panel && scrollBack !== null) { | |
| panel.scrollTop = scrollBack; | |
| scrollBack = null; | |
| } |
| for (let node = pane; panel && node !== panel; node = node.parentElement) | ||
| for (const sibling of node.parentElement.children) | ||
| if (sibling !== node) sibling.inert = on; |
There was a problem hiding this comment.
Shrinking sets inert = false on every sibling, and so does the setExpanded(false) call at mount, whatever the sibling was before. Nothing under the panel is made inert by other code today (holdWorkspaceBehindDialogs writes the panel itself), but the first thing that does will be undone by a shrink. Record the nodes this call changed and restore only those (with let covered = []; beside the other state):
| for (let node = pane; panel && node !== panel; node = node.parentElement) | |
| for (const sibling of node.parentElement.children) | |
| if (sibling !== node) sibling.inert = on; | |
| if (on) { | |
| covered = []; | |
| for (let node = pane; panel && node !== panel; node = node.parentElement) | |
| for (const sibling of node.parentElement.children) | |
| if (sibling !== node && !sibling.inert) { | |
| sibling.inert = true; | |
| covered.push(sibling); | |
| } | |
| } else { | |
| for (const sibling of covered) sibling.inert = false; | |
| covered = []; | |
| } |
| drag || | ||
| textDraft.busy() || | ||
| !root.hasAttribute("data-expanded") || | ||
| event.target.closest?.("input, textarea, select, [role=dialog]") |
There was a problem hiding this comment.
This listener is on document and never checks event.defaultPrevented. So a later handler that consumes Escape with preventDefault() alone also shrinks the board. The text draft only escapes this because it also calls stopPropagation(). A native <dialog> has no role attribute, so [role=dialog] misses it too. Both are cheap to close off:
| event.target.closest?.("input, textarea, select, [role=dialog]") | |
| event.defaultPrevented || | |
| event.target.closest?.("input, textarea, select, dialog, [role=dialog]") |
| details.addEventListener("toggle", paint); | ||
| // A shrinking board comes back to a drawer that may have been resized while | ||
| // it was out, so its docked height is measured again. | ||
| new MutationObserver(paint).observe(example, { |
There was a problem hiding this comment.
While the board is expanded, this observer (and the panel's ResizeObserver) still runs the example.open branch of paint. That branch measures .example-surface inside the absolutely positioned pane and writes a docked height based on it into surface.style.height. The board still looks right only because flex: 1 in the expanded rule overrides the inline height, and the next shrink measures again. Skipping that branch while example has data-expanded would stop the docked height being computed from the wrong box.
| return; | ||
| event.preventDefault(); | ||
| if (event.repeat || !editorOptions.execution) return; | ||
| if (document.querySelector("#example-board")?.hasAttribute("data-expanded")) |
There was a problem hiding this comment.
Cmd/Ctrl+Enter is swallowed here with no feedback, so a candidate who presses it while the board covers the panel sees and hears nothing. Two follow-ups worth considering: an expanded() method on what mountExampleBoard returns, so this check does not reach into the board's DOM, and a short announcement such as "Shrink the drawing to run tests." so the candidate knows why nothing ran.
| const { canvas } = await boardLayout(page); | ||
| const at = (x, y) => ({ x: canvas.x + x, y: canvas.y + y }); | ||
| const openDraft = async () => { | ||
| if ((await draft.count()) === 0) |
There was a problem hiding this comment.
draft locates .board-text-draft, which mountBoardText creates once and only hides on close. So count() is never 0 and openDraft never opens a box: the moved and scaled checks pass only because the box from the new box step is still open. The count() > 0 check before the final Escape has the opposite problem: if the draft were closed, getByRole("textbox", ...) would wait on a hidden element until the test times out. Check visibility in both places:
| if ((await draft.count()) === 0) | |
| if (!(await draft.isVisible())) |
Problem and behavior
Example drawing showed only a strip of its 16:10 canvas inside the test drawer and scrolled the rest: about 30% of it at first and at most 65% with the divider dragged all the way up (the table in #579). An Expand button, first on the drawing toolbar, now lays the board over the editor panel and shows the whole canvas at the largest 16:10 size the panel allows. The left sidebar, with the problem statement and the interview controls, stays visible and usable. Shrink or Escape returns to the editor with the code, the drawer height and the strokes unchanged.
At 1280x800 the canvas goes from 838x143 visible (of 838x524) to the whole 838x524.
Expanded, with "Hello" written on the board:
Shrunk back, after asking Jim what is on the board; he answers that it says "Hello":
Approach
The board keeps its one canvas and only its container moves, so the strokes, undo and the image Jim reads through
read_boardare the same before and after expanding. The button names its action (Expand drawing / Shrink drawing) and carries noaria-pressed, following the one-pattern toggle rule from #251. Escape shrinks the board unless it comes from a field or a dialog, from an input method composing, as a held repeat or with a modifier, and neither Escape nor the button acts during an ink stroke or while a text box is drawn, moved or scaled, so nothing is cut or moved by a size change. Focus stays where the key came from.Nothing the board covers can be operated while it is expanded: the rest of the editor panel and the board's own summary are
inert, Jim's full stage is hidden, and Cmd/Ctrl+Enter is taken but runs nothing, since its results would land in the covered drawer. The sidebar stays usable, and Shrink or Escape restores all of it; if something closes Example drawing while it is expanded, the board shrinks and the editor is freed as well. Four cases keep the stage in view, on top of the board: the wait for the report, where it is lifted over the overlay so Jim's closing lines are seen and announced; the compact card of a narrow window, whose captions are the accessible transcript that the stylesheet keeps there on purpose, and which sits over a corner of the board as it sits over the code when docked; blocked audio, so that Resume audio can be pressed; and a stage holding focus, which Resume audio moves to it once audio plays, until focus leaves it, so neither that focus nor the "Audio restored." announcement is lost. Drawing does not move focus, so after Resume audio the stage stays over the board's right side until focus moves on, by Tab, Shrink or a click on a control. The pane sits atz-index: 6, over the stage (5) and under the overlays (10), and the stage is lifted to 7 in the cases where it stays in view outside the wait. While expanded the panel does not scroll, so in #579's 1250x630 window with the divider at its maximum, where the panel overflows by a few pixels, the board stays at the panel's top.In a wide window, while expanded, Jim's stage is out of view: the avatar, Hide avatar, self-view, the turn ring and its status, the audio controls and the Live CC captions, so screen readers do not announce the captions or the turn status either. Jim's voice is unaffected, and the avatar keeps drawing while it is hidden, since the render loop stops only in the cases
docs/avatar-contract.mdlists. Pressing Hide avatar before expanding changes nothing here, since it removes only the avatar box (the same limit as #512), and shrinking brings the stage back with its avatar state as it was. While drawing, a candidate should not need to adjust Jim's volume, so the unreachable audio controls are unlikely to matter; the main cost is the captions, self-view and the avatar, which Shrink or Escape brings back at once. The board stays expanded if Jim moves to the behavioral round, until the candidate shrinks it, and Cmd/Ctrl+Enter runs again once it does.I also tried leaving the stage uncovered, with the board under it and room kept on the right. That costs the canvas the stage's column for the full panel height, including the empty area below the captions: at 1280x800 the canvas drops to about 666x416, about 37% less area, and no longer reaches the right edge. Where the stage sits is #512's decision, so this PR covers it rather than designing around it.
The uncovered variant, not part of this PR:
Validation
node --test tests/browser/example-board-ui.test.js: 21 pass in Chromium. The new tests check, at 1280x800, 1440x900 and 1920x1080, that the expanded board shows the whole canvas over the editor panel only and leaves the sidebar clickable; that strokes drawn before and while expanded stay on the canvas, in the image published for Jim and undoable; that the button or Escape shrinks it but not from a sidebar field, and that Escape leaves focus where it was, on the board's button, a tool or the sidebar; that Escape from an input method, a held repeat or a modifier chord leaves it expanded; that an ink stroke and a text box being drawn, moved or scaled hold off Enter and Space on the button and Escape, docked and expanded, each key checked alone, and that the button works again once the gesture ends; that the covered panel and Jim's stage are out of reach while expanded and exactly as reachable as before after shrinking, by the button and by Escape; that the real ending wait frominterview.htmlkeeps its own Escape; that Jim's stage is hidden outside the wait so Tab skips it; that a click where Hide avatar sits reaches the board; and that closing Example drawing shrinks it and leaves nothing in the panel inert.tests/browser/interview-layout.test.js, on the interview page itself: the setup check, the ending wait and the report each cover an expanded board (this fails with the formerz-index: 30); the board stays within a pixel of the panel's top at 1250x630 with the divider at its maximum, under the wheel and when the panel was scrolled before expanding; after a window resize while expanded, shrinking gives the docked drawing the height the drawer gives it at the new size (without the drawer'sdata-expandedobserver it kept a height measured while expanded, 669px against 120px at 1280x600); the full stage is hidden while expanded at 1440x900 while the compact card at 900x800 stays visible over the board; Resume audio is on top at both sizes when audio is blocked; and when focus moves to the stage and the notice goes, asaudio-playback.jsdoes once audio plays (done by script in the test, which does not run that module), the stage keeps focus and stays shown, with its "Audio restored." status, until focus moves on.tests/browser/run-shortcut.test.jschecks that Cmd/Ctrl+Enter runs nothing while expanded and runs again after shrinking.min-width: 0, which matters only when the expanded canvas is narrower than 32rem, below the issue's sizes; and the pane'sz-index: 6: with the full stage hidden and the stage lifted to 7 where it shows, no test can see it, and it is kept so the board never rises above the overlays../scripts/test.shon macOS, on this change before its rebase onto currentmain(whose new commits touch neitherweb/nortests/browser/) and two comment-only edits: the browser lane ran 2801 tests, 2773 passed and 26 Java tests skipped without a JDK. Two failed in the full run after 149s and 99s,python quadTree sharedandthe preflight preselects the language last interviewed in, in files this PR does not touch; both files pass when run alone. ESLint and indent pass.task-authoring-tests,run-budget-tests,interactive-review-hook-testsandgit-hooksfail for reasons outside this change, as on cleanmainon this machine: norss_kbon macOS, the/private/varpath alias, notimeout,shellcheckorruffinstalled, and a timing ratio.Close #579