IBX-12302: Provided possibility of inline change for simple fields - #2024
Open
kmadejski wants to merge 22 commits into
Open
IBX-12302: Provided possibility of inline change for simple fields#2024kmadejski wants to merge 22 commits into
kmadejski wants to merge 22 commits into
Conversation
Harden REST error parsing in request.helper.js so failures surface the
server's actual message instead of a JS syntax error.
- getErrorMessageObject is now content-type aware: it only calls
response.json() when the content-type header contains "json". For any
other content type (notably the XML <ErrorMessage> body returned when
Accept is unset or resolves to the XML visitor) it reads response.text(),
extracts the <errorMessage> element's text if present, otherwise falls
back to the raw trimmed text, otherwise response.statusText. The result
is always shaped as { errorMessage } so handleRequest's existing
defaultGetErrorMessage and the translated error.request.default_msg
fallback keep working unchanged.
- getRequestHeaders gains an `accept` option that is merged into the
returned headers as an Accept header when present. It is a no-op when
absent, so every existing call site (all ~11 ibexa.helpers.request.*
call sites, and the sole getRequestHeaders caller in
helpers/location.helper.js, which already sets Accept via
extraHeaders) is byte-identical in behaviour.
The default Accept header is intentionally left unset globally: changing
it would alter which server-side REST visitor (XML vs JSON) serialises
the response for every existing caller in the product. Out of scope for
this feature branch; worth its own ticket.
src/bundle/ui-dev/src/modules/common/helpers/request.helper.js is left
untouched: it never parses an error response body (handleRequestError
throws response.statusText directly), so it does not have this defect.
Add the PHP groundwork for inline (quick) editing of simple Field
values from the Content View:
- New siteaccess-aware config parser for a top-level `inline_field_edit`
node (enabled, supported_field_types, excluded_field_types), kept
separate from the core-owned `content_view` node whose prototyped
view-type schema is incompatible. Defaults to disabled.
- `ContentTab::getTemplateParameters()` now exposes `can_edit` (content-
specific edit permission via `canUser('content', 'edit', $content)`),
`inline_field_edit` (effective enabled flag and field type list, with
the exclusion subtraction computed in PHP), and `current_language_code`
(resolved from the request query parameter used by the existing
language switcher, falling back to the content's default language).
- Unit tests for the parser (default shape and overrides) and for the
new tab parameters.
Fix current_language_code resolution: the language switcher's route places 'languageCode' in request attributes, not the query string, so read it via Request::get() (attributes, then query) instead of query-only. Rewrite the covering test to populate route attributes the way the real route does, so it actually exercises this path.
Avoid the deprecated Request::get() shortcut; read attributes then query explicitly to resolve current_language_code, keeping the same lookup order the language switcher's Twig helper relies on.
Stamp the DOM contract data attributes that the upcoming quick-edit
JavaScript reads, additive only so a project override of
content_view_fields.html.twig keeps working.
On the fields wrapper: data-content-id and data-language-code, using
the current_language_code parameter from the tab (never app.request).
On each .ibexa-content-field: data-quick-edit, data-field-definition-
identifier, data-field-type-identifier and data-field-validators (the
field's validator configuration, JSON-encoded, falling back to "{}"
when empty) are added only when the field is quick-editable, i.e.
inline_field_edit.enabled and can_edit and the field type is in the
effective inline_field_edit.field_types list and the field definition
is translatable or the currently displayed language is the content's
main language (content.contentInfo.mainLanguageCode). Quick-editable
rows also get tabindex="0", role="button" and a translated aria-label
naming the field, so non-editable rows never become focusable.
Add the missing ibexa_locationview XLIFF entry for the quick-edit aria-label key introduced in the previous commit. New |trans/|desc keys are hand-committed into this catalogue in the same PR (see ff32938), not picked up by any extraction script this repo runs. id is sha1(resname), source/target match the |desc(...) default verbatim, entry inserted alphabetically by resname between content.edit.select_language and content.hidden.message.
Add the quick edit field value editor registry consumed by the
inline "quick edit" framework (a later task). Registers
`ibexa.addConfig('quickEdit.editors', {...}, true)` in the
ibexa-admin-ui-location-view-js entry with one entry per Tier A
field type: ibexa_string, ibexa_email, ibexa_text, ibexa_integer,
ibexa_float, ibexa_boolean, ibexa_date, ibexa_datetime, ibexa_time.
Each editor is a pure { render, harvest, validate? } triple: render
builds a detached native input/textarea from a REST field value hash
plus the field's validator configuration (deriving minlength/
maxlength/min/max only when a bound is an actual number), harvest
reads the element back into the hash shape, and validate (where a
field type has a configurable Ibexa validator) reuses the existing
translated ibexa.errors strings.
Preserves the value contract exactly: ibexa_boolean never harvests
null (Checkbox\Type::fromHash has no null guard); ibexa_date renders
and harvests in UTC in both directions for a byte-stable round trip;
ibexa_datetime is browser-local in both directions; ibexa_time keeps
0 as a legitimate value rather than treating it as empty.
Editors never fetch, guard concurrency, or touch a draft - that is
the responsibility of the framework that will call them.
Substitute the real field name into the reused ibexa.errors messages
instead of stripping the {fieldName} token. The registry contract now
passes fieldConfig.fieldName (from the row's .ibexa-content-field__name
text), so render() stashes it on the element via dataset.fieldName and
formatErrorMessage() substitutes it, matching the existing
.replace('{fieldName}', label) convention in fieldType/ibexa_integer.js.
Falls back to the previous prefix-drop only when the name is missing.
Add the quick-edit framework that drives the per-field-type editors: the interaction, the concurrency guarantees and the draft transaction. New script admin.location.quick.field.edit.js, registered in the ibexa-admin-ui-location-view-js encore entry. It no-ops silently when no [data-quick-edit] element is present, so a project overriding content_view_fields.html.twig keeps working with no console output and no listeners bound. One busy guard, held for the whole save transaction including the draft conflict modal, refuses every entry point while it is taken - double click, Enter on the focused row, confirm, and all three cancel paths - so two overlapping COPY -> PATCH -> PUBLISH chains are impossible and a later entry point inherits the guard by calling the same functions. A generation token covers the async prefill window: overlapping opens are allowed, but only the newest generation may render, so a superseded open appends no DOM and leaves no state, and the confirm button can only harvest the field the user typed into. The draft href comes from the 201 Location header and the 201 body is never parsed, so a structurally unexpected body cannot throw after the draft exists. Any failure after COPY deletes the draft before the error reaches the user. initialLanguageCode is always sent explicitly on the PATCH, and PUBLISH is reached with X-HTTP-Method-Override on a POST. Accept is set explicitly on every REST call, including the ones whose success is 204, through the accept option of getRequestHeaders - with no Accept header the REST layer resolves to the XML visitor and error bodies surface as parse failures instead of server messages. The 307 on GET currentversion is followed by fetch itself, which for a 307 replays method and headers, so one round trip is enough.
Address quick-edit framework review findings. The post-prefill gate now re-checks the busy guard, not only the generation token. A prefill resolving while a save was in flight would otherwise render, run the unguarded part of closeSession() over the row being saved and overwrite the active session, leaving the transaction's finally block acting on a detached session and the user's typed value gone with nowhere to retry. closeSession() bumps the generation token, before its own early return, so every cancel path invalidates a prefill still in flight instead of letting it render, append and steal focus after the user cancelled. The field row gives up its role="button" and tabindex for as long as an editor lives inside it, and gets them back on close before focus returns. ARIA makes the children of a role="button" element presentational, so assistive tech could otherwise flatten the input and the action buttons into a label. Space activates the row alongside Enter, as the WAI-ARIA button pattern requires, with the default suppressed so the page does not scroll. A double click bubbling out of the open editor keeps its native behaviour, so double-click-to-select-a-word works inside the input. The prefill no longer falls back to another translation's field value, which the PATCH would have copied into the language being displayed. A missing value in the displayed language is now a clear error. The fields wrapper is matched as [data-content-id] rather than by .ibexa-content-preview, which also serves as a body_class, and a wrapper missing part of the data contract now bails out instead of putting undefined in the REST URL.
Give a pending quick-edit open a real abort affordance, and stop a throwing editor from failing silently. A prefill that has not rendered yet has no editor and no session, so no cancel path could reach it: the cancel button and the Escape handler live on an editor row that does not exist, and the document mousedown handler returned early on a null session. Activating a row and then pressing Escape before the editor appeared therefore still rendered it and stole focus. The pending row is now tracked, a document-level Escape and a mousedown outside that row both invalidate it, and the bump inside closeSession() moves after its early return, where it describes what it actually does. openEditor() is invoked un-awaited from both entry points, so a throw from a third-party editor's render() became an unhandled promise rejection: no editor, no message, only a console entry. The entry points now go through a wrapper that surfaces the error as a notification and clears any pending open state, so a broken editor from the quickEdit.editors extension point degrades visibly.
Add the styling for the inline quick-edit feature, the last remaining
piece after the PHP config, Twig markup, editors and framework module:
- _inputs.scss: add the `--time` and `--datetime-local` modifiers,
reusing the same rule set as `--date`, so date/time/datetime-local
render as a consistent trio. `--email`/`--number` are intentionally
left out: they are Design System input types, already fully covered
by the base `.ids-input` styles (border/background/color/hover/focus
come from `input-base` in the DS's `mixins/_inputs.scss`, with no
per-type modifier needed), so adding legacy rules for them would be
a duplicate/conflicting ruleset rather than a fix.
- _field-group.scss: add a hover/focus-visible affordance on
`.ibexa-content-field[data-quick-edit]`, tinting the background via
the repo's `var(--ibexa-…, #{$ibexa-color-…})` custom-property
pattern (matching `_inputs.scss` and the dropdown item hover rule)
rather than a bare rgba() literal, so future theming has a hook.
Background-only, so hovering never shifts layout or covers the text.
- _quick-edit.scss (new): the editor row layout for
`.ibexa-quick-edit` / `__input` / `__actions`, the block names the
framework module already renders. The input grows to fill the row
via flexbox, the actions stay a fixed-size cluster beside it.
Registered via an explicit `@use 'quick-edit';` in ibexa.scss,
alongside `field-group`, not in the location-view Encore entry
(which only carries leaflet.css and isn't part of the SCSS chain).
Add Behat coverage for the inline "quick edit" affordance on the Content view Fields tab: a new QuickEdit.feature exercising the happy path per field-type family (text, numeric, boolean, date), all three cancel paths (button, Escape, outside click), the quick-edit draft-conflict modal (confirm and dismiss), a failed save surfacing the server's own message with no draft left behind, two rapid opens on different fields collapsing to one editor, an open refused while a save is in flight, and the no affordance cases (missing edit permission, feature flag off). New page objects/components (QuickEditField, QuickEditDraftConflictModal, VersionsList) and a QuickEditContext wire these scenarios to real DOM contracts and REST behaviour read from the implementation; they are registered in the admin-ui and admin-ui-full suites in behat_suites.yml. inline_field_edit.enabled defaults to false and this suite has no mechanism to flip a siteaccess config flag per scenario or edition, so - following the precedent already set by UserProfile.feature for another default-false flag - every scenario that needs it on is left without an edition tag and will not run under the current CI jobs until a project ships that override. Only the flag-off scenario, which needs no such override, keeps the standard edition tags. This suite cannot be executed from this repository (no provisioned Ibexa installation, database, web server or Selenium are available here), so these scenarios are unverified.
Address review feedback on the quick-edit Behat coverage: - Replace the Escape-key step's driver-dependent keyDown(xpath, 27), which is provably broken on behat/mink-selenium2-driver (its syn.js only maps the word 'escape', not the raw ESC byte) while only happening to work on dmore/chrome-mink-driver, with a driver-agnostic Session::executeScript() dispatching a real KeyboardEvent, the same escape hatch already used by DateAndTimePopup for a different driver limitation. - Make the inline_field_edit.enabled prerequisite impossible to miss: an explicit comment block at the top of QuickEdit.feature, and a new @requires-config:inline_field_edit.enabled tag (following this suite's only existing colon-qualified tag convention) on every scenario that needs it, instead of leaving them silently untagged. - Rename/retag the ibexa_date scenario to state plainly that it is a plain round trip under CI's actual (UTC) timezone and proves nothing about a non-UTC browser clock; record in the report that the UTC-in-both- directions date contract has no executable coverage anywhere and rests on code review alone. - Rebuild VersionsList on this repo's Table/TableBuilder abstraction (already used by DraftConflictDialog) instead of a hand-rolled XPath row traversal, keeping only a minimal headline-text existence check where the shipped markup gives no other way to select the drafts table. Unverified: this suite still cannot be executed from this repository.
Fix wave from the final whole-branch review, addressing defects found at the seams between the seven already-reviewed tasks: - Add 'ibexa_locationview' to bazinga_js_translation active_domains: the domain was never dumped, so every quick-edit JS string (confirm/cancel button labels, all error notifications, the draft-conflict modal) rendered as its raw translation key instead of translated text. - Scope the quick-edit input's flex-grow rule away from checkbox/radio inputs, so the boolean editor's checkbox no longer stretches to fill the whole field row. - Add the missing form-control class to the four legacy-class editors (text, date, datetime, time), matching how the content edit form always pairs it with ibexa-input. - Pass the error message, not the Error object, to showErrorNotification in the three quick-edit call sites that were showing a literal "Error: " prefix. - Default the three quick-edit template variables in content_view_fields.html.twig so a downstream template extending it degrades to the feature being off instead of throwing under strict_variables. - Make QuickEditField::getFieldPosition() throw a clear, actionable RuntimeException naming the missing label instead of silently returning a position past the end of the list. - Log the error to the console in requestOpen()'s rejection handler, so a throwing third-party quickEdit.editors render() stays diagnosable. - Add cursor: wait for the quick-editable row's aria-busy state, giving visible feedback for a double click during a slow prefill.
Clear five deferred-minor items from the inline quick edit review:
- formatErrorMessage() now substitutes {fieldName} with a replacer
function instead of a replacement string, so a field name containing
$& or $$ is inserted literally instead of being read as a special
replacement pattern.
- cancelSession() now also calls abortPendingOpen(), so cancelling an
open session no longer leaves pendingOpenNode pointing at another
field's already-settled prefill.
- A refused open during an in-flight save now gets visible feedback: a
transient ibexa-content-preview--busy class is added to the fields
wrapper for the whole save transaction (held into the reload on
success, released on every other exit the busy guard itself releases
on), giving every quick-editable row a non-interactive cursor instead
of a silent no-op on a stray double-click.
- role/tabindex are restored on the field row before triggering
location.reload() on a successful publish, so a slow or blocked
reload no longer leaves the row inert with a stale aria-label.
- Investigated extending the _inputs.scss clear-button block to
--time/--datetime-local; left it alone since that block only ever
matches a custom clear-button wrapper the quick edit editors' native
time/datetime-local inputs never render, and extending it would risk
duplicating the browser's own clear affordance for those input types.
Fix a configuration defect where the inline_field_edit node used addDefaultsIfNotSet(), causing Symfony to materialise the fully-defaulted node for every ibexa.system.* scope, including siteaccesses that never declare inline_field_edit at all. This made the parser write a siteaccess-scoped inline_field_edit.enabled: false for every siteaccess, which ConfigResolver then resolves before siteaccess-group scope, silently overriding an explicit enabled: true set at group scope (the documented way to enable this feature). Remove addDefaultsIfNotSet() from InlineFieldEdit::addSemanticConfig(), in line with how Pagination.php handles this: children keep their per-child defaults, but the array node itself is only materialised for scopes that actually declare it, so an undeclared scope contributes nothing and the empty() guard in mapConfig() correctly skips it. An explicit enabled: false still yields a non-empty array (all three children present) and is still mapped, since the guard checks array emptiness, not the enabled value. The default-scope values in ezplatform_default_settings.yaml were already correct and did not need changes. Extend InlineFieldEditTest to process the real semantic config tree (via TreeBuilder/Processor against addSemanticConfig()) instead of only calling mapConfig() with hand-built arrays, since the defect lives in the tree definition itself. New tests confirm: an undeclared scope materialises no inline_field_edit key and triggers no contextual parameter; an explicit enabled: false is still mapped; and a scope declaring only enabled: true still gets sensible defaults for supported_field_types and excluded_field_types.
Applies the approved Figma design for inline quick edit, replacing the
confirm/cancel icon-button row with the new visual treatment:
- Idle state is unchanged. On hover/focus-visible of a quick-editable
row, the field VALUE only (not the name, not the whole row) gets a
5%-alpha primary background band, and a 16x16 decorative pencil
(sprite icon "edit") appears after the value text.
- Edit mode renders no buttons for the eight field types that save on
Enter/discard on Escape/outside-click (already implemented, verified
not rebuilt); the input gets a permanent 1px primary border, white
background, 4px radius and 8/4 padding, scoped to
.ibexa-quick-edit__input only.
- ibexa_text (the one field type whose Enter cannot mean "save",
because it inserts a newline in the textarea) keeps an explicit
action pair, restyled as small semibold text-styled <button>s
("Save"/"Discard") below the input instead of icon buttons.
- check-circle is no longer used by this feature; discard remains used
elsewhere in the app. Confirm/cancel translation keys are kept with
updated wording ("Save and publish" -> "Save",
"Discard changes" -> "Discard").
- Updates the Behat coverage (QuickEditField, QuickEditContext,
QuickEdit.feature) to match: most scenarios now save via Enter, and
a Description (Text block) fixture field is added to exercise the
remaining Save/Discard button pair.
See figma-design-report.md in the SDD folder for the full token audit,
the Enter-key-swallowing analysis per field type, and the CSS
[hidden]-vs-display bug caught and fixed while implementing the hover
band.
Fix the hover jump on quick-editable field rows: padding for the hover/focus tint was previously applied only in the :hover/:focus-visible state, so entering hover added 8px/4px of padding that displaced the value text. The hint icon's display:none -> display:block toggle also could not be transitioned. Reserve the padding, negative margin and border-radius on .ibexa-content-field__value in the base [data-quick-edit] state instead of only on hover, so the element's occupied (margin) box - and the text's position within it - stay identical between idle and hover; only background-color differs. The hint icon stays in flow at all times (opacity 0/1 with pointer-events: none) instead of toggling display, so its reserved space never changes either. Add a background-color/opacity transition using the existing $ibexa-admin-transition easing with a new, locally-scoped 0.15s duration (the existing $ibexa-admin-transition-duration of 0.4s is tuned for larger interactions like panel open/close and reads as sluggish for a frequently re-triggered hover tint), and disable both transitions under prefers-reduced-motion.
Fix a backward-compatibility break in getRequestHeaders introduced
earlier on this branch: the destructured accept option had no default
value, so TypeScript inferred it as a required property of the
function's parameter type. Every consumer that calls getRequestHeaders
without passing accept (e.g. ibexa/segmentation's
targeted.content.map.ts and content-preview/content.preview.ts, plus
admin-ui's own location.helper.js) now fails to type-check with
TS2345: "Property 'accept' is missing ... but required".
Default accept to null in the destructuring, matching this file's
existing convention for optional destructured options (see
timezone.helper.js's `timezone = null`). This makes accept optional in
the inferred type without touching any other parameter's requiredness
(extraHeaders is unchanged) and without altering runtime behaviour:
the existing `...(accept && { Accept: accept })` guard already omits
the Accept header whenever accept is falsy, which was already true
whenever it was omitted, so every existing call site (including the
quick-edit feature's six REST calls that pass accept explicitly)
produces byte-identical headers.
Register the quick-edit Behat classes as container services so the `admin-ui` and `admin-ui-full` suites can build QuickEditContext. QuickEditContext was added to both suites in behat_suites.yml, and its constructor pulls in QuickEditField, QuickEditDraftConflictModal and VersionsList, but none of the four new classes were ever declared in this bundle's explicit Behat service files. This repo wires Behat classes one line at a time with no resource globs, so an unregistered context/component is invisible to FriendsOfBehat's Symfony extension: Behat's own ContextFactory then tries to instantiate QuickEditContext directly and can't supply its constructor arguments, so suite construction fails before any scenario runs - which is why unrelated features (Roles, Trash, fields/other, ...) were failing too. Fix: register QuickEditContext in feature_contexts.yaml, and QuickEditField, QuickEditDraftConflictModal and VersionsList in components.yaml, following each file's existing one-line `Class: ~` style under _defaults (autowire/autoconfigure/public: true). No other dependency needed registering: Session and TableBuilder are already resolved for existing components using the identical constructor shape (e.g. DraftConflictDialog takes the same Session+TableBuilder pair VersionsList does), and ContentViewPage is already registered in pages.yaml.
Clear the SonarCloud reliability rating regression on this branch's new code (one bug plus eleven code smells) without weakening the quick-edit feature's accessibility or behaviour: - Replace the ibexa-content-field row's role="button"/tabindex="0" with a real <button> wrapping the pencil icon, carrying the accessible name that used to live on the row. A native button fires the same click event for a mouse click and for keyboard Enter/Space alike, so the activation listener now binds to that click instead of hand-rolling key handling on the row (Web:MouseEventWithoutKeyboardEquivalentCheck, Web:S6819). Double-click on the row keeps opening the editor for mouse users; Escape-to-cancel and Enter-to-save inside the open editor are unchanged. The row's hover/focus band now triggers on :focus-within instead of :focus-visible, since focus lands on the nested button rather than the row itself, and focus restored after a cancel now returns to that button instead of a row that can no longer take it. - Use Number.parseInt/Number.parseFloat instead of the bare globals, and an optional chain in getValidatorConstraints(), in quick.field.edit.editors.js (javascript:S7773, javascript:S6582). - In the Behat helpers: throw the existing ElementNotFoundException instead of a generic RuntimeException when a field label lookup fails (php:S112, following the same precedent Table.php already uses); document why QuickEditField's and VersionsList's verifyIsLoaded() are intentionally empty (php:S1186); and drop the constructors on QuickEditField and QuickEditDraftConflictModal that only forwarded to parent::__construct() (php:S1185). Added Behat coverage for opening quick edit via the new trigger button.
|
Steveb-p
reviewed
Aug 21, 2026
Comment on lines
+8
to
+10
| {% set inline_field_edit = inline_field_edit|default({enabled: false, field_types: []}) %} | ||
| {% set can_edit = can_edit|default(false) %} | ||
| {% set current_language_code = current_language_code|default(null) %} |
Contributor
There was a problem hiding this comment.
If these values are expected to be passed from the ContentTab rendering class, then they should not have defaults. Template should fail loudly if variables aren't passed in while they're expected.
Comment on lines
+50
to
+54
| {# The real activation affordance. A genuine <button>, not the row, carries the | ||
| accessible name and native keyboard semantics, so Enter/Space fire an | ||
| ordinary `click` event here - admin.location.quick.field.edit.js listens for | ||
| that instead of hand-rolling key handling on the row. The icon inside stays | ||
| decorative/aria-hidden; the button's aria-label is the accessible name. #} |
Contributor
There was a problem hiding this comment.
It' stating the obvious (it is the section required for quick field edit) and tries to explain a quirk which - to be honest - I have no idea what it is about?
Does this react to enter/space key presses? What needs to be focused for it to react to it? Is the button visible on the page, or hidden? 🤔
| } | ||
|
|
||
| const textResponse = await response.text(); | ||
| const errorMessage = extractXmlErrorMessage(textResponse) || textResponse.trim() || response.statusText; |
Contributor
There was a problem hiding this comment.
Why are we considering XML here? We wanted to get rid of XML REST API usage withing AdminUI if at all possible.
Comment on lines
+53
to
+65
| ->arrayNode('supported_field_types') | ||
| ->info('List of Field Type identifiers eligible for inline editing.') | ||
| ->defaultValue(self::DEFAULT_SUPPORTED_FIELD_TYPES) | ||
| ->scalarPrototype()->end() | ||
| ->end() | ||
| ->arrayNode('excluded_field_types') | ||
| ->info( | ||
| 'List of Field Type identifiers to exclude from inline editing, ' . | ||
| 'subtracted from "supported_field_types".' | ||
| ) | ||
| ->defaultValue([]) | ||
| ->scalarPrototype()->end() | ||
| ->end() |
Contributor
There was a problem hiding this comment.
I am not a fan of two sources of truth, but since this is done on tiered parameters (i.e. one parameter can come from defaults, while another might be on site-access level) I am conditionally okay with this.
Comment on lines
+42
to
+48
| # Per the approved design, the editor row renders no confirm/cancel buttons at all for most field | ||
| # types - Enter saves, Escape/an outside click discards - so most scenarios below drive save via | ||
| # "I press the Enter key while quick-editing ...". The one exception is `ibexa_text` (the | ||
| # "Description" field added to the content type below): its textarea treats a plain Enter as a | ||
| # newline, so it alone renders an explicit Save/Discard button pair, and "Quick edit updates a | ||
| # text field via the Save button"/"Cancelling quick edit via the Discard button discards the | ||
| # change" below are the coverage for that pair specifically. |
Contributor
There was a problem hiding this comment.
I won't contest the design, but...
Instead of relying on double-click / escape press / etc. directly, and having different routes for different field types to facilitate described behavior, consider placing hidden buttons anyway and making your click / keypresses interact with those buttons instead.
It will make it easier to change your mind later, and should significantly reduce the difficulty of tests - at even in your code, in behat, there are notes that certain drivers do not emulate those events as you'd expect.
Comment on lines
+4
to
+40
| # ============================================================================================ | ||
| # PREREQUISITE - READ BEFORE RUNNING: every scenario below except the last one CANNOT PASS | ||
| # against this suite's current configuration, and will hard-fail at the first double-click. | ||
| # | ||
| # `inline_field_edit.enabled` (src/bundle/DependencyInjection/Configuration/Parser/ | ||
| # InlineFieldEdit.php) defaults to FALSE. Without it set to true for the siteaccess/siteaccess | ||
| # group the "admin" login runs under, content_view_fields.html.twig never renders the | ||
| # `data-quick-edit` attribute, and every "I double-click the ... field" step in this file will | ||
| # time out finding an editor that the page never offers. | ||
| # | ||
| # To run anything in this file besides the last scenario, the target project's own | ||
| # configuration must set, for the relevant siteaccess/siteaccess group: | ||
| # ibexa.system.<siteaccess-group>.inline_field_edit.enabled: true | ||
| # This is not something admin-ui's own repository can supply - it is a bundle, with no | ||
| # project-level `config/packages` of its own - so the override has to live in whichever full | ||
| # Ibexa project `browser-tests.yml` provisions to run this suite. No such override exists | ||
| # anywhere today. | ||
| # ============================================================================================ | ||
| # | ||
| # Because of that, this suite also has no in-suite mechanism to flip a siteaccess configuration | ||
| # flag for a scenario or a project edition: existing config-gated features (e.g. | ||
| # `user_profile.enabled`, also default-false) are instead covered by simply omitting the feature | ||
| # file's scenarios from the `@IbexaOSS`/`@IbexaHeadless`/`@IbexaExperience`/`@IbexaCommerce` tags | ||
| # used to select suites in CI (see UserProfile.feature, which carries no `@IbexaOSS` tag for | ||
| # exactly this reason), relying on the target project's own configuration to turn the feature on | ||
| # where it is meant to run. `inline_field_edit.enabled` has no project shipping it on by default | ||
| # anywhere yet, so the same treatment applies here: every scenario below that needs it on is left | ||
| # without an edition tag - they will not run under any of the `browser-tests.yaml` CI jobs, each | ||
| # of which filters by exactly one edition tag. | ||
| # | ||
| # On top of that, and because omitting a tag reads as "not yet categorised" rather than "cannot | ||
| # pass without a precondition", every one of those scenarios additionally carries | ||
| # `@requires-config:inline_field_edit.enabled` - a new tag, since no existing tag in this suite | ||
| # names a configuration prerequisite; it follows the closest existing convention for a | ||
| # colon-qualified tag (`@APIUser:admin`) rather than inventing an unrelated style. Only the | ||
| # "feature flag is off" scenario needs no override and no such tag, since it asserts today's | ||
| # actual default; it keeps the standard edition tags instead. |
Contributor
There was a problem hiding this comment.
Basically, CI won't show those tests as passing or failing, because they aren't even executed
Contributor
There was a problem hiding this comment.
seems they are here
- Will be consumed by 1 parallel Processes.
15/16 ✔ 6 s 93 ms /var/www/vendor/ibexa/admin-ui/features/standard/QuickEdit.feature
ah, but even the PR desc says otherwise, I would say this is major problem and we should have a way of testing this before this PR is merged.
Contributor
There was a problem hiding this comment.
but I will be frank, this whole comment sounds like opus slop, hard to read and it should be reformated and debullshited.
Contributor
There was a problem hiding this comment.
have you tried
Given I set configuration to "admin_group" siteaccess under "inline_field_edit" key
"""
enabled: true
"""
Implementation: vendor/ibexa/behat/src/lib/Core/Context/ConfigurationContext.php:124.
as it seems we have a proper way to enable configuration for behat tests.
@kmadejski
Contributor
There was a problem hiding this comment.
My 2 cents here: from Dawid's citation we can see that those tests ran for 6 seconds - which means they did not run at all. Here https://github.com/ibexa/admin-ui/blob/6.0/.github/workflows/browser-tests.yaml#L16 we can see that admin-ui jobs run on tags, this feature file does not have any tags -> here is an example config that should work https://github.com/ibexa/admin-ui/blob/6.0/features/standard/Languages.feature#L1
| try { | ||
| return JSON.parse(fieldNode.dataset.fieldValidators ?? '{}'); | ||
| } catch { | ||
| return {}; |
Contributor
There was a problem hiding this comment.
JSON.parse.
alongosz
changed the title
alongosz
requested review from
GrabowskiM,
OstafinL,
ViniTou,
albozek,
alekmick,
alongosz,
barw4,
bnowak,
ciastektk,
dew326,
konradoboza,
mikadamczyk,
tbialcz,
tischsoic and
wiewiurdp
August 21, 2026 11:01
ViniTou
reviewed
Aug 21, 2026
| 'field_definitions_by_group' => $fieldDefinitionsByGroup, | ||
| 'languages' => $languages, | ||
| 'location' => $contextParameters['location'], | ||
| 'can_edit' => $this->permissionResolver->canUser('content', 'edit', $content), |
Contributor
There was a problem hiding this comment.
Isnt that too broad? what if user have content/edit but no content publish? It would still render quick-edit controls and try to publishDraft() unless I am reading smth wrong.
Contributor
There was a problem hiding this comment.
even more, as we load all Drafts in js script, for checking of existence of drafts - we also need to include content/versionread here. Proably all checks should also take language into consideration?
| // the draft goes away before the error reaches the user. The cleanup itself is best | ||
| // effort - its own failure must not replace the message explaining what went wrong. | ||
| if (draftHref !== null) { | ||
| await deleteDraft(draftHref).catch(() => {}); |
Contributor
There was a problem hiding this comment.
what if user does not have content/versionremove? thats gonna leave orphaned drafts - this should go thru sudo probably.
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
7 participants
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.
Description
Adds inline quick edit to the admin content view. On the location view's Fields tab, a user
double-clicks (or focuses and presses Enter/Space on) a rendered field value, edits it in place, and
confirms. The change is saved by branching a draft from the current version over REST, PATCHing the
single field, publishing, and cleaning up on any failure.
Nine simple field types are supported, through a registry any bundle can extend.
Screen.Recording.2026-08-21.at.11.28.35.mov
Scope
ibexa_string,ibexa_text,ibexa_email,ibexa_integer,ibexa_float,ibexa_boolean,ibexa_date,ibexa_datetime,ibexa_time— allibexa/corefield types, all editors in admin-ui.Deliberately not in this PR: Selection, Country, Keyword, URL and ISBN (they need product
decisions on their controls and on exposing field settings to JS); rich text, images, relations,
matrix, address, measurement, taxonomy, page and the commerce field types (each needs its own UX and
most likely a modal host rather than an inline row). The registry is what makes each of those a
purely additive follow-up, with no admin-ui release in between.
Rollout
Behind a siteaccess-aware flag, disabled by default:
The node is top-level rather than nested under
content_view, becausecontent_viewis alreadyowned by
ibexa/corewith an incompatible prototyped view-type schema.Notable implementation points
flag is on, the field type is supported, the field is translatable or we are viewing the main
language, and the current user passes a per-object
content/editcheck. REST enforces again onsave.
render(hash, config)plusharvest(element)plus an optionalvalidate(element). It never fetches, never guards, nevertouches the draft transaction. A generation token makes a superseded or cancelled open mutate
nothing; a single busy guard, consulted by every entry point, makes two overlapping
COPY → PATCH → PUBLISH chains impossible.
Locationheader, never a parsed body, so a structurallyunexpected 201 cannot strand an orphan draft. Every failure after the draft exists deletes it, and
cleanup failure cannot mask the server's message.
Acceptis set explicitly on all six REST calls, including the two whose success is 204. Anunset
Acceptresolves server-side to the XML visitor, which used to surface as a JSON parse errorinstead of the server's message —
helpers/request.helper.jsis also hardened here to becontent-type aware (see below).
Date\Value::fromTimestamp()buildsnew DateTime("@ts")and zeroes the time, so the round trip isbyte-stable and the displayed date never shifts with the viewer's timezone. Boolean never sends
null(Checkbox\Type::fromHashhas no null guard).initialLanguageCodeis always sentexplicitly.
Interaction design
The inline editor follows the approved Cohesivo design:
icon appears after the value.
clicking outside discards.
ibexa_textonly: because a<textarea>treats Enter as a newline, that one field type getsexplicit Save / Discard text actions beneath the input.
Note that clicking outside discards. This differs from the proof of concept, which saved on
outside-click; the change is deliberate, because this flow publishes content and a stray click must
never publish.
Every colour comes from an existing token — the design's purple is exactly
$ibexa-color-primary. Theone deliberate deviation is the "Discard" label colour, specified as
#3E4145, for which the nearestexisting token (
$ibexa-color-dark-400) was used rather than introducing a hex.Deploying this: JS translations need a dump before the asset build
This feature adds a JS translation domain, which takes three steps in this order:
ibexa_locationviewinbazinga_js_translation.yamlactive_domains(done in this PR);bazinga:js-translation:dump <public>/assets --merge-domains);Step 3 must follow step 2, because
Resources/encore/ibexa.js.config.jsreadspublic/assets/translations/*.jsat build time and bundles those catalogues into theibexa-admin-ui-layout-jsentry. Build before dumping and every string silently renders as its rawtranslation key. This was hit and diagnosed during live testing.
Configuration must not be defaulted per siteaccess
The config parser deliberately does not use
->addDefaultsIfNotSet(). That call materialises thenode for every scope under
ibexa.system.*, which makes the parser writeenabled: falseat everyindividual siteaccess scope; since the ConfigResolver resolves siteaccess before siteaccess-group,
a defaulted
falsethen silently overrides the documentedadmin_groupsetting and the feature cannotbe switched on at all. Defaults live only at the
defaultscope inezplatform_default_settings.yaml,matching the existing
Paginationparser. There is a regression test for this.Incidental fix worth its own attention
helpers/request.helper.jspreviously calledresponse.json()unconditionally on any non-okresponse, so an XML
ErrorMessagebody surfaced to the user asUnexpected token '<'instead of theserver's message. It is now content-type aware, and
getRequestHeadersgained an opt-inacceptoption. The default
Acceptwas deliberately left unchanged — flipping it would alter whichserver-side visitor serialises responses for every existing JS caller in the product, which does not
belong in a feature PR. That is worth a separate ticket.
Upgrade note
Ibexa\AdminUi\Tab\LocationView\ContentTabgained two required constructor arguments(a permission resolver and a request stack). It is autowired, so no configuration change is needed,
but a downstream project that extends it and calls
parent::__construct(...)explicitly must adapt.Testing
Rector all clean.
paths, the draft-conflict modal, and the regression cases this feature's proof of concept earned.
Known testing limitations — please read before approving
inline_field_edit.enableddefaults tofalse and nothing in admin-ui can turn it on for the browser-test installation, whose configuration
lives outside this repository. Those scenarios carry
@requires-config:inline_field_edit.enabledand a header comment stating the prerequisite. Theyare inert until a follow-up enables the flag in the test installation.
asserting it would prove nothing, because CI browsers run a UTC system clock — a real
timezone-offset bug would stay invisible while the test passed. The scenario was therefore renamed
to claim only what it checks. This contract currently rests on code review.
registry have no unit tests. Introducing a test framework was judged out of scope for a feature PR;
the converters are pure and dependency-free so a future harness can cover them with no refactor.
covers neither
.twignor.xliff.🤖 Generated with Claude Code