Repository navigation
feat(native): Swift Thumbnail previews an image attachment with open and remove actions - #8323
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request needs an engineering owner's decision before it can be approved: the new draft component:swift/Thumbnail record sets requirements no current spec covers, including the unprefixed view name Thumbnail, FR1–FR6, AR1–AR5 and the pulsing skeleton, and an engineering owner records that decision by commenting /approve-spec <full head SHA> on the head that carries the fixes below (the author may do so if they are an engineering owner). Independently of that decision, three behaviors differ from core's Thumbnail and need fixing:
- A thumbnail that appears already uploading never announces “Loading”, because the announcement only runs on a later change (
Thumbnail.swift:70-76);.onChange(of: showUploadOverlay, initial: true)matches core. - On macOS the default
.hoverremove button is never revealed by keyboard focus, which core requires so the button stays reachable (Thumbnail.swift:173-175). - The remove glyph is hard-coded
xmarkinstead of the theme'scloseicon (Thumbnail.swift:138).
Please also add a largest-Dynamic-Type check to ThumbnailTests.swift, as AST-093 FR5 asks, and land after #8288 so pnpm check:knowledge can validate the record's schema version 5.
[Automated review]
| .accessibilityElement(children: .contain) | ||
| .accessibilityLabel(accessibleName) | ||
| .modifier(ThumbnailIdentifier(id: props.base.id)) | ||
| .onChange(of: showUploadOverlay) { _, isUploading in |
There was a problem hiding this comment.
A thumbnail that appears already uploading never announces: onChange(of:) skips the value the view appears with, while core's useBusyAnnouncement announces on mount (AST-086 FR8). .onChange(of: showUploadOverlay, initial: true) matches core; record AR3 needs the same change.
| var isRemoveVisible: Bool { | ||
| (props.showRemoveOn ?? .hover) == .always || Thumbnail.isTouchDevice || isHovering | ||
| } |
There was a problem hiding this comment.
On macOS (isTouchDevice is false) the default .hover button is revealed only under the pointer. Core also reveals it on keyboard focus so it stays reachable (Thumbnail.tsx:107-109); ORing a focus state for the open and remove buttons into this property would match (AST-082 FR3).
|
|
||
| private var removeButton: some View { | ||
| SwiftUI.Button(action: { props.onRemove?() }) { | ||
| Image(systemName: "xmark") |
There was a problem hiding this comment.
This hard-codes the glyph core draws as the shared close icon, so a theme's close replacement never reaches the thumbnail. astryx.icon("close"), as AstryxButton and AstryxBanner use, follows AST-093 FR4 and AST-082 FR17.
| @@ -0,0 +1,179 @@ | |||
| --- | |||
| schema_version: 5 | |||
There was a problem hiding this comment.
schema_version: 5 and template_version: 8 have no schema or template at this head (docs/schemas/knowledge/ has v1–v4, and the component template is version 7); they arrive with #8288, so FR1's pnpm check:knowledge validation can only run after it lands.
8dac3ee to
4c0b8bc
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new draft component:swift/Thumbnail record, whose requirements (FR1–FR6, ORD1–ORD2, AR1–AR5, AV1–AV2 and PR1) no current spec states yet; an engineering owner records that decision by commenting /approve-spec <full head SHA> on the final head, and the author may do so if they are an engineering owner. Thanks for fixing the upload announcement, the keyboard-focus reveal and the theme close icon; independently of the spec decision, please:
- Draw the image's inset edge on the tile rather than on the fill-scaled image, which clips it to two sides for any non-square photo (
Thumbnail.swift:105-110). - Add a largest-Dynamic-Type check to
ThumbnailTests.swiftas AST-093 FR5 asks, and capture the disabled and theme states at the largest text size and the macOS hover states, or mark them not measured. - Revisit AV2: core's skeleton is already an opacity pulse of
color.skeleton, not a shimmer, while this view pulsescolor.neutral; record validation also waits on #8288.
[Automated review]
| if let image = phase.image { | ||
| image.resizable().scaledToFill() | ||
| .modifier(ThumbnailImageName(alt: Thumbnail.meaningful(props.alt))) | ||
| .overlay( |
There was a problem hiding this comment.
This overlay takes the size of the fill-scaled image, which overflows the 64 pt tile for any non-square photo (a 3:4 photo fills 64 by 85 pt), so the tile's .clipShape at line 129 cuts the edge to two sides and drops its corners. Core draws the edge on the tile (Thumbnail.tsx:159-165). Moving the RoundedRectangle overlay after the picture's .frame(width:height:), or framing and clipping the image to Thumbnail.side before this overlay, keeps it on all four sides.
|
|
||
| - **AV1 — Glyphs and spinner.** The placeholder is the SF Symbol `photo.fill` | ||
| and the spinner the platform's progress view. | ||
| - **AV2 — Skeleton motion.** The skeleton pulses its opacity instead of core's |
There was a problem hiding this comment.
Core's skeleton is already an opacity pulse, of --color-skeleton from 0.25 to 0.925 (Skeleton.tsx:51-72, :78-110), even though core's docs call it a shimmer. The Swift skeleton pulses the plate's own color.neutral from 0.4 to 1 (Thumbnail.swift:101, :233), so the real variation is the token and range; AV2 and the theme values list should say which is intended.
| } | ||
|
|
||
| @MainActor | ||
| @Test func aThumbnailIs64PointsOnTheNeutralPlate() { |
There was a problem hiding this comment.
AST-093 FR5 asks for a check at the largest Dynamic Type size, which the Banner, Button, Heading, Text and TextInput tests carry. A Pixels(Thumbnail(src: …, isLoading: true), typeSize: .accessibility5) case would keep the tile at 64 by 64 and pin the upload spinner, which the pull request's largest-text capture shows covering nearly the whole tile.
65b97cd to
0282a0d
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new draft component:swift/Thumbnail record, whose requirements (FR1–FR6, ORD1–ORD2, AR1–AR5, AV1, PR1 and blank label/alt counting as absent) no current spec states yet; an engineering owner records it by commenting /approve-spec <full head SHA> on the final head, and the author may do so as an engineering owner. That decision needs to settle FR2's skeleton values and whether FR5's pointer-only tooltip is intended on touch screens, where core's Thumbnail opens the label on a tap. Thanks for fixing the inset edge and adding the largest-text test; independently of the decision, please:
- Make FR2 and
ThumbnailSkeletonagree: core's skeleton rests at 25% and pulses 25% to 92.5%, while the view holds full opacity under Reduce Motion and pulses 100% to 40% (Thumbnail.swift:260-274). - Back the verification map's "a hard-coded remove glyph … fails" with a test, or drop the claim; the only related test was removed in the last commit.
- Land after #8288 so
pnpm check:knowledgecan validate the record's schema version 5.
[Automated review]
|
|
||
| var body: some View { | ||
| color | ||
| .opacity(isStill ? 1 : (isDim ? 0.4 : 1)) |
There was a problem hiding this comment.
FR2 says this skeleton pulses "as core's does", but it holds color.skeleton at full opacity under Reduce Motion and pulses from 100% to 40%. Core's Skeleton rests at 25%, pulses 25% to 92.5% after a 1 s delay, and stays at 25% under reduced motion (Skeleton.tsx:51-106). Either draw core's range here or write these values into FR2 for an engineering owner to decide.
| | -------- | -------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------- | ------------------------------- | | ||
| | FR1 | `ThumbnailTests.swift` size, plate, and two-theme renders | no image; default, neutral, butter; the largest text size | a wrong size or a color of its own fails | `audit:swift/Thumbnail/tile` | | ||
| | FR2 | `ThumbnailTests.swift` loading states, skeleton color, contrast mix, and largest-text render | loading with and without `src`; Increase Contrast; the largest text size | a skeleton over an image, no upload look, a wrong skeleton color, or a tile that grows fails | `audit:swift/Thumbnail/loading` | | ||
| | FR3, FR4 | `ThumbnailTests.swift` remove, tap, and disabled tests and render | `always`, `hover`; disabled; loading | a hidden remove on touch or under keyboard focus, a hard-coded remove glyph, or a tap while disabled or loading fails | `audit:swift/Thumbnail/actions` | |
There was a problem hiding this comment.
No test in ThumbnailTests.swift draws or inspects the remove glyph since theRemoveButtonDrawsTheThemesCloseIcon was removed, so "a hard-coded remove glyph … fails" is not backed. A render test with a theme that remaps close, as IconTests.aThemeReplacesASharedIcon does, or dropping the claim would fix it.
888a198 to
3cba83c
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new draft component:swift/Thumbnail record, whose requirements no current spec states yet: FR1–FR5, FR6's initializer rule, ORD1–ORD2, AR1, AR2, AR4, AR5, AV1, PR1, and blank label or alt counting as absent. That decision includes whether OQ1's pointer-only tooltip is acceptable on touch screens, where core's Thumbnail shows the label on a tap and AST-082 FR3 asks touch platforms to behave the same way. An engineering owner records the decision by commenting /approve-spec <full head SHA> on the final head, and the author may do so as an engineering owner. Independently of that decision, please:
- Make FR2 describe a real timing: core's skeleton waits 1 s and climbs in ten steps (
Skeleton.tsx) whileThumbnailSkeletonramps smoothly from its first frame, so either state the Swift timing without "as core's does" or add the delay and the steps. - Land after #8288, so
pnpm check:knowledgecan validate the record's schema version 5.
[Automated review]
| | ID | Candidate invariant | Basis | Draft review state | | ||
| | --- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------- | ------------------ | | ||
| | FR1 | The tile MUST be 64 by 64 points, clipped to `radius.element`, on `color.neutral`. It MUST show the image at `src` cropped to fill the tile, with a 1-point `color.border` inset edge on all four sides of the tile; with no `src`, or when the image fails, a 24-point image silhouette in `color.iconSecondary`. | core `Thumbnail` styles | verify | | ||
| | FR2 | `isLoading` with no `src` MUST show a skeleton in `color.skeleton` at 25% opacity that pulses linearly to 92.5% and back over core's default 550 ms medium duration, as core's does, and rests at 25% under Reduce Motion; under Increase Contrast its color moves 30% toward `color.textPrimary`, as core's does under `prefers-contrast: more`. `isLoading` with a `src` MUST draw `color.overlay` over the image with the platform's small progress view in `color.onDark`, which keeps its size at any text size. | core `Thumbnail`; Swift accessibility X6 | verify | |
There was a problem hiding this comment.
FR2 says the pulse runs linearly "as core's does", but core's Skeleton waits 1 s before it pulses (DELAY_TIME, Skeleton.tsx:30) and climbs from 25% to 92.5% in ten held steps (Skeleton.tsx:42-72), while ThumbnailSkeleton ramps smoothly from its first frame (Thumbnail.swift:276-278). Either state the Swift timing here without "as core's does", for an engineering owner to approve, or add core's delay and steps to the view.
| .opacity(isBright && !isStill ? Self.brightestOpacity : Self.restingOpacity) | ||
| .onAppear { | ||
| guard !isStill else { return } | ||
| withAnimation(.linear(duration: Self.pulseDuration).repeatForever(autoreverses: true)) { isBright = true } |
There was a problem hiding this comment.
This starts the pulse on the first frame and ramps it smoothly. Core holds the skeleton at 25% for 1 s, a delay that "Prevents flash of animation for fast-loading content" (Skeleton.tsx:27-30), and then steps it in ten holds. A load under a second therefore flashes here but not on the web, and FR2 needs to match whichever timing is intended.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new draft component:swift/Thumbnail record, whose Swift presentation (FR1–FR4), single-trailing-closure onClick (FR6), AV1, PR1 and rule that a blank label or alt counts as absent no current spec states yet. That decision includes whether FR5 with OQ1 is acceptable: the label shows only under a pointer and becomes a VoiceOver hint on iPhone, while spec:AST-082 FR3 says a touch platform behaves as core's Tooltip touchTrigger defines, and core's Thumbnail opens the label on a tap. An engineering owner records it by commenting /approve-spec <full head SHA> on the head they approve; the author may do so as an engineering owner, and a new commit voids the approval. Separately, please list the macOS label tooltip under "Not measured" in the description, or attach a Mac capture, as spec:AST-093 FR5 asks for claims that were not captured.
[Automated review]
| | FR2 | `isLoading` with no `src` MUST show a skeleton in `color.skeleton` at 25% opacity; after 1 second it climbs to 92.5% in ten equal held steps over core's default 550 ms medium duration and back, repeating, as core's does, and it rests at 25% under Reduce Motion; under Increase Contrast its color moves 30% toward `color.textPrimary`, as core's does under `prefers-contrast: more`. `isLoading` with a `src` MUST draw `color.overlay` over the image with the platform's small progress view in `color.onDark`, which keeps its size at any text size. | core `Thumbnail`; Swift accessibility X6 | verify | | ||
| | FR3 | With `onRemove` and not disabled, a 20-point remove button MUST sit `space.s1` from the top trailing corner on `color.overlay` with the shared `close` `Icon` at `xsm` in `color.onDark`, which the theme can remap and which keeps its size at any text size. It is visible at rest with `always` and on touch devices; with `hover` under a pointer, it appears while the pointer is over the thumbnail or while its open or remove button has keyboard focus. | core `Thumbnail` `showRemoveOn`; `spec:AST-082` FR3 | verify | | ||
| | FR4 | With `onClick`, not disabled, and not loading, the tile MUST be a SwiftUI `Button` that paints `color.overlayPressed` while pressed. `isDisabled` MUST draw the thumbnail at half opacity and turn off its tap and remove button. | core `Thumbnail` | verify | | ||
| | FR5 | With a `label`, the thumbnail MUST show it as a tooltip where there is a pointer. | core `Thumbnail` `label`; `spec:AST-082` FR3 | verify | |
There was a problem hiding this comment.
FR5's tooltip shows only on a Mac with a pointer, and nothing in the change shows it working: the captures come from an iPhone simulator, where .help becomes a VoiceOver hint, no test reaches ThumbnailTooltip, and the verification map has no FR5 row. spec:AST-093 FR5 asks for a claim that could not be captured to be reported as not measured; listing the macOS label tooltip under "Not measured" in the description, or attaching a Mac capture, covers it.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new draft component:swift/Thumbnail record, whose Swift presentation (FR1–FR4), label bubble below the tile (FR5), single-trailing-closure onClick (FR6), AV1, PR1 and rule that a blank label or alt counts as absent no current spec states yet; an engineering owner records it by commenting /approve-spec <full head SHA> on the head they approve, the author may do so as an engineering owner, and a new commit voids the approval. Independently of that decision, the label tooltip needs to follow core's Thumbnail, as spec:AST-082 FR3 requires and FR5 itself claims:
- show the label on the tap that opens a tappable thumbnail too, while
onClickstill runs; - show nothing on keyboard focus, and nothing on hover or tap while the thumbnail is disabled;
- hide it when a pointer presses the thumbnail, then update FR5, its verification row and
theLabelTooltipOpensOnTapOnlyWithoutAnOpenAction.
Please also attach a simulator capture of the corrected label popover with the other states (spec:AST-093 FR5).
[Automated review]
|
|
||
| /// Whether a tap on a touch screen opens the label's tooltip: only when the | ||
| /// tile has no open action; open and remove keep their taps, as core's do. | ||
| nonisolated static func opensTooltipOnTap(hasOpen: Bool) -> Bool { !hasOpen } |
There was a problem hiding this comment.
This keeps the label shut on the tap that opens a tappable thumbnail and opens it on a disabled one, the reverse of core. Core's tooltip wraps the group root, which is not an action trigger, so every tap on an enabled thumbnail opens it, the open tap included, while onClick still runs (useTouchTrigger.ts:326-332); a disabled root takes no pointer events at all (Thumbnail.tsx:287-290). Opening on any tap while not disabled, and skipping the hover path while disabled, would match core under spec:AST-082 FR3.
| .onHover { isHovering = $0 } | ||
| .modifier( | ||
| ThumbnailTooltip( | ||
| label: Thumbnail.meaningful(props.label), isFocused: focusedPart == .open, |
There was a problem hiding this comment.
Core never shows Thumbnail's label on keyboard focus: the tooltip's trigger is the non-focusable group root, so focusTrigger: 'auto' attaches no focus listener (useTooltip.tsx:223-241, :502-510), and a mouse press hides an open tooltip (:452-471). Dropping isFocused and hiding the bubble on a pointer press would match core.
| | FR2 | `isLoading` with no `src` MUST show a skeleton in `color.skeleton` at 25% opacity; after 1 second it climbs to 92.5% in ten equal held steps over core's default 550 ms medium duration and back, repeating, as core's does, and it rests at 25% under Reduce Motion; under Increase Contrast its color moves 30% toward `color.textPrimary`, as core's does under `prefers-contrast: more`. `isLoading` with a `src` MUST draw `color.overlay` over the image with the platform's small progress view in `color.onDark`, which keeps its size at any text size. | core `Thumbnail`; Swift accessibility X6 | verify | | ||
| | FR3 | With `onRemove` and not disabled, a 20-point remove button MUST sit `space.s1` from the top trailing corner on `color.overlay` with the shared `close` `Icon` at `xsm` in `color.onDark`, which the theme can remap and which keeps its size at any text size. It is visible at rest with `always` and on touch devices; with `hover` under a pointer, it appears while the pointer is over the thumbnail or while its open or remove button has keyboard focus. | core `Thumbnail` `showRemoveOn`; `spec:AST-082` FR3 | verify | | ||
| | FR4 | With `onClick`, not disabled, and not loading, the tile MUST be a SwiftUI `Button` that paints `color.overlayPressed` while pressed. `isDisabled` MUST draw the thumbnail at half opacity and turn off its tap and remove button. | core `Thumbnail` | verify | | ||
| | FR5 | With a `label`, the thumbnail MUST show it as a tooltip as core does: where there is a pointer, and on keyboard focus of the open button, after core's 200 ms, in a bubble below the tile in core's tooltip colors (`color.textPrimary` behind `color.backgroundSurface` body text, padded `space.s1` by `space.s2`, `radius.container`) that VoiceOver skips, since the label is in the thumbnail's name; on a touch screen, a tap on a thumbnail without an open action opens it and the next tap, on the tile or elsewhere, closes it. The tap is shared with an enclosing row's button or link, which still runs. Open and remove keep their taps, and with VoiceOver running the tap is left alone. | core `Thumbnail` `label`; `spec:AST-082` FR3; `spec:AST-017` | verify | |
There was a problem hiding this comment.
FR5 says "as core does", but core shows the label on the tap that opens a tappable thumbnail, never on keyboard focus, never while disabled, and hides it on a mouse press; the FR5 verification row (line 165) pins the opposite of the first. Restating FR5 to core's rule keeps the record within spec:AST-082 FR3, which spec:AST-093 FR1 says wins over a native spec.
| // A tap belongs to the open action when there is one; otherwise it opens | ||
| // the label's tooltip, as core's touchTrigger does. | ||
| #expect(Thumbnail.opensTooltipOnTap(hasOpen: false)) | ||
| #expect(!Thumbnail.opensTooltipOnTap(hasOpen: true)) |
There was a problem hiding this comment.
This expectation pins the departure: core opens the label on the open tap too, and on no tap while disabled. Inverting the tappable case and adding a disabled case would hold FR5 to core.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new draft component:swift/Thumbnail record, whose Swift presentation (FR1–FR4), label bubble below the tile and tap-opened popover (FR5), single-trailing-closure onClick (FR6), AV1, PR1 and rule that a blank label or alt counts as absent no current spec states yet; an engineering owner records it by commenting /approve-spec <full head SHA> on the head they approve, the author may do so as an engineering owner, and a new commit voids the approval. Thanks for bringing the label tooltip in line with core on the open tap, keyboard focus, disabled thumbnails and a mouse press. Independently of that decision, please:
- keep
ThumbnailTooltip's body to one structure, so togglingisDisabledon a labeled thumbnail no longer rebuilds the tile, reloading its image, or leaves a bubble showing; - skip the tap popover while a pointer hovers the thumbnail, so an iPad click hides the label as FR5 says instead of reopening it as a popover;
- attach a simulator capture of the label popover over a tappable thumbnail, and of the tap after it, with the other states (
spec:AST-093FR5).
[Automated review]
| nonisolated static var delay: Duration { .milliseconds(200) } | ||
|
|
||
| func body(content: Content) -> some View { | ||
| if let label { |
There was a problem hiding this comment.
Because tooltipLabel returns nil while disabled, each flip of isDisabled moves content between this branch and the bare content below, which gives it a new identity: the tile's AsyncImage starts over from its empty phase and a skeleton restarts its one-second wait. isShown also keeps its value while no handler can clear it, so a bubble showing when the thumbnail was disabled comes back on re-enable. Attaching these modifiers whatever the label, doing nothing while it is nil, and clearing isShown, isTapShown and pending when it becomes nil keeps one structure.
| if !voiceOver { | ||
| view | ||
| // Shared with the open action and an enclosing row, which still run. | ||
| .simultaneousGesture(TapGesture().onEnded { isTapShown.toggle() }) |
There was a problem hiding this comment.
On iPadOS a trackpad or mouse click is a tap to this gesture too, so a click on a hovered thumbnail hides the bubble (line 363) and then opens the same label as a popover. Core opens on a tap only for touch and pen presses (useTouchTrigger.ts:64, :360) and hides on a mouse press, and FR5 says the label hides when the pointer presses; skipping the toggle while a pointer hovers the thumbnail would match.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new draft component:swift/Thumbnail record, whose Swift presentation (FR1–FR4), tap-opened label popover (FR5), single-trailing-closure onClick (FR6), AV1, PR1 and rule that a blank label or alt counts as absent no current spec states yet, including whether the iPhone popover may keep the next tap where core's tooltip lets it through; an engineering owner records it by commenting /approve-spec <full head SHA> on the pull request, the author may do so as an engineering owner, and a new commit voids the approval. Independently of that decision, please:
- present the tap popover only for a touch, so an iPad trackpad or mouse click hides the label as FR5's table says instead of reopening it as a popover (
Thumbnail.swift:386); - keep
ThumbnailTooltipto one structure, so togglingisDisabledon a labeled thumbnail no longer rebuilds the tile, reloading its image, or brings back a stale bubble (Thumbnail.swift:348-377); - attach simulator captures of the label popover and of the tap after it, as
spec:AST-093FR5 asks for every state.
[Automated review]
ea61157 to
26bc2f1
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the two new draft records, component:swift/Thumbnail and architecture:native-swift-library-tooltip, whose Swift presentation (FR1–FR4, including the 44-point remove target over the tile's top trailing corner), single-trailing-closure onClick (FR6), AV1, PR1, blank-name rule and tooltip INV1–INV9 no current spec states yet, in particular whether a scroll view around the thumbnail may cut off the label bubble (INV1). An engineering owner records that decision by commenting /approve-spec <full head SHA> on the pull request; the author may do so as an engineering owner, and a new commit voids the approval. Independently of that decision, please:
- correct the FR5 verification row, which says a tile rebuilt by a disabled toggle fails
togglingDisabledKeepsTheSameTile, a test that compares two separate renders and cannot see a rebuild; - attach captures of the tap-opened label bubble, alone and inside a scrolling strip, or say why they cannot be taken (AST-093 FR5);
- land after #8288 so
pnpm check:knowledgecan validate the Thumbnail record's schema-5 frontmatter (AST-093 FR1).
[Automated review]
| | FR1 | `ThumbnailTests.swift` size, plate, and two-theme renders | no image; default, neutral, butter; the largest text size | a wrong size or a color of its own fails | `audit:swift/Thumbnail/tile` | | ||
| | FR2 | `ThumbnailTests.swift` loading states, skeleton color, contrast mix, and largest-text render | loading with and without `src`; Increase Contrast; the largest text size | a skeleton over an image, no upload look, a wrong skeleton color, or a tile that grows fails | `audit:swift/Thumbnail/loading` | | ||
| | FR3, FR4 | `ThumbnailTests.swift` remove, tap, and disabled tests and render | `always`, `hover`; disabled; loading | a hidden remove on touch or under keyboard focus, a hard-coded remove glyph, or a tap while disabled or loading fails | `audit:swift/Thumbnail/actions` | | ||
| | FR5 | `TooltipPresenterTests.swift` for each input; `ThumbnailTests.swift` name and disabled-toggle tests | enabled and disabled; blank label | a tooltip on a disabled thumbnail or a tile rebuilt by a disabled toggle fails | `audit:swift/Thumbnail/tooltip` | |
There was a problem hiding this comment.
togglingDisabledKeepsTheSameTile renders two separate thumbnails, one enabled and one disabled, and compares their sizes, which are 64 by 64 whatever the view structure, so "a tile rebuilt by a disabled toggle fails" is not backed. Dropping that clause (and reporting the one-structure claim as not measured, beside AR5) or toggling isDisabled on one hosted view would make the row accurate.
26bc2f1 to
62af931
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new draft component:swift/Thumbnail record, whose Swift presentation (FR1–FR4, including the fixed 550 ms skeleton pulse), single-trailing-closure onClick (FR6), AV1, PR1 and blank-name rule no current spec states yet, in particular whether FR5 accepts the shared Swift tooltip's label bubble being cut off inside a scrolling attachment strip. An engineering owner records that decision by commenting /approve-spec <full head SHA> on the pull request; the author may do so as an engineering owner, and a new commit voids the approval. Independently of that decision, please:
- land after #8288 so
pnpm check:knowledgecan validate the record's schema-5 frontmatter (AST-093 FR1); - attach captures of the tap-opened label, alone and inside a scrolling strip, or say why they cannot be taken (AST-093 FR5).
[Automated review]
| | FR2 | `isLoading` with no `src` MUST show a skeleton in `color.skeleton` at 25% opacity; after 1 second it climbs to 92.5% in ten equal held steps over core's default 550 ms medium duration and back, repeating, as core's does, and it rests at 25% under Reduce Motion; under Increase Contrast its color moves 30% toward `color.textPrimary`, as core's does under `prefers-contrast: more`. `isLoading` with a `src` MUST draw `color.overlay` over the image with the platform's small progress view in `color.onDark`, which keeps its size at any text size. | core `Thumbnail`; Swift accessibility X6 | verify | | ||
| | FR3 | With `onRemove` and not disabled, a 20-point remove button MUST sit `space.s1` from the top trailing corner on `color.overlay` with the shared `close` `Icon` at `xsm` in `color.onDark`, which the theme can remap and which keeps its size at any text size. It is visible at rest with `always` and on touch devices; with `hover` under a pointer, it appears while the pointer is over the thumbnail or while its open or remove button has keyboard focus. | core `Thumbnail` `showRemoveOn`; `spec:AST-082` FR3 | verify | | ||
| | FR4 | With `onClick`, not disabled, and not loading, the tile MUST be a SwiftUI `Button` that paints `color.overlayPressed` while pressed. `isDisabled` MUST draw the thumbnail at half opacity and turn off its tap and remove button. | core `Thumbnail` | verify | | ||
| | FR5 | With a `label`, the thumbnail MUST show it through the Astryx tooltip (`architecture:native-swift-library-tooltip`), as core's tooltip wraps the whole thumbnail: not a tab stop, and a direct tap opens it while an open action still runs, as the Tooltip by input table below states. A disabled thumbnail shows none, and the label is not a hint, since it is in the thumbnail's name. | core `Thumbnail` `label`; core `Tooltip`; `spec:AST-082` FR3 | verify | |
There was a problem hiding this comment.
FR5 adopts the shared Swift tooltip, whose INV1 draws the bubble in the thumbnail's own layer, so inside a horizontal ScrollView such as a composer's attachment strip the label shows only as a sliver at the strip's top edge, while core's top-layer tooltip is never clipped. Approving FR5 accepts that for Thumbnail; if it should not, FR5 needs a placement rule that draws the label above clipping containers before approval.
62af931 to
073373b
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new draft component:swift/Thumbnail record, whose Swift presentation (FR1–FR4, including the fixed 550 ms skeleton pulse), single-trailing-closure onClick (FR6), AV1, PR1 and blank-name rule no current spec states yet, in particular whether FR5 accepts the shared Swift tooltip's label bubble being cut off inside a scrolling attachment strip. An engineering owner records that decision by commenting /approve-spec <full head SHA> on the pull request; the author may do so as an engineering owner, and a new commit voids the approval. Independently of that decision, please:
- set
schema_version: 3andtemplate_version: 7inThumbnail.spec.md, main's component template, as the Swift Avatar and Icon records now use; - add Thumbnail to the wire nodes
AstryxNodeViewdraws inpackages/native/specs/swift/library/component-api.md.
[Automated review]
| schema_version: 5 | ||
| template_version: 8 |
There was a problem hiding this comment.
main has knowledge schemas v1–v4 and component template 7 (docs/templates/knowledge/versions.json), and the sibling Swift Avatar and Icon records now use schema_version: 3 and template_version: 7. The same two values here make this record follow main's component template, as the knowledge-contracts architecture (INV27) and AST-093 FR1 ask.
| case .card(let card): Card(card.viewProps(send: send), children: card.props.children ?? [], send: send) | ||
| case .icon(let icon): Icon(icon.viewProps(send: send)) | ||
| case .avatar(let avatar): Avatar(avatar.viewProps(send: send)) | ||
| case .thumbnail(let thumbnail): Thumbnail(thumbnail.viewProps(send: send)) |
There was a problem hiding this comment.
With this arm, packages/native/specs/swift/library/component-api.md ("AstryxNodeView draws Avatar, Card, Heading, Icon, and Text wire nodes. Any other node … renders in its place as unsupported") goes out of date; adding Thumbnail to that list keeps the record true, as the Avatar change did.
073373b to
5f9b489
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the new draft component:swift/Thumbnail record, whose Swift-only requirements no current spec states yet, in particular FR4's open and remove buttons that now stay mounted but inert while uploading or disabled (core's Thumbnail removes them) and FR5's label bubble, which a scrolling attachment strip cuts off. An engineering owner records that decision by commenting /approve-spec <full head SHA> on the pull request; the author may do so as an engineering owner, and a new commit voids the approval. Independently of that decision, please:
- set
schema_version: 3andtemplate_version: 7inThumbnail.spec.md, as the Swift Avatar and Icon records use; - reword FR4 so the remove button stays active during an upload, as the view and core behave;
- retake the disabled-state captures at this head, since the attached ones still show no remove button, or report that state as not measured;
- add Thumbnail to the wire nodes
AstryxNodeViewdraws inpackages/native/specs/swift/library/component-api.md.
[Automated review]
| | FR1 | The tile MUST be 64 by 64 points, clipped to `radius.element`, on `color.neutral`. It MUST show the image at `src` cropped to fill the tile, with a 1-point `color.border` inset edge on all four sides of the tile; with no `src`, or when the image fails, a 24-point image silhouette in `color.iconSecondary`. | core `Thumbnail` styles | verify | | ||
| | FR2 | `isLoading` with no `src` MUST show a skeleton in `color.skeleton` at 25% opacity; after 1 second it climbs to 92.5% in ten equal held steps over core's default 550 ms medium duration and back, repeating, as core's does, and it rests at 25% under Reduce Motion; under Increase Contrast its color moves 30% toward `color.textPrimary`, as core's does under `prefers-contrast: more`. `isLoading` with a `src` MUST draw `color.overlay` over the image with the platform's small progress view in `color.onDark`, which keeps its size at any text size. | core `Thumbnail`; Swift accessibility X6 | verify | | ||
| | FR3 | With `onRemove`, a 20-point remove button MUST sit `space.s1` from the top trailing corner on `color.overlay` with the shared `close` `Icon` at `xsm` in `color.onDark`, which the theme can remap and which keeps its size at any text size. It is visible at rest with `always` and on touch devices; with `hover` under a pointer, it appears while the pointer is over the thumbnail or while its open or remove button has keyboard focus. | core `Thumbnail` `showRemoveOn`; `spec:AST-082` FR3 | verify | | ||
| | FR4 | With `onClick`, not disabled, and not loading, the tile MUST be a SwiftUI `Button` that paints `color.overlayPressed` while pressed. `isDisabled` MUST draw the thumbnail at half opacity and turn off its tap and remove button. While loading or disabled, the open and remove buttons MUST stay mounted but inert, so focus on them stays put and VoiceOver reads them dimmed (`spec:AST-085`; Swift accessibility X2). | core `Thumbnail` | verify | |
There was a problem hiding this comment.
FR4 reads as making the remove button inert while loading, but the view keeps it working during an upload (removes is hasRemove && !isDisabled, Thumbnail.swift:232), as core does (Thumbnail.tsx:382), and removing an attachment is how a person cancels its upload. Suggested wording: “While loading, the open button, and while disabled, both buttons, stay mounted but inert.”
5f9b489 to
fab4ede
Compare
|
Posted by Cindy's agent on her behalf. Decision (Cindy, 2026-10-26): approved. The Swift Thumbnail record on this head is accepted as written. Open and Remove stay mounted but inert while unavailable, per AST-085. The tooltip can be clipped inside a scrolling strip, per tooltip INV1. What this PR needs: nothing more. It merges at this head once CI and CodeBunny are clean. |
|
/approve-spec fab4ede |
There was a problem hiding this comment.
Changes are requested: the Swift Thumbnail record is approved by an engineering owner on this head, but two behaviors contradict core's Thumbnail record, which became current after this branch was cut. Please make keyboard focus on the open or remove button show the label tooltip, as core's record FR3 requires (FR5's table and focus: .none turn it off today), and keep a pointer from revealing the inert remove button on a disabled thumbnail on a Mac, where core's record FR6 turns pointer events off. Both fixes need a new head, so an engineering owner records the decision again by commenting /approve-spec <full head SHA> there; the author may do so as an engineering owner.
- Keyboard reveal:
Thumbnail.spec.md:97andThumbnail.swift:78. - Hover while disabled:
Thumbnail.swift:73and:237-247.
[Automated review]
| // itself in core's tooltip: not a tab stop, and core's `auto` touch opens | ||
| // it on a tap, since the wrapper is not a control, while an open action | ||
| // still runs. The label is already in the name, so no hint. | ||
| .astryxTooltip(Thumbnail.meaningful(props.label), focus: .none, isDisabled: isDisabled) |
There was a problem hiding this comment.
focus: .none keeps keyboard focus from ever showing the label: the shared modifier counts focus only when focus != .none (TooltipPresenter.swift:143). Core's Thumbnail record, now current on main, states the rule in FR3, "A tooltip can be revealed from the keyboard (WCAG 2.2 2.1.1)", and lists core's non-focusable root as an implementation gap, the same form FR4 uses for the unmounted Open this view already corrects. Showing the label while focusedPart is set, and keeping the tap with touch: .tap (a .control focus resolves auto touch to none), would match it.
| | Input | With a `label` | Disabled, or no `label` | | ||
| | -------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------- | | ||
| | Pointer | the bubble shows the label above the tile after 200 ms, stays while the pointer is on it, and a press hides it | nothing | | ||
| | Keyboard focus | nothing, as core's wrapper is not a tab stop | nothing | |
There was a problem hiding this comment.
This row makes core's recorded gap the Swift rule. Core's Thumbnail record (FR3) asks for a tooltip the keyboard can reveal, so the row should say keyboard focus on Open or Remove shows the label, alongside the view change at Thumbnail.swift:78.
| /// shows it at rest. | ||
| var isRemoveVisible: Bool { | ||
| Thumbnail.isRemoveVisible( | ||
| showRemoveOn: props.showRemoveOn ?? .hover, isTouch: Thumbnail.isTouchDevice, isHovering: isHovering, |
There was a problem hiding this comment.
isRemoveVisible never reads isDisabled, and .onHover at line 73 always runs, so on a Mac a pointer over a disabled thumbnail reveals the inert ×. Core's record FR6 turns pointer events off while disabled. Passing isHovering: isHovering && !isDisabled, with a disabled case in hoverRevealsRemoveUnderAPointerOrKeyboardFocusAndAlwaysOnTouch, would match it; touch keeps its dimmed button.
fab4ede to
20c20b4
Compare
|
/approve-spec 20c20b4 |
…and remove actions
20c20b4 to
f7468f0
Compare
|
/approve-spec f7468f0 |
There was a problem hiding this comment.
The Swift Thumbnail looks ready: an engineering owner approved the new Swift record and the tooltip's inside focus kind on this head, keyboard focus on Open or Remove now shows the label, and a pointer no longer reveals the remove button on a disabled thumbnail, as core's Thumbnail record requires. Nothing more is needed from you beyond the pending required checks. When the record is next edited, AR1 could say that a tappable thumbnail's open button hides the picture, as the view and core's open button do.
[Automated review]
User need
Chat composers, upload forms, and galleries on iOS show image attachments as small previews that a person can open or remove. There is no Swift
Thumbnail, so an app draws its own: its own size, no placeholder when an image fails, no clear upload state, a remove button too small to hit, and VoiceOver hearing "image" instead of the file name.Problem and solution fit
Thumbnailview. Core's 64 pt preview on the theme's neutral plate: the image with an inset edge, a silhouette when there is no image or it fails, a skeleton while loading with no image, and an upload overlay with the platform's progress view over one.onClickmakes the tile a SwiftUIButton;onRemoveadds a 20 pt remove button with the theme'scloseicon in the top trailing corner, with a 44 pt target.showRemoveOn: .hoverkeeps it visible on touch devices and reveals it under a pointer on an enabled thumbnail or while either button has keyboard focus, as core defines;.alwaysshows it at rest. Disabled turns both off and a pointer reveals nothing on it; loading turns off the tap.Current specification
spec:AST-082FR3 (core's touch behavior forshowRemoveOn), FR7.spec:AST-086FR8, FR11 (a busy start is announced once).spec:AST-093FR1–FR5.component:swift/Thumbnailatpackages/native/apple/Sources/Astryx/Thumbnail/Thumbnail.spec.md(draft,platform_of: component:Thumbnail).Public delta and caller burden
Thumbnail(Astryx) and generatedThumbnailProps,ThumbnailShowRemoveOn(AstryxNative).onClick;onRemoveis always labeled.@astryx.spinner.loading,@astryx.thumbnail.fallbackName,@astryx.thumbnail.open,@astryx.thumbnail.remove.Design and alternatives
AsyncImage,ProgressView, andButton; SwiftUI has no thumbnail control, so the tile and overlays are drawn here (AST-093 FR2).Evidence
Capture set: iPhone 16 simulator, iOS 18.5, Xcode 16.4, built from this PR's head. Each capture shows an image, no image, a failed image, the skeleton, the upload overlay, the remove button (hover on touch and always), a tappable thumbnail, disabled thumbnails, and two themes.
Not measured: the macOS hover and keyboard-focus reveal of the remove button, and the label tooltip on hover and tap, alone or inside a scrolling strip. The capture harness drives the Simulator with
simctl, which can take screenshots but cannot hover or tap; showing them needs an app-hosted UI test, which the package does not have yet. That the tile keeps one structure across a disabled toggle or an upload ending is also not measured, since a test cannot see a rebuild.The record uses main's component template (schema 3 / template 7), as the Swift Avatar and Icon records do;
check:knowledgepasses.Implementation choices under current rules
spec:AST-082FR3). Thumbnail shows its label through the shared Astryx tooltip from feat(native): one internal Swift tooltip that follows core's useTooltip on pointer, keyboard, touch, and VoiceOver #8402, as core's Thumbnail wraps itself in core's tooltip. It is not a tab stop itself; keyboard focus on its open or remove button shows the label at once, as focus within core's tooltip wrapper does (the shared tooltip's newinsidefocus kind), and a direct tap opens the label while an open action still runs. A disabled thumbnail shows none. The tooltip keeps one view structure, and the open button sits over the picture instead of wrapping it, so neither a disabled toggle nor an upload ending reloads the photo. FR5's table states Thumbnail's choices.Open-work search
Searched current records and open pull requests before creating
component:swift/Thumbnail, by canonical id (component:swift/Thumbnail,component:Thumbnail), paths (packages/native/apple/Sources/Astryx/Thumbnail/,packages/native/src/components/Thumbnail.ts), symbols (Thumbnail,ThumbnailProps,SWIFT_IOS_STRING_KEYS,@astryx.thumbnail.*), and terms (native,swift,thumbnail). No other record or open pull request owns this platform component; the Swift views moved onto core names in #8299 and the Icon slot in #8302 are its neighbors.Scope
Writing rubric results
Testing
vitest run packages/native: the Thumbnail contract and the regenerated string catalogs pass their drift checks.swiftc -typecheckofAstryxNative,Astryx, and the tests passes. An immediate-mode render checks the 64 pt tile on the default and butter plates, the dimmed disabled tile, the names, a trailing closure asonClick, and that a hidden thumbnail draws nothing.Every changed file is under
packages/native/.A Thumbnail wire node draws as Thumbnail in
AstryxNodeView(AST-082 FR19).The open button while loading or disabled, and the remove button while disabled, stay mounted but inert (
.disabled), so focus on them stays put and VoiceOver reads them dimmed. This is core's rule (spec:AST-085FR1, FR4, FR6, as the core Thumbnail record states); core's current unmounting is a known core gap, not a Swift departure. On touch a disabled thumbnail shows its dimmed remove button. During an upload the remove button stays active, as core's does. The one accepted difference from core is the tooltip bubble, which a scrolling strip can clip (architecture:native-swift-library-tooltipINV1).