Skip to content

fix: accessibility for apperance#62489

Open
kristian-zendato wants to merge 1 commit into
masterfrom
fix/accessibility-for-apperance-and-accessibility
Open

fix: accessibility for apperance#62489
kristian-zendato wants to merge 1 commit into
masterfrom
fix/accessibility-for-apperance-and-accessibility

Conversation

@kristian-zendato

@kristian-zendato kristian-zendato commented Jul 24, 2026

Copy link
Copy Markdown

Summary

  • Give theme preview images meaningful alternative text instead of empty alt.
  • Present background options as a single HTML list while keeping the layout (custom / plain color / default on the first row, shipped backgrounds on the next).
  • Make each theme option a focusable control that activates the theme on click/keyboard, and keep the visual radio/checkbox out of the tab order (tabindex="-1").

Checklist

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@kristian-zendato
kristian-zendato requested a review from a team as a code owner July 24, 2026 18:01
@kristian-zendato
kristian-zendato requested review from nfebe, sorbaugh and susnux and removed request for a team July 24, 2026 18:01
Signed-off-by: Kristian Zendato <kristian.zendato@nextcloud.com>
@kristian-zendato
kristian-zendato force-pushed the fix/accessibility-for-apperance-and-accessibility branch from bdd1aba to cd5393a Compare July 24, 2026 18:40
alt=""
class="theming__preview-image"
:src="imageUrl"
<button

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not valid you cannot have a checkbox inside a button.
This should be just a DIV element and checkbox within already is the semantic input element.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So the outer elements needs to be a div and the outer fieldset needs a role="radiogroup".


<div :class="$style.backgroundSelect">
<!-- Custom background -->
<ul

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why?
This is semantically not really correct, fieldset already was correct.
As this is a group of related input fileds. Its not really a list.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

https://github.com/nextcloud-gmbh/customer-feature-requests/issues/1624

Related to this feedback, they want the background to be HTML list.

@susnux susnux Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But thats invalid see my feedback on the issue. IMHO a false-positive text and should be closed as wont fix with proper reference to WCAG rules as the current semantics are correct (expect my other comment above).

@susnux susnux added this to the Nextcloud 35 milestone Jul 24, 2026

@susnux susnux left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See my comment - parially the issue is wrong as semantics for WCAG needs radiogroup and radio within the theme selection.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants