Skip to content

icons: Use the theme background for the decoration border - #62

Merged
ccrisan merged 1 commit into
mainfrom
fix/icon-decoration-border-theme-background
Oct 11, 2026
Merged

ccrisan merged 1 commit into
mainfrom
fix/icon-decoration-border-theme-background

Conversation

@ccrisan

@ccrisan ccrisan commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

An icon decoration draws a border so the dot reads as separate from what is behind it. _findBgColor() picked that colour by walking up the ancestors for the first non-transparent background-color.

That walk answers differently depending on when it runs.

IconLabelViewMixin.makeIconLabelContainer() creates the icon element and calls _applyIcon() on it before appending it, so on a first render nothing up the chain has a computed style, the walk falls off the top, and the code uses Theme.getVar('background-color'). On a re-render the element is in the document, the walk succeeds, and it takes whatever is actually behind the icon.

What that looks like

A selected list row, decoration border dark. Change an attribute on the port so its icon changes. The list patches the row in place, setIcon() re-renders onto the same attached element, and the walk now stops at the row itself:

div.qui-icon-decoration            rgba(0, 0, 0, 0)
div.qui-icon                       rgba(0, 0, 0, 0)
div.qui-icon-label-view.selected   rgb(98, 171, 234)   <- stops here

The border turns into the selection highlight and stays that way: nothing renders that icon detached again, so every later update repeats the attached walk and finds the same colour. Deselecting the row does not clear it either — the colour is sampled once and frozen. Only a reload restores it, by rebuilding the rows.

Measured on a running instance, same row, before and after one attribute change:

decoration border
after reload rgb(68, 68, 68)
after an in-place update rgb(98, 171, 234)

I confirmed the row was patched rather than rebuilt by marking the row, icon and decoration elements: all three markers survived the update.

The asymmetry is old. It stayed invisible while every port update rebuilt the whole list, because rebuilt rows always render detached and always took the fallback. In-place row updates are what exposed it.

The change

Use the theme background directly and drop the walk. That is the value the first render already produced, it does not depend on attachment, and it matches what the stylesheet does where the sampled answer was wrong — less/main-ui.less:84 already overrides it with border-color: @background-color !important for top bar icons.

The contrast test is unchanged: a decoration that does not stand out against the page background still falls back to the icon's dominant colour.

_findBgColor() had one caller and is removed.

Checked

On a running instance, with the port toggled offline and back:

decoration border
before rgb(68, 68, 68)
toggled offline rgb(68, 68, 68)
toggled back rgb(68, 68, 68)

The decoration background still tracks the port state (green to grey and back); only the border stops moving.

As a side effect this also removes a getComputedStyle call per icon on the attached path.

Sampling the ancestors for a background answered differently depending
on whether the icon was attached when it rendered: detached it fell back
to the theme background, attached it took whatever sat behind it. A row
patched in place therefore picked up the selection highlight and kept
it.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is small, correct, removes a helper with no remaining references, and preserves the existing fallback behavior with no regressions.

0 open findings

What changed in this PR

This PR fixes a visual bug in icon decoration borders. An icon decoration draws a border so its dot reads as distinct from the background behind it. Previously, _findBgColor() determined that border colour by walking up the DOM ancestors looking for the first non-transparent background-color. Because icons render both detached (first render) and attached (in-place re-render), this walk produced different answers depending on timing — on an in-place row update it could sample a selection highlight and freeze the border in that colour until reload. The fix uses the theme background colour directly, which matches the value the first render already produced and what the stylesheet enforces elsewhere.

Changes:

  • Replace the ancestor background-color walk with Theme.getVar('background-color') for the decoration border's contrast check.
  • Remove the now-unused _findBgColor() helper (its single caller is updated).
  • Add an explanatory comment documenting why the theme background is used instead of sampling ancestors.
File Description
js/​icons/​multi-state-sprites-icon.js Uses the theme background colour for the decoration border instead of walking ancestors, and removes the unused _findBgColor() helper.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@ccrisan
ccrisan merged commit b2f3365 into main Oct 11, 2026
5 checks passed
@ccrisan
ccrisan deleted the fix/icon-decoration-border-theme-background branch October 11, 2026 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants