Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions .changeset/8506-detail-placeholders-shared-empty-value.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
---
'@object-ui/plugin-detail': patch
'@object-ui/components': patch
---

fix(plugin-detail): the record page's two hand-rolled empty placeholders become the shared `EmptyValue`

`DetailSection` (the details body grid) and `HeaderHighlight` (the ADR-0085
highlights strip) each spelled their own `<span>—</span>` for a missing value and
resolved their own accessible name from `detail.noValue`. Both now render
`@object-ui/components`' `EmptyValue`, which resolves exactly that key with the
same `"No value"` English fallback through a provider-safe hook — so the
accessible name is byte-identical in every locale, and the placeholder gains the
shared `data-slot="empty-value"`, `no-underline` and `select-none` treatment.

Two deliberate visual changes, decided per site:

- Both retire their own `text-muted-foreground/60 text-sm` treatment and take the
shared `text-muted-foreground/50`. That was not a neutral difference: a
type-aware cell renderer already draws the same shared component at `/50` in
the very next row or chip (an unparseable `datetime` is the reachable case), so
one section could show two dashes in two greys.
- `DetailSection` keeps its `title` hover affordance, and keeps it working: the
shared component is `pointer-events-none`, on which a `title` never renders a
tooltip, so that one site restores `pointer-events-auto`. `HeaderHighlight` has
no `title` and takes the shared non-interactive default unchanged.

