feat: colour tag chips by tag name - #12892
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
37eb28c to
fd0f182
Compare
LukasHirt
left a comment
LukasHirt
left a comment
There was a problem hiding this comment.
TBH, I am not sure whether I like the increased boilerplate when using the OcTag component. Ideally, I would love to see this baked in somehow directly into the component itself so that we don't need to always pass it as props - is there any chance to make it work like that?
Also, while testing it, I found the following:
Is that on purpose that the tag icon is missing from the badge in the resource table now?
Also, the +1 is not aligned vertically.
I am not seeing the tags being coloured - I checked the theme.json response and I do not see the tags being returned there even though they are part of the theme.
Tag chips were all the same grey, so a file's tags were hard to tell apart at a glance and the same tag looked like nothing in particular across the app. Fill each chip with a colour derived from the tag name: `hash(tag) % length` picks a slot in the theme's new `designTokens.tagColorsList`, emitted as `--oc-color-tag-N`. The colour is derived, never stored, so a tag looks the same for every user on every device with no API, storage or migration change. The label colour is computed per fill rather than pinned to a token: the fills come from the theme, so one token cannot stay legible against all of them, and `--oc-color-text-inverse` gets it backwards in the light theme. The calculation lives in `useTagColor` rather than in `OcTag` because the theme store is a reactive dependency and `getComputedStyle` is not — resolving it in the component would leave the label stale after a theme switch. The fill is read straight out of `tagColorsList` rather than from the emitted css var, since `getHexFromCssVar` reports an unresolvable var as `#000000` and would hand a pale fill an illegible white label. A filled chip carries `oc-tag-filled`, which forces the colours of everything nested inside it down as `inherit`/`currentColor`. Those nested elements otherwise repaint themselves — a `router-link` takes the global anchor colour, a raw `OcButton` its own swatch, `.oc-tag .oc-icon > svg` a muted fill — which left navy labels and a washed-out close icon on the deep light-theme fills, and labels that were too light on the pale dark-theme ones. Nothing hardcodes a colour, so both themes are covered by construction. `OcTag` keeps its default appearance: it is the generic badge (the app-store "most recent" pill, instance badges, the disabled-space indicator, the `+N` counter), so both `fillColor` and `labelColor` are opt-in. `initializeThemes` now merges with replace semantics for arrays. deepmerge concatenates by default, which would turn a `tagColorsList` present in both `defaults` and a theme into 60 entries and shift every hue. Also make the tag overflow indicator on the files list a real, keyboard reachable button that shows the hidden tags in a popover instead of opening the sidebar. That popover is sized to its tags: `OcDrop` is a fixed 300px wide, which left it nearly empty when it held a single chip capped at 80px by `.resource-table-tag`, with the right alignment the tags column passes down pushing that chip to the far edge. 300px is kept only as a cap so a long tag still truncates. Both `theme.json` copies gain 30 placeholder fills per theme, per theme rather than in `defaults`, so the feature can be exercised end to end while design's real values are outstanding. They were generated by binary-searching HSL lightness for a target relative luminance per hue, since equal-lightness hues differ a lot in perceived brightness: deep fills with white labels in the light themes, pale fills with black labels in the dark ones. Worst-case label contrast is 6.31:1 (light) and 8.52:1 (dark), higher in both high-contrast themes. Both copies are needed — web-runtime's file is what `pnpm build` publishes to `web/dist/themes`, while an ocis binary serves the go:embed-ed `services/web/assets/themes` copy instead (services/web/pkg/server/http/server.go layers it over the embedded core assets). Every theme in a file has the same list length, so a tag keeps its colour slot across a theme switch. TODO: replace the placeholder palettes with the ones from design.
Could you confirm that's what you were running?
|
Collapse the two `OcTag` colour props into one. `fillColor` and `labelColor` made every call site restate a pair that is really one decision, so the theme store now emits a *pair* per palette entry — `--oc-color-tag-N` alongside a derived `--oc-color-tag-N-text` — and `tagColorVarsFor(index)` maps an index onto both. `useTagColor` is down to `tagColorIndex(name)`, and call sites pass a single `colorIndex`. This supersedes the paragraph in the previous commit that describes `fillColor`/`labelColor` as separately opt-in. Derive the label colour from the theme instead of hardcoding it. `useTagColor` held literal BLACK and WHITE constants; the pairing is now computed once per theme in the store from the theme's own `colorPalette['text-default']` and `['text-inverse']`, so a theme that restyles its text colours restyles its tag labels with it. Two properties of real theme data that this has to respect. `text-default` is not reliably the darker of the two candidates — the light theme states `#041e42` while the dark theme pairs `#dadcdf` with a `text-inverse` of `#000000` — so `pickReadableTextColor` compares two unlabelled candidates and never assumes which is which. And a theme colour is not always hex: the shipped ownCloud light theme writes `text-default` as `oklch(13% 0.028 261.692)`, for which `hexToRgb` returns null, and an unguarded version would have painted every label in it, dark-on-dark included. A pair that cannot be measured falls back to plain black and white. Normalise the hash at the source. `hashString` returned a possibly negative int32 and left every caller to coerce it; it now returns `hash >>> 0`. This is output-preserving — `ToInt32(h >>> 0) === ToInt32(h)`, so the `& 0x00ffffff` in `generateHashedColorForString` is unchanged. Align the tag overflow indicator with the cell text. `vertical-align: text-bottom` on `.resource-table-tag-more` puts `+1` on the text's descender, and both it and the now `inline-flex` chip wrapper are `vertical-align: middle` so the two boxes agree. Min label contrast across all ten shipped theme entries is 6.33:1. TODO: replace the placeholder palettes with the ones from design.
fd0f182 to
24bffe5
Compare
…asured `pickReadableTextColor` treated an unmeasurable background the same as an unmeasurable candidate and returned the first candidate for both. The theme store already substitutes black and white when a candidate cannot be read, so that branch was unreachable from it; the reachable one was an unmeasurable *fill*, which returned the theme's `text-default` unguarded. That is the one case where the wrong answer is invisible until it ships. A fill does not have to be hex to work: `oklch()` is valid css, so the browser paints the chip while the contrast maths gets nothing to compare, and neither candidate is known to be the darker one — the shipped dark theme states `text-default` as `#dadcdf` against a `text-inverse` of `#000000`. A pale fill would have taken a near-white label. Every placeholder palette entry is currently hex, so nothing renders wrong today; replacing them with design's real values is what would have exposed it. So the function returns null when any of the three colours cannot be measured, and the store omits that entry's `-text` property rather than emitting a colour it could not choose. The label then inherits the surrounding text colour, which is legible against the page by construction — the behaviour the pre-review `tagLabelColor` had when it returned an empty string. `tagColorVarsFor` names `currentColor` as the var's fallback to state that, instead of relying on an undefined custom property being invalid at computed-value time. The fallback is written without a space after the comma because happy-dom, which the unit tests run in, discards a `var()` declaration that has one. The test for the old behaviour asserted "falls back to the dark candidate" but passed only because the fixture happened to pass the dark candidate first — the function has no concept of which is darker. Replaced by tests naming what it actually does, plus the two cases that were missing: an unmeasurable fill emits no label colour, and a measurable entry beside it is unaffected.
That mockup won't fully reflect the current state of the UI. I'm not necessarily against removing the icon from there but since that changes the current look, I would suggest not to do it in this PR. |
I did not set this env var to use the default ones - shouldn't that include the colors? I will do some more testing today. |
The chips lost the `price-tag-3` icon and their rounded corners, both read off the ticket's mockup. The mockup does not reflect the current UI, and dropping the icon is a design change in its own right, so it does not belong in a PR about colour: colour is now the only thing this changes about a chip. The icon and `rounded` came out together under one spec decision and go back together, at every site that had them — the resource table rows, the new tag overflow popover, and both `TagsSelect` templates. The search tag filter had no chip at all before, only an icon and a label; it keeps the chip, since that is where the colour shows, and takes the icon back inside it. No styling was needed for the icon on a fill: `.oc-tag.oc-tag-filled .oc-icon > svg` already outranks `.oc-icon-passive > svg`, so the icon inherits the label colour that goes with the fill rather than the passive swatch. `TagsSelect.spec.ts` asserted the icon was absent, but could not have caught it either way: `OcIcon` renders through `inline-svg`, which resolves to nothing in happy-dom, so no icon's name reaches the rendered html and the assertion held whether or not the icon was there. It now reads the name off the `OcIcon` prop, and the resource table row — the one site the review actually pointed at, which had no icon coverage — gets the same assertion.
Updated |
theme.json is served by the ocis web service, not the vite dev server, so a frontend from source against a pre-branch ocis image serves the old theme. WEB_ASSET_THEMES_PATH was only a shortcut to avoid the rebuild. |
|
I'm running the binary directly. Not docker image. So in my setup, it should take whatever is filled in the theme.json file and serve that. |
|
I did one more test and colors are correctly rendering now. |
| <oc-drop | ||
| v-if="item.tags.length > 2" | ||
| :toggle="`#tags-overflow-${resourceDomSelector(item)}`" | ||
| mode="hover" |
There was a problem hiding this comment.
Clicking +N hides the popover it opens.
mode="hover" maps to trigger: 'mouseenter focus' (OcDrop.vue:185) and hideOnClick: true is hardcoded (OcDrop.vue:234). tippy only bails out on a reference click when the trigger contains click (tippy.cjs.js:846), so it falls through to instance.hide() and sets didHideDueToDocumentMouseDown, which then blocks the focus trigger from reopening. Mouse users have to move the pointer off and back on.
hideOnClick isn't a prop, so this can't be fixed at the call site — it needs mode="click" or a new prop on OcDrop.
| </component> | ||
| <oc-tag | ||
| v-if="item.tags.length > 2" | ||
| :id="`tags-overflow-${resourceDomSelector(item)}`" |
There was a problem hiding this comment.
No aria-expanded / aria-haspopup on this button, so screen readers announce an action button with no indication that a popover exists or is open.
Related: the changelog calls this "a real, keyboard reachable button", but Enter/Space does nothing — the drop is hover/focus-driven. Might be worth deciding whether activation should toggle it.
| size="small" | ||
| class="resource-table-tag-more" | ||
| @click="openTagsSidebar" | ||
| :aria-label="getTagsOverflowAriaLabel(item)" |
There was a problem hiding this comment.
WCAG 2.5.3 Label in Name: the visible text is + 3 but the accessible name is Show 3 more tags. The visible string isn't contained in the name, so speech-input users saying "click plus three" get no match.
Keeping + 3 in the name and appending the description would cover it.
| class="vs__deselect-lock" | ||
| name="lock" | ||
| size="small" | ||
| variation="inherit" |
There was a problem hiding this comment.
The lock icon has no accessible-label, so OcIcon renders it aria-hidden="true". Screen-reader users never learn the tag is read-only — they just find the deselect button absent.
Since this line is already being touched: :accessible-label="$gettext('Read-only tag')".
| "icon-medical": "#0984db" | ||
| } | ||
| }, | ||
| "tagColorsList": [ |
There was a problem hiding this comment.
tagColorsList is added to each theme but not to defaults.designTokens — here or in services/web/assets/themes/owncloud/theme.json, where defaults carry only breakpoints, fontSizes, sizes, spacing. Every shipped theme has 30 entries; defaults have none.
So a deployment serving its own theme.json gets no tag colours at all. Lower impact now that 2150c3d restored the icon and rounded corners (chips degrade to the previous look rather than breaking), but putting the list in defaults would let custom themes inherit a palette for free.
Five review comments landed on #12892 after it merged. **The tag overflow popover dismissed itself on click.** `mode="hover"` maps to tippy's `mouseenter focus` trigger, and `hideOnClick` is hardcoded true in `OcDrop`. tippy only skips its document-press hide when the trigger contains `click`, so a click on the button fell through to `hide()` and set `didHideDueToDocumentMouseDown`, which then blocked the focus trigger from reopening it — the pointer had to leave and come back. `mode="click"` fixes it without touching the design system: tippy toggles a click-triggered instance, so the button opens and closes it, Enter/Space work because a native button activation dispatches a click, and Escape still closes via the `hideOnEsc` plugin with a click able to reopen it. Under `hover` that last one was a dead end — Escape hid the popover and no new focus event could follow. The trade is that hovering no longer reveals the tags; a disclosure that answers to the keyboard is worth more than a hover shortcut. That click also had to stop propagating: the button sits inside the `<tr>` that `OcTable` highlights on click, so opening the popover selected the file as well. tippy's listener is on the same element, so stopping propagation still toggles the popover. `is-nested` goes with it. It was set purely to suppress the `hideAll` that ran when a pointer crossed the button, and pointer movement no longer opens anything. Dropping it also un-silences `showDrop`/`hideDrop`, which `OcDrop` only emits for drops that are not nested. **The button claimed to be a popover trigger without saying so.** It now carries `aria-haspopup` and an `aria-expanded` driven by those two events. tippy manages `aria-expanded` itself only when the reference does not already have one (`hasAriaExpanded` is read once at create time), and Vue renders the attribute before `OcDrop` mounts, so there is one owner, not two. **The accessible name dropped the visible text.** WCAG 2.5.3 Label in Name wants the visible `+ 3` inside the name; `Show 3 more tags` left speech input with nothing to match. The count now leads the string. **The read-only tag lock was invisible to screen readers.** No `accessible-label` means `OcIcon` renders it `aria-hidden`, so the only signal that a tag cannot be removed was the absent deselect button. **Themes inherited no tag palette.** `tagColorsList` was missing from `defaults.designTokens`, so a deployment serving its own theme.json gets grey chips. Seeding the defaults with the light palette turned out to be the wrong place for it: every other value in that block is an empty-string placeholder, because supplying colours is a theme's responsibility. The defaults stay colourless. `useTagColor` already returns -1 without a list and `OcTag` then renders the plain chip, which is what tags looked like before this feature.
Five review comments landed on #12892 after it merged. **The tag overflow popover dismissed itself on click.** `mode="hover"` maps to tippy's `mouseenter focus` trigger, and `hideOnClick` is hardcoded true in `OcDrop`. tippy only skips its document-press hide when the trigger contains `click`, so a click on the button fell through to `hide()` and set `didHideDueToDocumentMouseDown`, which then blocked the focus trigger from reopening it — the pointer had to leave and come back. `mode="click"` fixes it without touching the design system: tippy toggles a click-triggered instance, so the button opens and closes it, Enter/Space work because a native button activation dispatches a click, and Escape still closes via the `hideOnEsc` plugin with a click able to reopen it. Under `hover` that last one was a dead end — Escape hid the popover and no new focus event could follow. The trade is that hovering no longer reveals the tags; a disclosure that answers to the keyboard is worth more than a hover shortcut. That click also had to stop propagating: the button sits inside the `<tr>` that `OcTable` highlights on click, so opening the popover selected the file as well. tippy's listener is on the same element, so stopping propagation still toggles the popover. `is-nested` goes with it. It was set purely to suppress the `hideAll` that ran when a pointer crossed the button, and pointer movement no longer opens anything. Dropping it also un-silences `showDrop`/`hideDrop`, which `OcDrop` only emits for drops that are not nested. **The button claimed to be a popover trigger without saying so.** It now carries `aria-haspopup` and an `aria-expanded` driven by those two events. tippy manages `aria-expanded` itself only when the reference does not already have one (`hasAriaExpanded` is read once at create time), and Vue renders the attribute before `OcDrop` mounts, so there is one owner, not two. **The accessible name dropped the visible text.** WCAG 2.5.3 Label in Name wants the visible `+ 3` inside the name; `Show 3 more tags` left speech input with nothing to match. The count now leads the string. **The read-only tag lock was invisible to screen readers.** No `accessible-label` means `OcIcon` renders it `aria-hidden`, so the only signal that a tag cannot be removed was the absent deselect button. **Themes inherited no tag palette.** `tagColorsList` was missing from `defaults.designTokens`, so a deployment serving its own theme.json gets grey chips. Seeding the defaults with the light palette turned out to be the wrong place for it: every other value in that block is an empty-string placeholder, because supplying colours is a theme's responsibility. The defaults stay colourless. `useTagColor` already returns -1 without a list and `OcTag` then renders the plain chip, which is what tags looked like before this feature.
4 participants
Description
Tag chips were all the same grey, so a file's tags were hard to tell apart at a glance and the same tag looked like nothing in particular across the app.
Each chip is now filled with a colour derived from the tag name:
hash(tag) % lengthpicks a slot in the theme's newdesignTokens.tagColorsList, emitted as--oc-color-tag-N. The colour is derived, never stored, so a tag looks the same for every user on every device with no API, storage or migration change. Different tags may collide on one colour; that is accepted, because a tag's label always renders beside its chip and the colour is reinforcement, never the identifier.Notes on the design:
--oc-color-text-inversegets it backwards in the light theme. The calculation lives inuseTagColor, not inOcTag, because the theme store is a reactive dependency andgetComputedStyleis not — resolving it in the component would leave labels stale after a theme switch. The fill is read straight out oftagColorsListrather than from the emitted CSS var, sincegetHexFromCssVarreports an unresolvable var as#000000and would hand a pale fill an illegible white label.oc-tag-filled, which forces the colours of everything nested inside it down asinherit/currentColor. Those nested elements otherwise repaint themselves — arouter-linktakes the global anchor colour, a rawOcButtonits own swatch,.oc-tag .oc-icon > svga muted fill — which left navy labels and a washed-out close icon on the deep light-theme fills, and labels that were too light on the pale dark-theme ones.OcTagkeeps its default appearance. It is the generic badge (the app-store "most recent" pill, instance badges, the disabled-space indicator, the+Ncounter), sofillColorandlabelColorare both opt-in.initializeThemesnow merges arrays with replace semantics. deepmerge concatenates by default, which would turn atagColorsListpresent in bothdefaultsand a theme into 60 entries and shift every hue.OcDropis a fixed 300px wide, which left it nearly empty when it held a single chip capped at 80px, with the tags column's right alignment pushing that chip to the far edge. 300px is kept only as a cap so a long tag still truncates.Important
The palettes in this PR are placeholders. Both
theme.jsoncopies gain 30 fills per theme so the feature can be exercised end to end while design's real values are outstanding. They were generated by binary-searching HSL lightness for a target relative luminance per hue, since equal-lightness hues differ a lot in perceived brightness: deep fills with white labels in the light themes, pale fills with black labels in the dark ones. Worst-case label contrast is 6.31:1 (light) and 8.52:1 (dark), higher in both high-contrast themes. They need swapping for design's palette before merge.Both
theme.jsoncopies are needed:web/packages/web-runtime/themes/…is whatpnpm buildpublishes toweb/dist/themes, while an ocis binary serves thego:embed-edservices/web/assets/themes/…copy instead (services/web/pkg/server/http/server.golayers it over the embedded core assets). Every theme in a file carries the same list length, so a tag keeps its colour slot across a theme switch.Related Issue
Motivation and Context
Tags carry no visual distinction today, so scanning a file list for a particular tag means reading every label. Colouring the chips makes a tag recognisable at a glance and consistent wherever it appears, with nothing persisted and no schema change — tag colour is a pure function of the tag string.
How Has This Been Tested?
docker compose upwithowncloud/ocis:dev+pnpm vite), Chrome, light and dark themes, in both regular and high-contrast variantsweb-pkg,design-systemandweb-app-files, including new tests for the hash → index mapping (pinned to known-good literals),useTagColor's fill/label pairing,OcTag's opt-in fill, the files-list chips and the overflow popover.resource-table-tag-overflow.oc-drop(0,2,0) beats.oc-drop(0,1,0), and theoc-tag-filledrules each win on specificity alone rather than by stylesheet orderScreenshots (if appropriate):
To follow — the palette is a placeholder, so screenshots would show colours that are not the final ones.
Types of changes
Checklist: