[6.x] Modernize image editor - #19600
Draft
brianjhanson wants to merge 18 commits into
Draft
Conversation
It sat at z-index 1000, above the slideout panels (100), the modal shade (100) and the image editor modal, so it painted over anything opened on top of the page. Nothing in the editor's own content sits between 10 and 99, so lowering it keeps it above the page it scrolls over without competing with overlays. The matching `.element-editor__header` rule is dead — the class is never applied — but carried the same value, so it moves too rather than waiting to reintroduce this if it's ever revived. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
`.surface` set `inline-size` on the inline axis but only `max-block-size` on the block axis, so a fullscreen dialog came out full-width and as tall as its content. That also starved anything slotted into it: the body row is `1fr` of an auto-height grid, so `block-size: 100%` on a child resolved against nothing. Adds a `--c-dialog-block-size` hook mirroring the inline-axis ones, defaulting to `auto` and to `100dvh` under `fullscreen`. Non-fullscreen dialogs are unchanged. The Fullscreen story existed but asserted nothing, which is why this shipped; it now checks the surface reports viewport dimensions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
Setting `name` puts the group in single-select mode, where it rewrites `active` on its children from its own `value`. With `value` unset that cleared every child, so a group handed its selection in markup lost it, and a consumer setting `active` had it stripped on the next sync with nothing to say why. It now seeds `value` from whichever child is marked `active`, the way craft-radio-group adopts its `name` from slotted inputs. An explicit `value` still wins, adoption happens once, and multi-select is untouched — it already reads `active` off its children. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
Rewrites Craft.AssetImageEditor — 4,400 lines of jQuery-and-Garnish —
as a Vue module, keeping the same behaviour and the fabric.js 1.7 API
for now. Split by responsibility rather than as one class:
geometry.ts pure maths — containment, vertices, hit-testing,
resize algebra, transposition
constraints.ts aspect-ratio options and their orientation
fabric.ts the only file that touches fabric, so the v7
upgrade is a change here and nowhere else
useEditorState reactive state and the geometry derived from it
useImageCanvas canvas lifecycle, sizing, zoom, viewport mask
useCropper cropping layer, handles, move and resize
useFocalPoint the focal marker
useImageTransforms rotate, flip, straighten
useEditorInteractions pointer and keyboard editing
useImageEditor load, resize, view transitions, save
jQuery is gone: mouse/touch pairs become pointer events with capture,
`$el.offset()` becomes `getBoundingClientRect()`, and Craft.Queue
becomes a promise chain. Screen-reader announcements go through `t()`;
they were bare template literals and stayed English in every locale.
Presentation is the component's: the cursor is a bound ref rather than
`$('.body').css()`, and the custom-constraint inputs are markup rather
than built from JS and read back with `.val()`.
Covered by tests over the geometry pipeline — the containment invariant
every drag depends on, handle clearance, and the zero-size guards.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
Renders the ported editor as a fullscreen craft-dialog on the asset edit page, so it gets a real <dialog>'s focus containment and Escape handling rather than a hand-rolled modal. AssetEditViewModel::imageEditor() supplies what the dialog needs, or null when the asset isn't an image the user may edit — the same value decides whether the dialog renders at all, keeping the permission and format checks server-side. The Edit Image button drops its jQuery blob for a `data-image-editor` attribute; the screen delegates on it from the preview fragment's `ready` event. Saving in place reloads so the thumbnail refreshes; saving a copy leaves this asset alone and needs nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
A toolbar control that turns something on and stays that way is a toggle button — `aria-pressed` plus a visibly pressed state — not a switch, which is what craft-switch is for. `active` already carried the visible half, but nothing tied `aria-pressed` to it: two things to set, and setting one and forgetting the other is the easy mistake. With `toggle`, `aria-pressed` is derived from `active`, so what a screen reader hears and what people see can't drift apart. It never flips `active` itself. Selection may be owned elsewhere — craft-button-group writes `active` on its children, and a consumer may refuse the change — so it reports intent through a cancelable `craft-toggle` and leaves the decision to whoever owns the state. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
`requestClose()` is the single funnel for the close button, Escape and the backdrop, so asking there covers every way out at once. It now fires a cancelable `craft-before-hide`, for a dialog holding work that would be lost. Deliberately silent when `opened` is set to false directly: that's a decision already taken, so saving and closing doesn't prompt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
The graduations were `display: inline-block`, so the newlines between them in the template each rendered as a space — about 3.8px per graduation. That made the strip 1942px where the positioning maths assumes 10px apiece, or 1400, and put zero some 265px from the cursor: around 20 degrees out. Flex lays them flush, and `flex: none` keeps them 10px whatever the width of the window they slide behind. Two related fixes while in here. The graduations element is the window the strip slides behind and the maths centres on its width, so it clips itself now rather than relying on a wrapper around the component — the legacy markup had one, its rule scoped to the modal. And repositioning bails when that window has no layout at all, rather than centring against zero and burning in an offset the resize observer never revisits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
The inset that gives the cropper's handles room to be drawn lived inside `getZoomToFitRatio`, which only runs while cropping. Every other view laid the image out against the full editor, so opening the crop controls reframed the image by the margin doubled — 40px on a 1168x574 editor — on top of the zoom change. That reads as a lurch. There's one content box now, used by every view, so moving between them is a zoom and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
Both were moved by however much the editor had changed, which compounds: every resize left them a little further off the picture. Both are stored relative to the image at a zoom of 1, so reading that back puts them on the same part of it whatever the editor did. The focal point's restore is split in two. It ended by adding the marker to the canvas, which fabric does unconditionally — right for a transition that lifted it off, but it would stack up duplicates on every resize. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
`reset()` winds every edit back without refetching: the source is already on the canvas and everything done to it lives in state. `isDirty` is derived from that state rather than a flag each mutating call has to remember to set — one missed call site and someone loses work to a confirmation that never appeared. Two ordering fixes alongside. Switching views laid the image out and started its transition before the view flipped, so both ran against a width the crop sidebar was about to change; the view flips first now, and one measurement is taken once it lands. And the cropper and focal point are repositioned after the image has taken its new size, not before — they read where it is, so reading it early put them where it used to be. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
Crop is a toggle that shows and hides the sidebar, reading its state off the editor rather than keeping a second copy. Focal Point shows as active while the asset has one — existence, not the transient picked-up state, which also fixes the button removing a focal point it thought was absent. Reset to original sits at the far end of the toolbar, disabled until there's something to discard, and asks first. Closing with unsaved edits asks too, through the dialog's new `craft-before-hide`. The dialog is always dark: judging an image against light chrome skews how it reads. Its body loses its padding so the editor meets the edges, and the sidebar states its width — it holds block-level fields, which fill their parent rather than asking for a width, so flex had nothing to size it from and it collapsed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
The generated palette already had a dark set, keyed on `[data-theme='dark']`
as a plain attribute selector — but putting that on anything other than the
root did nothing, so the image editor dialog stayed light.
A custom property carrying a `var()` is substituted where it's declared, not
where it's used. `:root { --c-surface-raised: var(--color-base-50) }` resolves
to white at the root, and that resolved value is what inherits; redeclaring
the palette further down can never reach back into it. Measured in Chromium:
the raw palette followed the subtree, everything built out of it did not.
So the two semantic layers repeat themselves on `[data-theme]` — one in
`tokens.css`, one emitted by `generate-colors.js` — which re-resolves them
against that element's palette.
`color` needs the same treatment for the same reason, and its absence is how
this surfaced: dark text on a themed dark background, a contrast ratio of
1.07, caught by the accessibility check on a new story. Backgrounds are left
alone — they're painted deliberately by whatever owns the surface.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
So a link can land straight in the editor rather than on the edit screen with a button to press. Read server-side like the rest of this screen's state, and guarded on there being an image editor at all, so the parameter can't ask for one this asset doesn't get. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
📚 Storybook previews@craftcms/ui — open Storybook Changed components: resources/js — open Storybook No changed components detected in this Storybook. |
Opening the image editor puts `?editing=true` in the URL and closing it takes it back out, so the address bar says what's on screen and a refresh lands where you left off. Driven by a watcher on the open state rather than the handlers, so it covers every way out — Escape, the close button, a save — not only the paths that happen to run through a function here. `history.replaceState` rather than an Inertia visit: this is UI state, and a round-trip to re-render the page behind an open dialog just to toggle a query parameter is a lot of work for nothing. Inertia's own history entry is handed straight back so only the URL changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
It was pinned to landscape, so a portrait photo opened with the wrong button selected and every ratio the wrong way round until you noticed. The asset's dimensions come through with the rest of the editor's props, so the orientation is right before anything renders rather than flipping once the image loads. Reset returns to the same, not to landscape. Square images and ones with no dimensions on record read as landscape, which is the button that ends up selected for them anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
Saving already dismissed the dialog, but the return trip left the screen
in the wrong place.
Saving a copy now redirects to the new asset. The URL comes from the
server, so the `{id}{slug}` path doesn't have to be rebuilt in JS.
Saving in place refetches for the new thumbnail, and that refetch has to
drop `editing` from the URL itself: the watcher that mirrors the editor
doesn't flush until after the handler returns, and a reload refetches
whatever the URL says at the time — so it came back with the editor
asked to open again, and reinstated the parameter in history on the way.
The dialog also closes before it emits, so a consumer that navigates on
the way out isn't doing it while the dialog still reads as open.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
The editor opened itself on load with nothing having asked it to. `craft-dialog` takes its state as the `opened` property, backed by the `open` attribute, so `:open` went down Vue's attribute path. `patchAttr` only strips a false boolean for the seven names in `isSpecialBooleanAttr` — `open` is in the wider `isBooleanAttr` list, which it never consults — so `false` was written out as the string "false", and Lit's boolean converter reads attribute presence. Bind `.opened` instead, which also survives the element not being upgraded yet. The test pins the attribute form too, so switching it back fails loudly rather than quietly showing the dialog. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HY6JVxWjJjmZUMuSiBUW9p
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings the image editor into the new world. I shifted a few things around in the process which hopefully doesn't ruffle too many feathers.
Full description will come when this is out of draft state.