`EmptyValue`'s docblock also stops calling its `glyph` default an "en-dash"; it
is and always was U+2014, an em-dash. Comment only — no behaviour change in
`@object-ui/components`.
18 changes: 15 additions & 3 deletions packages/components/src/custom/empty.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,21 @@ function EmptyContent({ className, ...props }: React.ComponentProps<"div">) {
* EmptyValue — universal inline placeholder for missing cell/field values.
*
* Use this anywhere a renderer would otherwise show "-" or "—" for a null,
* undefined or empty value. It renders a muted, non-interactive en-dash that
* does not inherit link/button colors from surrounding ancestors, so a missing
* value never looks clickable.
* undefined or empty value. It renders a muted, non-interactive EM-dash (the
* `glyph` default below is U+2014, and every call site depends on that width)
* that does not inherit link/button colors from surrounding ancestors, so a
* missing value never looks clickable. This sentence said "en-dash" until
* objectui#8506: the word was wrong, never the code — do not "fix" the glyph
* to match a stale docblock.
*
* ⚠️ A caller that passes a `title` through `...props` must also pass
* `pointer-events-auto` in `className`. `pointer-events-none` below stops this
* span being a hit target, so a `title` on it never renders a tooltip — the
* hover falls through to whichever ancestor has one. The attribute stays in the
* DOM either way, which is exactly why nothing catches it: a test that reads
* `getAttribute('title')` is green over a tooltip that can never appear
* (objectui#8506, where `DetailSection` kept such a `title` and two landed pins
* navigate by it).
*/
function EmptyValue({
className,
Expand Down
33 changes: 27 additions & 6 deletions packages/plugin-detail/src/DetailSection.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
CollapsibleTrigger,
CollapsibleContent,
Button,
EmptyValue,
Tooltip,
TooltipContent,
TooltipProvider,
Expand Down Expand Up @@ -336,14 +337,34 @@ export const DetailSection: React.FC<DetailSectionProps> = ({
}
const isEmpty = !hasCellValue(value);
if (isEmpty) {
// The SHARED placeholder (objectui#8506). The span that stood here
// spelled its own em-dash and resolved its own `aria-label` from
// `detail.noValue`; `EmptyValue` resolves EXACTLY that key, with the
// same `"No value"` English fallback, through a provider-safe hook — so
// the accessible name survives byte-for-byte in every locale and the
// duplicate resolution goes away. Two deliberate props:
//
// - `title` — the sighted-mouse counterpart of the accessible name,
// added alongside it in the same commit. It rides through
// `EmptyValue`'s `...props`. `pointer-events-auto` is what keeps it
// a real tooltip: the shared component sets `pointer-events-none`,
// which stops the placeholder being a hit target, and a `title` on
// an element that can never be hovered is a dead attribute — the
// hover would fall through to this row's own
// `title={t('detail.editInlineHint')}` instead. It is also the only
// instrument that tells THIS placeholder apart from the one a cell
// renderer draws one row over (`DetailSection.emptinessAuthority-8376`,
// `record-details.emptySectionDefault`), which read it by title.
// - no `className` typography — the retired `text-muted-foreground/60
// text-sm` is a deliberate change, not an oversight: it made this
// row's dash a different grey and a different size from the dash
// `DateTimeCellRenderer` draws for an unparseable value in the very
// next row of the same section. They now draw identically.
return (
<span
className="text-muted-foreground/60 text-sm select-none"
aria-label={t('detail.noValue', { defaultValue: 'No value' })}
<EmptyValue
className="pointer-events-auto"
title={t('detail.noValue', { defaultValue: 'No value' })}
>
</span>
/>
);
}
// Use type-aware cell renderer; respect format hints (e.g.
Expand Down
27 changes: 21 additions & 6 deletions packages/plugin-detail/src/HeaderHighlight.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import * as React from 'react';
import {
cn,
Button,
EmptyValue,
Tooltip,
TooltipContent,
TooltipProvider,
Expand Down Expand Up @@ -249,12 +250,26 @@ export const HeaderHighlight: React.FC<HeaderHighlightProps> = ({
</span>
)
) : isEmpty ? (
<span
className="block text-sm text-muted-foreground/60 select-none"
aria-label={t('detail.noValue', { defaultValue: 'No value' })}
>
</span>
// The SHARED placeholder (objectui#8506). This span
// spelled its own em-dash and resolved its own
// `aria-label` from `detail.noValue`; `EmptyValue`
// resolves EXACTLY that key with the same `"No value"`
// English fallback, so the accessible name survives
// byte-for-byte in every locale. Unlike the body grid's
// placeholder this one carries NO `title`, so nothing
// here needs `pointer-events` back — the strip's chip
// keeps its own `onDoubleClick`, which fires from the
// chip, not from the dash.
//
// `block` is layout, not typography: the filled branch
// below is a `block` span inside the same
// `min-w-0 flex-1` box, and an inline dash would sit on
// a different baseline. The retired
// `text-sm text-muted-foreground/60` IS a deliberate
// typography change — it made the strip's own dash a
// different grey from the one a cell renderer draws for
// a chip two columns over.
<EmptyValue className="block" />
) : (
<span
// Hover reveals the full value; for option-backed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,22 @@
*
* ## Reading the affordances
*
* Two different placeholders can carry `aria-label="No value"` on this page:
* `HeaderHighlight`'s own span, and `@object-ui/components`' `EmptyValue`, which
* the field cell renderers draw. Only the latter carries
* `data-slot="empty-value"`, so the strip's own affordance is read as
* `[aria-label="No value"]:not([data-slot="empty-value"])` and cannot pick up a
* cell renderer's placeholder by accident. The summary chips are read through
* the `aria-label="<field>: <display>"` each `Badge` already carries, which
* names the field — so "which chip" is asserted, not merely "how many".
* ⚠️ This instrument was re-derived by objectui#8506 and the reason is worth
* keeping. It used to read the strip's affordance as
* `[aria-label="No value"]:not([data-slot="empty-value"])`, because two
* DIFFERENT placeholders could carry that name on this page — `HeaderHighlight`'s
* own hand-rolled span, and `@object-ui/components`' `EmptyValue`, which the
* field cell renderers draw — and only the latter carried the `data-slot`.
* objectui#8506 made the strip adopt the shared component, so that `:not()`
* matched NOTHING and this case failed while the surface was perfectly correct:
* the harness navigated by exactly the thing that changed.
*
* The replacement navigates by the field LABEL, which no placeholder change can
* move: each chip is `<span>{label}</span>` followed by the value slot, so
* `chipOf(label)` is the chip box and the affordance is read INSIDE it. That
* also strengthens the assertion from "how many" to "which chip", the way the
* summary chips below are already read through the `aria-label="<field>:
* <display>"` each `Badge` carries.
*/

import { describe, it, expect, beforeAll, afterEach } from 'vitest';
Expand Down Expand Up @@ -90,9 +98,16 @@ afterEach(cleanup);
* CI summary would carry none of the reason. */
const shown = (text: string) => screen.queryByText(text) !== null;

/** `HeaderHighlight`'s OWN em-dash affordance — see the docblock. */
const stripAffordances = (c: HTMLElement) =>
c.querySelectorAll('[aria-label="No value"]:not([data-slot="empty-value"])');
/**
* The chip box a field's LABEL sits in — the strip renders the label as the
* chip's first child. Anchored on the label, never on the placeholder: see the
* docblock.
*/
const chipOf = (label: string) => screen.getByText(label).parentElement as HTMLElement;

/** WHICH of `labels`' chips draw the em-dash affordance — see the docblock. */
const chipsDrawingAffordance = (labels: string[]) =>
labels.filter((l) => chipOf(l).querySelector('[data-slot="empty-value"]') !== null);

/** The summary chip for `field`, read by the `aria-label` the Badge carries. */
const chipFor = (c: HTMLElement, field: string) =>
Expand Down Expand Up @@ -146,9 +161,9 @@ describe('HeaderHighlight — the ADR-0085 strip trims (#8394)', () => {
expect(shown('Notes'), 'CONTROL: the whitespace-only chip is on screen at all').toBe(true);

expect(
stripAffordances(container),
chipsDrawingAffordance(['Industry', 'Notes', 'Amount']),
'exactly the whitespace-only chip draws the strip\'s `No value` em-dash',
).toHaveLength(1);
).toEqual(['Notes']);
expect(
container.textContent,
'the raw whitespace must not reach the DOM as a rendered value',
Expand Down Expand Up @@ -180,15 +195,15 @@ describe('HeaderHighlight — the ADR-0085 strip trims (#8394)', () => {
expect(shown('Alpha'), 'a multiselect array renders its options').toBe(true);
expect(shown('Beta'), 'a multiselect array renders its options').toBe(true);
expect(
stripAffordances(container),
chipsDrawingAffordance(['Office Location', 'Tags', 'Industry']),
'NO chip here is empty — an object value is a value on this surface',
).toHaveLength(0);
).toEqual([]);
});

it('NON-REGRESSION — an emptiness test that answers EMPTY for everything is refused (`0` and a string)', () => {
// Every whitespace case above is ALSO satisfied by deleting the feature.
// This one is not: `0` is the value a careless `!value` rewrite loses.
const { container } = render(
render(
<HeaderHighlight
fields={highlights(['industry', 'amount'])}
data={{ industry: 'Manufacturing', amount: 0 }}
Expand All @@ -199,9 +214,9 @@ describe('HeaderHighlight — the ADR-0085 strip trims (#8394)', () => {
expect(shown('Manufacturing'), 'a plain string is a value').toBe(true);
expect(shown('0'), '`0` is a value, not a blank').toBe(true);
expect(
stripAffordances(container),
chipsDrawingAffordance(['Industry', 'Amount']),
'no chip draws the em-dash — nothing here is empty',
).toHaveLength(0);
).toEqual([]);
});
});

Expand Down
Loading
Loading