Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
There was a problem hiding this comment.
This pull request needs an owner decision before it can be approved, and it has independent fixes to make either way.
Owner decision: no Lightbox requirement yet covers gallery pages sharing one caption row, and this change pursues that direction while the design decision is open. The head also adds new requirements to the Lightbox record: AR9, the controls' backing and contrast with zoom on; and the zoom-settle formula with its motion roles. Approval needs a spec-gate owner to settle these: either approve the shared caption row and how it reserves its height (which today conflicts with the style-authoring rule that content renders once), or keep each caption under its own photo. An engineering owner records the decision by commenting /approve-spec <full head SHA>, and the author may do so as an engineering owner.
Blocking findings:
- The caption row renders hidden copies of the unmounted pages' captions to reserve its height, which the component style-authoring architecture's INV7 ("Content renders once") rules out unless an owner amends it.
- Against
mainthe diff is 8,108 authored lines, because the three stacked pull requests ride along with this 250-line change, so land them first or retarget so this pull request carries only its own commit. - In a gallery with
hasZoom, a click or tap on the reserved caption band under a photo without a caption no longer closes the viewer, whilemainand a gallery without zoom close it (useZoomStage.ts, the stage's tap only counts points inside the stage). .changeset/lightbox-gallery-paging.mdstill says each caption sits "right under its photo", but it now sits at the bottom of the stage, 252 px below a landscape photo on a phone.- The Lightbox record's FR5 requires the page settle to play named motion roles, and the head records it as playing the pager's own values instead, so register those values as roles or record an owner exception.
[Automated review]
| const onStage = | ||
| point.x >= 0 && | ||
| point.x <= geometry.width && | ||
| point.y >= 0 && | ||
| point.y <= geometry.height; | ||
| if (onStage && !onPhoto) { | ||
| optionsRef.current.onTapOutside?.(); |
There was a problem hiding this comment.
In a gallery with hasZoom, the caption row reserved for the tallest caption lies outside the stage, so a tap or click on that dark band under a captionless photo matches neither branch and the viewer stays open. main closes on it, and so does a gallery without zoom (Lightbox.tsx:885-887). Close for any point that is neither on the photo nor on a [data-lightbox-caption], as the no-zoom recognizer does.
|
|
||
| [feat] A Lightbox gallery now pages the way the iOS Photos viewer does. On a touch screen, drag or flick between items: the row follows your finger, and the release lands on the next or previous item, one at most. A gap of 40 px separates the items while they move. A zoomed photo pans with your finger until it reaches its edge; a new drag from that edge moves to the next item. A photo you leave zoomed returns to its fitted size once the gallery rests. Under a finger, the previous and next buttons and the arrow keys slide to the item instead of swapping it in place. | ||
|
|
||
| Each item's caption moves with its item, right under its photo. `onIndexChange` also reports a swipe. |
There was a problem hiding this comment.
With the shared caption row the caption no longer sits right under its photo. It sits at the bottom of the stage: 252 px below a landscape photo on a 390 × 844 viewport. Suggested: "Each item's caption moves with its item, in a caption row the gallery's photos share."
| const unrenderedCaptions = isGallery | ||
| ? mediaArray.flatMap((item, i) => | ||
| item.caption && !pageIndexes.includes(i) ? [i] : [], | ||
| ) | ||
| : []; |
There was a problem hiding this comment.
These captions render again, hidden, only to reserve the row's height. The style-authoring architecture's INV7 rules this out ("Content renders once: no hidden twin to measure or reserve space"). It needs an owner decision or a different way to reserve the height.
8004b0f to
f53416e
Compare
There was a problem hiding this comment.
This pull request needs an owner decision before it can be approved, and it has independent fixes to make either way.
Owner decision: no Lightbox requirement covers gallery pages sharing one caption row, and the head also adds new requirements to the Lightbox record: AR9 (the controls' backing and contrast with zoom on) and the zoom-settle formula with its motion roles. A spec-gate owner needs to settle these: approve the shared caption row and how it reserves its height, which today conflicts with the style-authoring rule that content renders once, or keep each caption under its own photo. An engineering owner records the decision by commenting /approve-spec <full head SHA>, and the author may do so as an engineering owner.
Blocking findings:
- The caption row renders hidden copies of the unmounted pages' captions to reserve its height (
Lightbox.tsx, the caption sizer), which the component style-authoring architecture's INV7 ("Content renders once") rules out unless an owner amends it. - Desktop behavior change needs an owner decision: a mouse drag or a trackpad's sideways scroll now pages every gallery, with default props (
Lightbox.tsx, the pager'sallowedPointerTypes), so gate it to touch input or cite the decision that covers it. - Against
mainthe diff is 8,175 authored lines, because the three pull requests below ride along with this 294-line change, and their bodies carry a different stack line, so land them first or make the stack declaration identical in all four. - The Lightbox record's FR5 requires the page settle to play named motion roles, and the head records it as playing the pager's own values instead, so register those values as roles or record an owner exception.
[Automated review]
| </div> | ||
| )} | ||
| <div | ||
| aria-hidden |
There was a problem hiding this comment.
These captions render a second time, hidden and inert, only to reserve the caption row's height. The component style-authoring architecture's INV7 rules this out ("Content renders once: no hidden twin to measure or reserve space"), and the open dialog grows from 27–28 to 37–43 elements in a four-item captioned gallery. Either an owner amends INV7, or the row reserves its height without copying content, or each caption stays with its photo.
| // Every pointer pages, a mouse's drag and a trackpad's scroll included. | ||
| // A drag that starts on a video's controls stays the video's. | ||
| panOptions: { | ||
| allowedPointerTypes: ['touch', 'pen', 'mouse'], |
There was a problem hiding this comment.
Desktop behavior change needs an owner decision: with 'mouse' here and no touch gate, a mouse drag or a trackpad's sideways scroll pages every gallery, with default props (a drag from x 900 to 300 at 1280 × 800 moves from 1 / 4 to 2 / 4; main stays on 1 / 4). Gate this paging to touch input, or cite the decision that covers it.
There was a problem hiding this comment.
The owner decision is the current record: component:Lightbox FR11 on main says "A mouse, a pen, and a trackpad also use FR7's drag paging…". This paging comes from #8197, where the engineering owner stamped it.
| thumbnail transition. | ||
| - **Touch presentation (FR8–FR10, FR12).** On a touch screen there is no drag | ||
| or pinch to close, tap to hide the controls, or thumbnail transition. | ||
| - **Paging motion roles (FR5).** The page settle and the buttons' slide play |
There was a problem hiding this comment.
FR5 requires the page settle to be "a named motion role ... read through the motion role table, never a literal". This gap records the shipped page settle and button slide as playing the pager's own values instead. Register them as roles, as zoomMedia.ts does for the zoom settle, or record an owner exception.
f53416e to
e44bf26
Compare
There was a problem hiding this comment.
This pull request needs a spec owner's decision before it can be approved, and it has independent fixes to make either way.
Spec decision: no Lightbox requirement covers gallery pages sharing one caption row, and the head adds new requirements to the Lightbox record: AR9 (the controls' backing and contrast with zoom on), the zoom-settle formula, and its motion roles. A spec-gate owner needs to approve the shared caption row and how it reserves its height, which today conflicts with the style-authoring rule that content renders once, or keep each caption under its own photo, and approve or reject the new record text. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner.
Blocking findings:
- The caption row renders hidden, inert copies of the unrendered pages' captions to reserve its height (
Lightbox.tsx, the caption sizer), which the component style-authoring architecture's INV7 ("Content renders once") rules out unless an owner amends it. - Desktop behavior change needs an owner decision: with default props a mouse drag or a trackpad's sideways scroll now pages every gallery (
Lightbox.tsx, the pager'sallowedPointerTypes), and with zoom a ctrl-wheel zooms the photo and a released mouse drag coasts (useZoomStage.ts), so gate these to touch input or cite the decision that covers them. - Against
mainthe diff is 8,172 authored lines, because the three pull requests below ride along with this 294-line change and the stack line is not identical in every listed pull request, so land them first or make the stack declaration hold. - The Lightbox record's FR5 requires the page settle to play named motion roles, and the head records it as playing the pager's own values instead, so register those values as roles or record an owner exception.
[Automated review]
| </div> | ||
| )} | ||
| <div | ||
| aria-hidden |
There was a problem hiding this comment.
These captions render a second time, hidden and inert, only to reserve the caption row's height. The component style-authoring architecture's INV7 rules this out ("Content renders once: no hidden twin to measure or reserve space"), and the open dialog grows from 27–28 to 37–40 elements in a captioned gallery. The copies also move to the new current page on every page change, so a caption's own content mounts twice and remounts each time. Either an owner amends INV7, or the row reserves its height without copying content.
| // Every pointer pages, a mouse's drag and a trackpad's scroll included. | ||
| // A drag that starts on a video's controls stays the video's. | ||
| panOptions: { | ||
| allowedPointerTypes: ['touch', 'pen', 'mouse'], |
There was a problem hiding this comment.
Desktop behavior change needs an owner decision: with 'mouse' here and no touch gate, a mouse drag or a trackpad's sideways scroll pages every gallery with default props (a drag from x 900 to 300 at 1280 × 800 moves from 2 / 5 to 3 / 5; main stays on 2 / 5). FR11 is current, but the record lists these fine-pointer gestures as not built yet, so this is new desktop behavior rather than a fix. Gate this paging to touch input, or cite the decision that covers it.
| thumbnail transition. | ||
| - **Touch presentation (FR8–FR10, FR12).** On a touch screen there is no drag | ||
| or pinch to close, tap to hide the controls, or thumbnail transition. | ||
| - **Paging motion roles (FR5).** The page settle and the buttons' slide play |
There was a problem hiding this comment.
FR5 requires the page settle to be "a named motion role ... read through the motion role table, never a literal". This gap records the page settle and button slide as playing the pager's own values instead. Register them as roles named by this record, as zoomMedia.ts does for the zoom settle, or record an owner exception.
e44bf26 to
c98544b
Compare
There was a problem hiding this comment.
This pull request needs a spec owner's decision before it can be approved, and it has independent fixes to make either way.
Spec decision: no Lightbox requirement covers gallery pages sharing one caption row, and the head adds new requirements to the Lightbox record: AR9 (the controls' backing and contrast with zoom on), the zoom-settle formula and its motion roles. A spec-gate owner needs to approve the shared caption row and how it reserves its height, which today conflicts with the style-authoring rule that content renders once, or keep each caption under its own photo, and approve or reject the new record text. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner.
Blocking findings:
- The caption row renders hidden, inert copies of the unrendered pages' captions only to reserve its height (
Lightbox.tsx, the caption sizer), which the component style-authoring architecture's INV7 ("Content renders once") rules out unless an owner amends it. - The Lightbox record's FR5 requires the page settle to play named motion roles, and the head records it as playing the pager's own values instead, so register those values as roles or record an owner exception.
- Against
mainthe diff is 8,442 authored lines, because the three pull requests below ride along with this 294-line change and #8215 does not contain #8197's head, so land the lower pull requests first or make every listed head contain the one below it. - Regression: the Chinese
hasZoomdescription still says every zoomed drag pans with momentum, while a mouse drag now stops at the release, so narrow it as the English description was narrowed.
Drag paging is now limited to touch input, so a mouse and a trackpad page a gallery as they do on main.
[Automated review]
| type: 'boolean', | ||
| description: '启用双击缩放(仅图片)。缩放后可拖动平移。', | ||
| description: | ||
| '启用缩放(仅图片):双指捏合(或按住 Ctrl 滚动滚轮),或双点按、双击在该处放大并还原;Enter/Space/+/- 键以中心放大 2 倍并还原。缩放后可拖动或在触控板上滚动平移(带惯性),或用方向键平移。', |
There was a problem hiding this comment.
Regression: this still says every zoomed drag pans with momentum (带惯性), but at this head a mouse drag pans 1:1 and stops at the release; only a finger in the touch presentation and a trackpad's scroll coast, as the English description at line 101 now says. Suggested: 缩放后可拖动或在触控板上滚动平移,或用方向键平移;在手机或平板上以及使用触控板滚动时,平移带惯性。
c98544b to
20b897a
Compare
There was a problem hiding this comment.
This pull request needs a spec owner's decision before it can be approved, and it has independent fixes to make either way.
Spec decision: no Lightbox requirement covers gallery pages sharing one caption row, and the head adds new requirements to the Lightbox record: AR9 (the controls' backing and contrast with zoom on), the zoom-settle formula and its motion roles, beside unrecorded video and index behaviours. A spec-gate owner needs to approve the shared caption row and how it reserves its height, which today conflicts with the style-authoring rule that content renders once, or keep each caption under its own photo, and approve or reject the new record text. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner.
Blocking findings:
- In a captioned gallery without zoom, a tap or a mouse click on the dark band between a photo and its caption, which now sits in the bottom row, leaves the viewer open, while
mainand a zoomable gallery close on it, because the hit test inLightbox.tsxtreats one box spanning photo and caption as the media. - The caption row renders hidden, inert copies of the unrendered pages' captions only to reserve its height, which the component style-authoring architecture's INV7 ("Content renders once") rules out unless an owner amends it.
- The Lightbox record's FR5 requires the page settle to play named motion roles, and the head records it as playing the pager's own values instead, so register those values as roles or record an owner exception.
- Against
mainthe diff is 8,622 authored lines, because the three pull requests below ride along and #8215 does not contain #8197's head, so land the lower pull requests first or declare a stack line that every listed head satisfies. - Miss: the caption sizer picks captions with a truthiness test (
item.caption && …) instead ofisRenderable, so a0caption reserves no height and atruecaption reserves an empty box.
The Chinese hasZoom description now matches the English one.
[Automated review]
| // and its caption, as one box. The photo takes no pointer events, so a | ||
| // press on it lands on its box. An element with no size yet (a photo still | ||
| // loading) adds nothing to the box. | ||
| const isOnCurrentMedia = (x: number, y: number) => { |
There was a problem hiding this comment.
With the shared caption row the caption no longer touches its photo, so this one box spanning photo and caption takes in the dark band between them. In a captioned gallery without zoom, a tap or a mouse click there now leaves the viewer open (274 px of band on a 390 × 844 phone), while main and the hasZoom path close on it. Counting a point as media only inside the photo's rows or the caption's own box would restore that.
| // the photo row. | ||
| const unrenderedCaptions = isGallery | ||
| ? mediaArray.flatMap((item, i) => | ||
| item.caption && !pageIndexes.includes(i) ? [i] : [], |
There was a problem hiding this comment.
Miss: this presence test uses truthiness rather than isRenderable, so a 0 caption beyond the neighbours reserves no height and a true caption reserves an empty box. Suggested: isRenderable(item.caption) && !pageIndexes.includes(i).
| </div> | ||
| )} | ||
| <div | ||
| aria-hidden |
There was a problem hiding this comment.
These captions render again, hidden and inert, only to reserve the caption row's height. The component style-authoring architecture's INV7 rules this out ("Content renders once: no hidden twin to measure or reserve space"), so it needs an owner decision or a different way to reserve the height.
| thumbnail transition. | ||
| - **Touch presentation (FR8–FR10, FR12).** On a touch screen there is no drag | ||
| or pinch to close, tap to hide the controls, or thumbnail transition. | ||
| - **Paging motion roles (FR5).** The page settle and the buttons' slide play |
There was a problem hiding this comment.
FR5 requires the page settle to be "a named motion role ... read through the motion role table, never a literal". This gap records the page settle and button slide as playing the pager's own values instead. Register them as roles named by this record, as zoomMedia.ts does for the zoom settle, or record an owner exception.
20b897a to
86d0dd1
Compare
There was a problem hiding this comment.
Changes requested for this stack. Every record change carries an engineering owner's spec approval at its head, so no spec decision is pending, but #9810 has three fixes to make. A pull request lands only when it and every pull request below it are approved.
#8147: approved on its own changes.
#8148: approved on its own changes. It lands once #8147 lands.
#8197: approved on its own changes. It lands once #8147 and #8148 land.
#9810: changes requested
- Miss: in a zoomable gallery, Enter or Space on "more" or "less" zooms the photo instead of toggling the caption, because the toggle sits inside the zoom stage whose Enter/Space handler takes keys from every descendant; handle only the stage's own keys and render the caption band outside the
role="button"stage, with a test that Enter on "more" opens the caption. - Clicking "less" briefly unmounts the toggle, because the clamp measurement taken while the caption was open reads "not clamped", so focus falls to the page body and the arrow keys stop paging; keep the closed-state measurement while the caption is open and add a test that focus stays on the toggle.
- The approved FR17 says Lightbox owns no colours beyond AR9's backing and on-dark colour, but every captioned gallery's band and the open caption paint their own 90 % dark backing; name that backing in FR17 with a new approval, or remove it.
Whole stack: it fails on the #9810 findings above; it now merges cleanly with main, and the earlier zoom-settle and focus findings are fixed.
[Automated review]
| isZoomTarget && !isZoomed && styles.imageWrapperZoomable, | ||
| isZoomTarget && isZoomed && styles.imageWrapperZoomed, | ||
| )} | ||
| onKeyDown={isZoomTarget ? handleImageKeyDown : undefined} |
There was a problem hiding this comment.
The caption band (and its "more"/"less" Button) renders inside this role="button" stage, so handleImageKeyDown receives Enter and Space from the toggle, calls preventDefault() and zooms the photo; the caption never opens from the keyboard. Act only when e.target === e.currentTarget, and render the band outside the stage.
There was a problem hiding this comment.
Fixed in 645984d: the gallery's zoom button is now the current photo's box, with the caption band outside it. Enter or Space on "more" opens the caption, and a browser test covers it.
| {mediaArray[i].caption} | ||
| </Text> | ||
| {i === index && | ||
| (isCaptionClamped || isCaptionOpen) && ( |
There was a problem hiding this comment.
While the caption is open, measureCaptionClamp records isCaptionClamped = false (no clamp at maxLines={0}), so the render after "less" drops this Button and focus falls to the page body; the next ArrowRight no longer pages. Keep the closed-state measurement while the caption is open.
There was a problem hiding this comment.
Fixed in 645984d: the toggle keeps the closed caption's measurement while the caption is open. A test checks that focus stays on it after "less".
|
|
||
| // The caption's backing: the controls' backing colour, darker, so the caption | ||
| // keeps 4.5:1 over any photo or backdrop behind it (WCAG 2.2 1.4.3). | ||
| const CAPTION_BACKING = `color-mix(in srgb, ${colorVars['--color-on-light']} 90%, transparent)`; |
There was a problem hiding this comment.
FR17, as approved, allows "no Lightbox-owned colors beyond AR9's backing and on-dark color", and AR9 covers only the controls. This backing is painted under every captioned gallery's band, with or without hasZoom. Name it in FR17 or drop it.
There was a problem hiding this comment.
Fixed in 645984d: FR17 now names the caption's backing, approved at that head.
| const pageIndexes = isGallery | ||
| ? [index - 1, index, index + 1].filter(i => i >= 0 && i < mediaArray.length) | ||
| : []; | ||
| // Every page keeps a caption band once any item has a caption. |
There was a problem hiding this comment.
Non-blocking (INV29): this comment restates hasCaptionBand on the next line; drop it.
5a2ba52 to
645984d
Compare
|
/approve-spec 645984d |
There was a problem hiding this comment.
Changes requested for this stack. Every record change carries an engineering owner's spec approval at its head, so no spec decision is pending, and the three earlier #9810 fixes are in: Enter and Space on "more" toggle the caption, "less" keeps its focus, and FR17 names the caption's backing. One #9810 fix remains. A pull request lands only when it and every pull request below it are approved.
#8147: approved on its own changes.
#8148: approved on its own changes. It lands once #8147 lands.
#8197: approved on its own changes. It lands once #8147 and #8148 land.
#9810: changes requested
- The new focus repair after paging (
Lightbox.tsx:1064-1091) re-implements whatnoteFocusWithinandmoveFocusLeftBehindinutils/focusReturn.tsalready do for Collapsible, LayoutPanel and SideNav, and its own lost-focus test already differs fromisFocusDetached; note the current page withnoteFocusWithinbefore the index changes, callmoveFocusLeftBehindfrom the layout effect, and delete the copy.
Whole stack: it fails on the #9810 finding above; it still merges cleanly with main.
[Automated review]
| const noteFocusBeforePaging = useCallback(() => { | ||
| const active = imageWrapperRef.current?.ownerDocument.activeElement; | ||
| refocusAfterPagingRef.current = | ||
| active != null && | ||
| (active === captionToggleRef.current || active === pageMediaRef.current); | ||
| }, []); | ||
| // Effect: syncs DOM focus with the new page, since the browser drops focus | ||
| // from the page that turned inert. | ||
| useIsomorphicLayoutEffect(() => { | ||
| if (!refocusAfterPagingRef.current) { | ||
| return; | ||
| } | ||
| refocusAfterPagingRef.current = false; | ||
| const doc = imageWrapperRef.current?.ownerDocument; | ||
| const active = doc?.activeElement; | ||
| const isFocusDropped = | ||
| active == null || | ||
| active === doc?.body || | ||
| active.closest('[inert]') != null; | ||
| const target = isZoomTarget ? pageMediaRef.current : closeButtonRef.current; | ||
| if (isFocusDropped && target != null) { | ||
| focusForLastInput(target); | ||
| } | ||
| }, [index, isZoomTarget]); |
There was a problem hiding this comment.
This repeats the shared focus-return path in utils/focusReturn.ts: noteFocusWithin(<current page>) before setIndex, then moveFocusLeftBehind(note, target, <that page>) from this layout effect, as Collapsible, LayoutPanel and SideNav do. The copy already drifts: isFocusDropped misses the documentElement case that isFocusDetached counts. Call the shared pair and delete refocusAfterPagingRef's comparison and isFocusDropped.
There was a problem hiding this comment.
Fixed in 57425e7: paging notes the current page with noteFocusWithin and moves focus with moveFocusLeftBehind from the layout effect. The copy is gone.
645984d to
57425e7
Compare
|
/approve-spec 57425e7 |
There was a problem hiding this comment.
Changes requested on #9810; #8147, #8148 and #8197 stay approved.
#8147: approve. No blocking findings.
#8148: approve. No blocking findings.
#8197: approve. No blocking findings.
#9810: request changes. The paging focus repair now goes through the shared focus-return helpers, which settles the previous finding.
- Miss: closing the viewer returns focus with
focusForLastInput(triggerElementRef.current)(Lightbox.tsx:709) instead of the sharedreturnFocusForLastInputthat Dialog, BottomSheet, DropdownMenu and Lightbox's own removal path call, so it drops that helper'spreventScrolland a later fix to the shared return would miss Lightbox; callreturnFocusForLastInputthere.
Whole stack: fails on the #9810 finding. A pull request lands only when it and every pull request below it are approved.
[Automated review]
| dialog.close(); | ||
| if (triggerElementRef.current instanceof HTMLElement) { | ||
| triggerElementRef.current.focus(); | ||
| focusForLastInput(triggerElementRef.current); |
There was a problem hiding this comment.
Miss: this is the overlay focus return that returnFocusForLastInput (utils/focusReturn.ts) already owns: Dialog, BottomSheet, DropdownMenu and this file's own removal path (prepareFocusReturnOnRemoval) call it. This copy drops its preventScroll. Suggested change:
returnFocusForLastInput(triggerElementRef.current);There was a problem hiding this comment.
Fixed in 053f321: the close returns focus with returnFocusForLastInput, and a unit test checks the preventScroll.
| captionBandUnderOpen: { | ||
| backgroundColor: 'transparent', | ||
| }, | ||
| // A caption in the band, and its "more" button after its last line. |
There was a problem hiding this comment.
Non-blocking (INV29): this comment restates the style's name and contents; drop it.
| navNext: { | ||
| insetInlineEnd: spacingVars['--spacing-3'], | ||
| }, | ||
| // The counter's box; its type and color are its Text's. |
There was a problem hiding this comment.
Non-blocking (INV29): this comment restates the code; drop it.
57425e7 to
053f321
Compare
|
/approve-spec 053f321 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. As a draft from a fork, it runs once the pull request is marked ready and a maintainer adds the ci:run label.
#8147: approve. No blocking findings.
#8148: approve. No blocking findings.
#8197: approve. No blocking findings.
#9810: approve. No blocking findings: closing the viewer now returns focus through the shared returnFocusForLastInput, which settles the previous finding.
Whole stack: passes. A pull request lands only when it and every pull request below it are approved.
- Non-blocking:
GalleryKeepsIndexlost its story description (apps/storybook/stories/Lightbox.stories.tsx:191), which documented the refused-index spring-back; consider restoring it.
[Automated review]
053f321 to
c56f4e1
Compare
|
/approve-spec c56f4e1 |
There was a problem hiding this comment.
Changes requested on #9810; #8147, #8148 and #8197 stay approved.
#8147: approve (merged). No blocking findings.
#8148: approve. No blocking findings in its own change.
#8197: approve. No blocking findings; it needs a rebase onto #8148's new head.
#9810: request changes. The record text is approved at this head; four fixes remain:
- Miss: Lightbox turns off
Text's own truncation measurement and re-checks the clamp itself only when the caption's box resizes, pages or opens (Lightbox.tsx:868-905), so a caption whose text grows past two lines while the viewer is open shows "…" with no "more" button, against FR1; measure it through the shareduseTruncationhook, extended for this case, and drop the copy. - While "more" shows, the caption's text moves into a new wrapper beside two float spans, and every caption and the toggle gain wrappers, but the component style-authoring rules say a state never adds elements and every added element needs counts before and after (
Lightbox.tsx:1633,1660-1683,1691), so render one structure in every state and record the counts (the open dialog grows from 38 to 53 elements on a clamped page). - Miss: a single photo's caption has no backing and measures 4.00:1 on the default backdrop over a white page (
Lightbox.tsx:1732), while the new FR17 verification row says a single photo's caption under 4.5:1 fails, so give it the caption's backing or narrow that row to gallery captions. - Regression: since #8147 merged into
mainas its own commit, this branch conflicts withmaininzoomStage.ts,zoomStage.test.ts,motion/uikit/zoomMedia.ts,motion/MEASUREMENTS.mdand.changeset/uikit-zoom-media.md, so rebase #8197 onto #8148's new head, then this pull request.
Whole stack: fails on #9810's findings. A pull request lands only when it and every pull request below it are approved. Required CI has not run on this head yet.
[Automated review]
| {i === index && | ||
| hasCaptionTogglePlace ? ( | ||
| <div | ||
| {...stylex.props( | ||
| styles.captionFlow, | ||
| )}> | ||
| <span | ||
| aria-hidden="true" | ||
| {...stylex.props( | ||
| styles.captionToggleSpacer, | ||
| styles.captionToggleSpacerHeight( | ||
| captionLastLineTop, | ||
| ), | ||
| )} | ||
| /> | ||
| <span | ||
| aria-hidden="true" | ||
| {...stylex.props( | ||
| styles.captionTogglePlace, | ||
| styles.captionTogglePlaceWidth( | ||
| captionToggleWidth, | ||
| ), | ||
| )} | ||
| /> |
There was a problem hiding this comment.
While the toggle shows, the caption's text moves into this captionFlow div beside two empty float spans; in every other state it renders bare. The component style-authoring rules (INV7) say a state never adds elements, and every added element needs measured counts before and after. Render this structure in every state, with the floats sized by style (zero without a toggle), and record the before and after element counts in the description.
There was a problem hiding this comment.
Fixed in 0fc994c. Every captioned page now renders the same elements in every state: without a toggle, the two floats have no size. The toggle's slot is merged into the Button. The description's "Minimal DOM" section lists the element counts before and after, per state, and screenshots of all four states match the previous head pixel for pixel.
Each page of a Lightbox gallery laid out its own photo and caption, so a neighbour with a longer caption arrived with a smaller photo. Every page now has a caption band two caption lines tall under its photo, whatever the caption, so the photo area is the same on every page and paging never resizes it. A page with no caption keeps the band's space, so its photo sits where every other page's does, but it shows no backing there. A longer caption shows two lines, and its second line ends in a "more" button, so the caption keeps the band's whole width. The toggle is one line tall, so its fill stays on that line. The button opens the whole caption over the bottom of the photo, on the scrim the controls use, and its "less" closes it. An open caption too long for its panel scrolls, and it keeps the toggle's width clear at the end of every line. Paging closes it too. The whole caption is always the viewer's accessible description, clamped or open. The viewer lays out no hidden copies of other captions. Each caption renders the same elements in every state: without a toggle, the toggle's place in its last line has no size. The viewer renders inside MediaTheme, dark, so its controls, caption and counter take the theme's dark surface tokens and its rules for them, with no Lightbox-owned colors beyond the controls' and the caption's backing. The caption is body Text, both of its lines start at one edge, and "more" and "less" are a ghost Button. The band clears the screen's bottom edge and its safe area. Focus the viewer moves follows the last input, so a tap that opens it paints no ring. The caption sits on its own backing, the controls' backing colour at 90 %, so it keeps 4.5:1 over any photo or backdrop behind it. With zoom, the controls' icons and the Counter's text keep the on-dark colour, whatever a theme does to buttons or Text. An open caption is the clamped caption itself, unclamped over the photo's bottom, so the caption renders once. In a gallery the keyboard's zoom button is the current page's photo box, so the caption band and its button sit outside it, and Enter or Space on "more" toggles the caption. Paging from a focused "more", "less" or photo moves focus to the new page's photo, or to Close without zoom, so the arrow keys keep paging. "Less" keeps its focus.
|
/approve-spec c56f4e1 |
c56f4e1 to
0fc994c
Compare
|
/approve-spec 0fc994c |
There was a problem hiding this comment.
Changes requested: the record text is approved for this head, and four blocking issues remain.
This pull request was reviewed on its own because its Stack: line no longer holds: #8148 has merged, and #8197's current head is not contained in this branch.
- Miss: A gallery caption that fills its two lines and then grows while the viewer is open never shows "more", because Lightbox re-measures the clamp only on resize, attach and open or close instead of using
useTruncation, which also watches content changes (Lightbox.tsx:862-899). - Miss:
useZoomStage.ts:288-311adds its own capturepointerdownlistener to set the pan's begin threshold, which the sharedPanGestureRecognizeralready takes as a per-presshysteresisfunction, so pass it there and drop the listener and its gap sentence, as #8197's current head does. - Miss: A single photo's caption has no backing and measures 4.0:1 on the default backdrop over a white page, under the 4.5:1 that FR17's verification row requires, while the caption-row changeset claims 4.5:1 over any photo, so give it the caption's backing or narrow the row and the changeset to gallery captions.
- Regression: The branch no longer merges into
main(13 conflicting files, includingLightbox.tsxanduseZoomStage.ts) and, with the stack broken, carries 10,763 authored lines as one change, so rebase #8197 ontomainand this commit onto #8197.
CI has not built or tested this head.
[Automated review]
| setIsCaptionClamped( | ||
| caption != null && caption.scrollHeight > caption.clientHeight + 1, | ||
| ); |
There was a problem hiding this comment.
Miss: this clamp check re-runs only when the caption's box resizes, a page attaches or the caption opens or closes. A caption whose text grows while its box stays two lines tall never gets "more": in an open gallery whose caption grows from 64 to 277 characters, the Text holds 152 px of content in its 56 px clamp and no toggle renders. useTruncation (Text/useTruncation.ts) already owns this measurement and also observes content changes. Extend it so a caller reads isTruncated with the tooltip off and keeps the clamped result while the caption is open, then delete this copy.
| <Text | ||
| ref={captionRef} | ||
| id={captionId} | ||
| as="div" | ||
| type="body" | ||
| xstyle={styles.caption}> |
There was a problem hiding this comment.
Miss: a single photo's caption gets no backing, so it is white on the default backdrop: 3.98:1 at 14 px over a white page. FR17's verification row names "a single photo's caption" and fails "a caption under 4.5:1". Give it the caption's backing, as the gallery band has, or narrow the row to gallery captions and record the single caption as a gap.
|
|
||
| [feat] A Lightbox gallery's caption sits in a band two lines tall under every photo, so the photo keeps one size and place from item to item. A photo with no caption keeps the band's space without its backing. A longer caption shows two lines, the second ending in a "more" button, which opens the whole caption over the photo. The whole caption is always the viewer's accessible description. | ||
|
|
||
| The viewer is a media surface: its controls, caption and counter take the theme's dark surface tokens through `MediaTheme`. The caption is body `Text` whose lines start at one edge, "more" and "less" are a ghost `Button`, and the band clears the screen's bottom edge and its safe area. The caption sits on a backing that keeps it at 4.5:1 over any photo. A tap that opens the viewer paints no focus ring on its first control, and paging from a focused "more" or "less" keeps focus in the viewer. |
There was a problem hiding this comment.
"The caption sits on a backing that keeps it at 4.5:1 over any photo" holds only in a gallery: a single photo's caption has no backing and measures 3.98:1 over a white page. Unless the single caption gains the backing, suggested: "A gallery's caption sits on a backing that keeps it at 4.5:1 over any photo."
| minWidth: 0, | ||
| maxWidth: '100%', | ||
| }, | ||
| // Text's two-line clamp. |
There was a problem hiding this comment.
Non-blocking (INV29): the clamp is Text's own; this style only lets the clamped Text shrink. Suggested: // Lets the clamped Text shrink inside the band.
Stack: #8147 → #8148 → #8197 → #8215, #9810 (both on #8197)
Stacked on #8197's 0fc7062 (the merge worker's head), on main 1dce318. Review this pull request's one commit: vjeux@0fc994c
The design owner's option C for a gallery's captions (card 46): a constant photo area; the caption in a fixed two-line band; a longer caption clamps, and "more" opens the full caption over the bottom of the photo on a scrim; the full caption is always in the accessible description; no hidden sizing copies.
It answers the design owner's re-check (card 46C (Cindy)): the caption sat too close to the bottom, its second line was indented, its size was unusual, and the Close button showed an outline on a tap's opening. The caption must also meet 4.5:1 over a mid-grey photo (Specs decision, Nov 5 2026). Lightbox renders through Astryx parts on a media surface:
MediaTheme(dark), so its controls, caption and counter take the theme's dark surface tokens and its rules for them. No Lightbox color is left beyond the controls' and caption's backing and the on-dark color (AR9).Textin the body type, so both caption lines start at one edge. Owner decision, Nov 5 2026 (Cindy): body text.Buttonone caption line tall, at the end of the caption's last line, so the caption keeps the band's whole width and the clamp's "…" comes just before "more" (card 46D). Its hit area reaches out toward 44 px under a finger, sideways and down, never into the line above. An open caption that scrolls in its panel keeps the toggle's width clear at the end of every line.This round answers the review on the rebased head: the toggle no longer sits inside the zoom button, "less" keeps its focus, and FR17 names the caption's backing. Out of scope: the backdrop keeps the page theme's
--color-overlayand blur, as today.::backdropinherits from the dialog element, outside the media surface.User impact
In a Lightbox gallery, every page has a caption band two caption lines tall under its photo, whatever its caption. So the photo area is the same on every page, and paging never resizes it. A caption longer than two lines shows two lines and a "more" button. "More" opens the whole caption over the bottom of the photo, on a dark backing, and "less" or paging closes it. Under a finger the button takes taps out toward 44 px, and its tap is its own, not the photo's. A screen reader gets the whole caption as the viewer's description, clamped or open.
Contrast
Measured on #10084's exact preview (4c07f5c, stacked on this pull request) in Chromium: default theme, a 402 px phone viewport, coarse pointer, text against what lies under it:
Across the shipped themes (butter, chocolate, gothic, matcha, stone), body stays at or above 7.3:1, matcha the lowest. Supporting in its secondary color falls to 2.7:1 in matcha and 3.8:1 in chocolate over a white photo; the other themes give 5.0–5.6:1.
Record change
component:LightboxFR1 gains: "In a gallery, Caption sits in a band two caption lines tall under every page's Media. A longer caption shows two lines and a "more" button that opens it in full over the bottom of the Media, on a scrim. The full caption is always the viewer's accessible description. The caption band uses Astryx parts: the caption text isTextwith the body type, and "more"/"less" is a ghostButton, not hand-styled elements."MediaTheme(dark), so its controls and caption take the theme's dark surface tokens and the active theme's component rules (architecture:theme-applicationINV8), with no Lightbox-owned colors beyond AR9's backing, the caption's backing and the on-dark color." Its basis reads "Specs decision, Nov 5 2026". Its verification row names the unit file and the browser interaction spec, and states the caption's contrast over a white, a mid-grey and a black photo (body 14.7:1 over white).spec:AST-038's existing-code row now reads: "Lightbox's caption renders through Text under MediaTheme (component:Lightbox)", with the rest of the row unchanged.Writing rubric results
Visual authority
DDrow reused or amended.Design decisionstable by a team design owner.docs/design/.The design owner chose option C on card 46 and asked for these fixes on card 46C; the record states them in FR1 and FR17.
How to check
State matrix
Minimal DOM
Element counts for the current page's caption, measured in Chromium on Storybook builds. Main renders a gallery's caption as one
Textunder the photo, with no clamp and no toggle.Every captioned page renders the same elements in every state. The toggle is the only element that comes and goes, and only while the caption runs past two lines. Each element's job:
aria-controls.Text: the caption itself, also the viewer's description.Button(4 elements with its label), only while the caption runs past two lines. It hosts its own position, so the slot that wrapped it is gone.Testing
Red first:
At this head (0fc994c): Lightbox units 139/139; core
tsc0, strict ESLint 0, comment sentence lengths 0 over. On a Storybook build of this head: Chromium Lightbox a11y specs 38/38; WebKitLightboxViewportandLightboxKeyboard17/17; Lightbox Paging, Zoom and EveryPointer specs 78 passed in Chromium (3 WebKit-only skips); WebKit Paging, EveryPointer and Keyboard 38 passed (5 skips).Lightbox.tsx(the clamped caption isDIV(DIV(SPAN(),SPAN())), the one that fitsDIV()) and passes here.At c56f4e1: Lightbox units 138/138; core
tsc0, strict ESLint 0, comment sentence lengths 0 over, Prettier clean;pnpm check:knowledgeandcheck:repopass. On a Storybook build of this head: Chromium Lightbox a11y specs 38/38; Lightbox Paging, Zoom and EveryPointer specs 55 passed in Chromium (3 WebKit-only skips); WebKit Paging and EveryPointer plus theLightboxViewportcaption cases 34 passed (5 skips).LightboxViewport.a11y.browser.spec.ts: "a gallery page without a caption shows no caption backing, and every page's photo keeps its top" and "a clamped caption's "more" ends its second line, with no column of its own" (402 × 874 and 1440 × 900), and "… still ends its second line after the viewport narrows (402 → 360)" fail on 053f321's build and pass here. "An open caption scrolled to its end keeps every line beside "less" clear of it" (360 × 150, 390 × 150) fails on a build that keeps the inline place while the caption scrolls, and passes here. The unit case "follows the caption in reading order, and its place in the last line is hidden from assistive technology" fails on 053f321's product.iPhone 17 Pro simulator (iOS 26.2, Safari), on c56f4e1's Storybook build, every frame viewed: every page's photo top is 189 pt; page 1's two lines end in "…" and "more" (its last line ends at 318 pt, "more" sits at 328–390 pt on that line); page 2's two lines fit with no toggle; page 3 has no caption and no backing (
rgba(0, 0, 0, 0)); page 4's one line sits on its backing.c56f4e1's preview on the iOS 26.2 simulator (Safari, every frame viewed), pages 1 to 4:
Desktop, Chromium at 1440 × 900. Before (main, which has no captioned gallery story: "Gallery" page 1 and its captionless page 2, and "With caption" for a long caption):
After (c56f4e1, "Gallery with captions"): "more" collapsed, the same caption expanded, the captionless page, and a short caption:
At 053f321: core
tsc0, strict ESLint 0, comment sentence lengths 0 over, Lightbox units 137/137. Red first: the new case "returns focus to the opener without scrolling the page when the viewer closes" fails on 57425e7's product, whose close focused the opener withoutpreventScroll. On a Storybook build of this head: Chromium Lightbox a11y specs 31/31; Paging and EveryPointer 28 passed in Chromium (3 WebKit-only skips); Paging, EveryPointer and Keyboard 26 passed in WebKit (5 skips); the a11y audit finds 0 new violations;pnpm check:knowledgeandcheck:repopass.At 57425e7 (053f321 without the shared focus return): the LightboxKeyboard spec 9/9 in Chromium; Lightbox Paging and EveryPointer specs 28 passed in Chromium (3 WebKit-only skips), and Paging, EveryPointer and Keyboard 26 passed in WebKit (5 skips).
At 40e8001, the same behavior before the focus handoff moved to the shared helpers (the head also differs by the FR17 basis cell and two comments):
tsc0; strict ESLint on every changed file 0; comment sentence lengths 0 over;pnpm check:knowledgeandcheck:repopass (check:changesets,check:i18n-catalog,check:i18n-runtime,check:use-clientandcheck:package-boundariesat 65534b6, the same code without the viewport hook).Existing cases: the fine-pointer touch-action lists gain the media surface's
auto; the scroll case's viewport is 240 px tall so the body-size caption still overflows its panel; the paging specs find the pager's viewport bydata-lightbox-viewport, since the zoom button is now the photo box.iPhone 17 Pro simulator (iOS 26.2, Safari), on 4dded66's build, every frame viewed: a tap's opening leaves Close without a ring, and a key's opening shows it; the caption is body
Text(16 px in the default theme), its lines start at one edge, and the band keeps 12 px under them.