Skip to content

icons: Apply the decoration border color the icon lookup finds - #61

Merged
ccrisan merged 1 commit into
mainfrom
fix/icon-decoration-border-color
Sep 28, 2026
Merged

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

Conversation

@ccrisan

@ccrisan ccrisan commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Stacked on #60, which kept this behaviour as it was so that it could be fixed on its own.

The bug

An icon decoration picks a border colour to separate it from what is behind it. When the decoration contrasts well with the background it borrows the background colour; otherwise it asks _findIconColor() for the icon's dominant colour, which is resolved asynchronously off a canvas.

Before #60 the styles were collected into an object and applied at the end:

else {
    this._findIconColor(element).then(function (color) {
        css['border-color'] = color
    })
}

decorationDiv.css(css)

.css(css) runs synchronously; the callback runs later. It wrote into an object nobody looked at again, so in that branch the border colour never reached the element. #60 kept that as-is, with a comment. This assigns to the element instead.

Shown on a device

The branch is not reachable on the port list, where the contrast test always goes the other way, so I forced it with a temporary build and loaded the same 448-port list. Same page, same icons, only this commit differing:

decorations inline border-color
without this commit 236 (none) on all 236
with this commit 236 rgb(98, 171, 234) on all 236

So the lookup resolves, and the value now lands. The temporary build was only for the test; nothing of it is in this PR.

Cost

None. _findIconColor() was already called and already did its canvas work in this branch — decode the sprite sheet, sample every pixel, count colours. Only the answer was thrown away.

Worth knowing

_findIconColor() resolves from img.onload and has no onerror, so if the sprite ever fails to load the promise never settles. That is unchanged here, but it now means a missing border rather than a discarded value.

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.

Copilot review overview

🟡 Changes recommended

The new async then(...) write can race with subsequent renderTo() calls and overwrite a newer border color unless guarded (e.g., with a per-element request id).

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Fixes icon decoration border rendering when the decoration/background contrast is low by applying the asynchronously computed dominant icon color directly to the decoration element instead of mutating a no-longer-used style object.

Changes:

  • Apply borderColor directly on the decoration element inside the _findIconColor() promise resolution.
  • Continue the jQuery-to-DOM refactor in renderTo() by using classList, children iteration, and direct style assignments.
File Description
js/​icons/​multi-state-sprites-icon.js Applies async-resolved border color directly to the decoration element during render.

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

Comment thread js/icons/multi-state-sprites-icon.js Resolved
Base automatically changed from perf/icon-render-classes to main September 28, 2026 19:45
The callback wrote into a style object that had already been applied, so
in the low-contrast branch the border color never reached the element.
The canvas work behind that lookup was already being done; only its
result was dropped.

The element outlives a render, so each one stamps it with a token and a
lookup resolving after a newer render is ignored.
@ccrisan
ccrisan force-pushed the fix/icon-decoration-border-color branch from ee89e91 to c808d95 Compare September 28, 2026 19:49
@ccrisan
ccrisan requested a lite review from Copilot September 28, 2026 19:52

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.

Copilot review overview

🟢 Approval recommended

The change directly fixes the described async style-application bug and includes a verified guard against stale promise resolutions overwriting newer renders.

Review effort: Lite
Findings: None

Resolved since last review (1)

@ccrisan
ccrisan merged commit 633cce5 into main Sep 28, 2026
5 checks passed
@ccrisan
ccrisan deleted the fix/icon-decoration-border-color branch September 28, 2026 19:54
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