Page MenuHomePhabricator

CSS-only Checkbox, Radio, ToggleSwitch should consistently set cdx-label class on their <label>
Closed, ResolvedPublic

Description

https://gerrit.wikimedia.org/r/c/design/codex/+/940257 updated the Checkbox, Radio and ToggleSwitch components to use the Label component internally. This means the Vue versions of these components always use a structure where the a <div class="cdx-label cdx-checkbox__label"> contains a <label class="cdx-label__label">.

However, the CSS-only versions of these components are messier. The documentation examples with complex labels do use this structure, but the basic examples use the old structure, with just a <label class="cdx-checkbox__label">. This means that, for CSS-only Checkboxes (and Radios and ToggleSwitches), the .cdx-checkbox__label element could either be a <label> with no other classes, or it could be a <div> that also has a cdx-label class which contains a <label>. This inconsistency makes it easy to accidentally break the CSS-only version when changing the styles: for example, the current iteration of https://gerrit.wikimedia.org/r/c/design/codex/+/980457 tries to change a &__label selector to &__label.cdx-label, which works for the Vue version and for the complex CSS-only versions, but breaks the simple CSS-only version.

We should decide whether this inconsistency is a feature (simpler markup for CSS-only components) or a bug (inconsistency that can be confusing for Codex devs) and make changes accordingly:

  • If we decide this is a feature, we should make sure it's documented properly:
    1. Change the &__label selectors to &__label, &__label.cdx-label so that it they are more specific than Label's styles, but also still apply to the simple CSS-only versions.
    2. Add a comment explaining what's going on, either above these selectors, or in the template (something like <!-- In CSS-only components this can be <label class="cdx-checkbox__label"> instead -->), or both.
  • If we decide this is a bug, we should fix it carefully, to avoid a breaking change:
    1. Update the CSS-only documentation for these components to always use a full Label component (<div class="cdx-label"> containing a <label class="cdx-label__label">).
    2. For backwards compatibility (to ensure existing uses of the old HTML still work), combine the old and new selectors: instead of changing &__label to &__label.cdx-label, change it to &__label, &__label.cdx-label. This way the styles will work with both the old and the new CSS-only component markup.
    3. Add a comment above these selectors explaining this. Ideally we'd do this in a way that's easy to track, so that we can remember to remove the extra selector when we do a 2.0 release in the future. Maybe we could standardize on the DEPRECATED: prefix for this purpose, e.g. // DEPRECATED: Allow CSS-only Checkboxes not to set class="cdx-label".
    4. Update the CSS-only component demos in the sandbox and CodexExample to use the new markup.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

I designed it this way originally for 2 reasons:

  • As you mentioned, because it prevents users from having to add extra classes and markup when using the CSS-only version of these components
  • Because there are already some uses of the CSS-only Checkbox out there (Popups, ULS) and I wanted to avoid a breaking change

To the first point, the old version of the markup avoids the following:

  • An additional div wrapping the label
  • Introduction of several cdx-label classes

This isn't that bad, but it could increase confusion and opportunities for error. That said, as we know, the code has become so complex that we ran into some gnarly issues when updating it. If we do go for option 2, I think we should do the deprecating change, update the 2 files I posted and VueTest/CodexExample to use the new markup, then make a breaking change in the next release - this will help us avoid confusion in the future.

Small note while touching upon this in T355081: From a code readability perspective <div class="cdx-label cdx-checkbox__label"> is giving me a stomach ache. So we have a top-level block cdx-label besides a child element cdx-checkbox__label.

CCiufo-WMF subscribed.

Moving to the backlog for now as in discussion with @Catrope we triaged this as medium-to-low priority tech debt that needs more refinement. If the concerns continue to bother us we can resurface the task.

It gets worse: the cdx-checkbox__label span has display: inline-flex, which means that mixing spaces and links breaks. For example, if you set the label of a CSS-only checkbox to foo <a href="#">(bar)</a> baz, the rendering looks like foo(bar)baz because the spaces between the text and the link get swallowed by flexbox.

The real Label component fixes that by wrapping the text in another span, but the CSS-only checkbox doesn't do this, leading to this display bug. This affects the CodexHTMLForm versions of Checkbox and Radio as well (that's how I found it).

Change #1027206 had a related patch set uploaded (by Catrope; author: Catrope):

[mediawiki/core@master] [WIP] CodexHTMLForm: Wrap radio/checkbox labels in full Label components

https://gerrit.wikimedia.org/r/1027206

Change #1028262 had a related patch set uploaded (by Catrope; author: Catrope):

[design/codex@main] Checkbox, Radio, ToggleSwitch: Always use Label component in CSS-only version

https://gerrit.wikimedia.org/r/1028262

Change #1028262 merged by jenkins-bot:

[design/codex@main] [DEPRECATING CHANGE] Checkbox, Radio, ToggleSwitch: Always use Label component in CSS-only version

https://gerrit.wikimedia.org/r/1028262

Change #1032095 had a related patch set uploaded (by VolkerE; author: VolkerE):

[mediawiki/core@master] Update Codex from 1.5.0 to 1.6.0

https://gerrit.wikimedia.org/r/1032095

Change #1032095 merged by jenkins-bot:

[mediawiki/core@master] Update Codex from 1.5.0 to 1.6.0

https://gerrit.wikimedia.org/r/1032095