Skip to content

Fix dark/light icon variant for External links in the Settings menu - #1130

Open
cookie-monster1649 wants to merge 1 commit into
nextcloud:masterfrom
cookie-monster1649:fix/settings-menu-dark-icon-variant
Open

Fix dark/light icon variant for External links in the Settings menu#1130
cookie-monster1649 wants to merge 1 commit into
nextcloud:masterfrom
cookie-monster1649:fix/settings-menu-dark-icon-variant

Conversation

@cookie-monster1649

Copy link
Copy Markdown

Problem

External links configured with position settings used the wrong icon
variant in Nextcloud's account and Settings menus.

For a paired icon such as audiobookshelf.svg / audiobookshelf-dark.svg,
the base filename audiobookshelf.svg was always registered. Nextcloud's
Settings menu then applies its own theme-dependent filter:

filter: var(--background-invert-if-dark);

In the dark theme this inverts the source icon. Because the source was
already the light-coloured icon, it was inverted to black and became hard
or impossible to see against the dark menu background.

Root cause

External registers the same navigation entry in two places:

  • Application::registerSites()
  • BeforeTemplateRenderedListener::generateNavigationLinks()

Both supplied the configured base filename for Settings entries, so a fix
in only one file had no visible effect.

Change

For entries of type SitesManager::TYPE_SETTING, the registration code now:

  1. Reads the configured icon filename (or external.svg).
  2. Derives the paired dark filename by inserting -dark before the extension.
  3. Uses it only if that file exists in the app data icon directory.
  4. Falls back to the configured icon otherwise.
  5. Skips derivation if the admin already chose a -dark. icon (avoids foo-dark-dark.svg).

Header links, guest/login links, public footer links and quota icons are
unchanged. The same block is applied in both registration paths.

Why this works

-dark is the dark-coloured source asset. In the Settings menu the light
theme shows it unchanged, and the dark theme applies core's existing
inversion to produce a light icon — matching core's rendering rather than
adding theme detection inside External.

Scope / non-goals

Does not change header, footer, quota, mobile/desktop client icon selection,
stored site config, or icon upload. Full-colour (non-monochrome) icons can
still be affected by core's inversion; this follows the app's existing paired
monochrome-icon convention and does not claim to solve arbitrary coloured icons.

Follow-ups (not in this PR, to keep it targeted)

  • Extract the selection into a shared method/service so the two paths can't diverge.
  • Read the icon directory listing at most once per request.
  • Unit tests for the pairing/fallback cases.

Standards checked

  • DCO sign-off added to the commit
  • Coding standard (php-cs-fixer, Nextcloud ruleset) — ran locally, no violations
  • PHP lint (php -l) passes on both changed files
  • REUSE / license headers — no new files added, existing headers untouched
  • info.xml lint — file not touched
  • Psalm — not run locally (separate toolchain + baseline); change is trivially typed, will be verified by CI
  • PHPUnit — no test changes; no existing unit tests exercise this path

Testing

  • Light theme shows the dark-coloured icon correctly.
  • Dark theme shows a light icon after core's CSS inversion.
  • Account menu receives /apps/external/icons/audiobookshelf-dark.svg instead of audiobookshelf.svg.
  • Header / non-Settings entries unchanged; falls back to base icon when no -dark pair exists.
  • php -l and php-cs-fixer pass on both changed files.

This change was AI-assisted and validated on Nextcloud 34.0.1.

Fixes#430

External links with position `settings` used the base (light) icon in
Nextcloud's account/Settings menus. Core recolours those entries with the
`--background-invert-if-dark` filter, which expects a dark-coloured source:
in the dark theme the light base icon was inverted to black and became
invisible.
For Settings-menu entries, prefer the paired `-dark` icon variant (e.g.
`foo-dark.svg` for `foo.svg`) when such a file exists in app data, falling
back to the configured icon otherwise. Header links, guest/login links,
public footer links and quota icons are unchanged.
The same selection is applied in both navigation registration paths
(Application::registerSites and BeforeTemplateRenderedListener), since both
register the same navigation entry; changing only one had no visible effect.
Fixes: nextcloud#430
This change was AI-assisted and validated on Nextcloud 34.0.1.
Signed-off-by: cookie-monster1649 <jj@monester.au>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong dark/bright icon version on settings

1 participant

@cookie-monster1649