Inappropriate trailing colons in Claro and Umami

I know that classy and stable have been frozen, but they have a bug. Hence this issue to highlight and hopefully correct this. Thanks @joelpittet for the initial creation of this issue.

The problem is that some classes in the code no longer match the styles that are used in the CSS. The problem is in classy/css/components/container-inline.css:

.container-inline label:after,
.container-inline .label:after {
  content: ':';
}
.form-type-radios .container-inline label:after {
  content: '';
}

The problem is that .form-type-radios does not exist. It is either .form-type-radio (or the parent class .form-radios). This results in the trailing colons not getting removed from inline radio's, leading to a bad user experience:

Because of the non-existing .form-type-radios, the following bit of CSS also no longer applies and as such is useless. Moreover this only worked for LTR languages. For RTL languages the indentaion was in the wrong direction. This has been resolved in seven per request of lauriii in #2236789-91: Styling of inline radios broken: inappropriate trailing colons (breaks EditorImageDialog). Since it is not used I believe this part can be removed from classy:

.form-type-radios .container-inline .form-type-radio {
  margin: 0 1em;
}

Finally there is an issue with radio buttons no longer aligning with their labels in RTL languages as can be seen below:

In order to overcome this, the container-inline div and label should be inline-blocks rather than inline elements. This too has been resolved in #2236789: Styling of inline radios broken: inappropriate trailing colons (breaks EditorImageDialog) (and I can imagine this beging a potential disruptive change) . But it can be resolved by adding this to classy:

.container-inline div,
.container-inline label {
  display: inline-block;
}

Or by changing the existing definition in stable/css/system/components/container-inline.module.css:

.container-inline div,
.container-inline label {
-  display: inline;
+  display: inline-block;
}

I'd be happy to provide patches for the indicated bugs, but would like some feedback on the desired approach. The first bug would be the least disruptive change and would restore desired (and probably previous) behavior.
The second piece of code does not do anything and will hopefully be in seven when #2236789: Styling of inline radios broken: inappropriate trailing colons (breaks EditorImageDialog) lands. It can be removed, but it can be left doing nothing as well.
The 3rd piece of code is a requirement for correct handling of RTL radio's and labels but has the potential to break existing themes. What is more important, a good UI, or the risk of affecting one or more themes?

I guess this status is most applicable?

@neograph734 - excellent analysis :) I am running into this as well (Drupal 8 system classy theme forces colons on container-inline radio labels where they do not belong) and would like to help work toward a fix. Is it best to -remove- the cited container-inline :after lines or override with radio-specificity to undo it?

Partial patch per #2, addresses inappropriately added colons on container-inline radios and checkboxes and provides the required whitespace as Classy had originally intended, before the radios and checkboxes wrappers selectors had changed in 8.5.x core. I didn't look at anything else like ltr/rtl that Neograph734 mentioned in #2.

I manually tested by making Bartik the admin theme and opened image dialog in CKEditor.
The selector

+++ b/core/themes/classy/css/components/container-inline.css
@@ -7,12 +7,12 @@
+.container-inline.form-radios label:after,

does not reach the radio label. There is no instance of .container-inline.form-radios
It will work with this selector .container-inline.form-radio ~ label:after, but there may be a more elegant way to accomplish this.

I'll keep an eye on this issue to review and RTBC as we're in the process of removing all core theme dependencies on Classy in #3050389: [META] Remove dependency to Classy from core themes. This includes copying many Classy assets to those themes, and I'd like to avoid copying ones with known bugs🙂.

I have reroll the patch and make changes as per above comment.

Reroll the patch again.

Fixing test case review the patch.

I was about to RTBC this, but then in a code review I ran into the following:

.form-type-radios .container-inline label:after,
.form-type-checkboxes .container-inline label:after {
  content: "";
}
.form-type-radios .container-inline .form-type-radio,
.form-type-checkboxes .container-inline .form-type-checkbox {
  margin: 0 1em;
}

The margin also suffers form the exact same mixup of classes. Only making the colons work and leaving the other wrong CSS in place feels wrong. So we should either fix the margin too, or remove it entirely.

But when I was testing this patch with different themes I noticed that the Seven theme gives a much better user experience than applying a margin on both sides.

That theme uses slightly different margins:

.container-inline .form-radios .form-type-radio,
.container-inline .form-checkboxes .form-type-checkbox {
  margin-right: 1em;
}
[dir="rtl"] .container-inline .form-radios .form-type-radio,
[dir="rtl"] .container-inline .form-checkboxes .form-type-checkbox {
  margin-right: 0;
  margin-left: 1em;
}

I have added that to the patch. I know it goes somewhat outside the scope of the title, but since the margin code is already there and suffers from the same mixup, I believe this has always been the intended behavior. If not desired, we should at least delete the margin: 0 1em, as currently these margins are not doing anything.

What are the steps to replicate this?

Also seven + bartik have been removed in D10 so would need to determine if this is a problem in claro + olivero

The same CSS is included in Claro, but I'm not able to trigger the issue described without steps to reproduce. I've never seen this issue before in the wild either.

Leaning toward closing without further info?

Confirmed this is still happening in Claro/Umami (did not test Olivero). I added steps to reproduce in the IS. The only core instance of this seems to be in deprecated CKEditor 4 related forms, but this issue should remain open since .container-inline should fully support all core form elements, even if they don't happen to appear in an inline form.

This requires some extra steps to reproduce so I'm not sure what kind of momentum this issue will have going forward, but the problem still exists

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Made an updated MR with the changes from #2912824-22: Inappropriate trailing colons in Claro and Umami just applied to the current core themes.

Wonder if this will require a simple test case to check if the colon is there or not. Or is that overkill?

Posted to #bugsmash and @mstrelan commented

The colon is never really there. I'm not sure if we can even check pseudo-elements? Either way I think PHP should trust that CSS is doing CSS things, it would be more appropriate to check that PHP / twig is outputting the appropriate markup so CSS can style it accordingly. That may already exist?

So since this is pure css don't think we need tests then.

But there was a CI failure in the MR.

Hiding the files as the work is being done in the MR now.

Oops! Fixed.

Thanks!

Changes requested in MR.

Also, lets celebrate that this is the rare occasion where I'm tagging "needs screenshots" instead of wearily pointing out there are too many of them. There should be shots for Claro and Umami with checkboxes and radios.

Please review.

Threads appear to be answered

Moving to NW per #39 for screenshots and issue summary update please.

I mentioned earlier that the Claro solution was using selectors that don't exist in Claro. It looks like the Claro changes were removed entirely, when the solution should be changing them to the correct selector.

Because the merge request is currently not mergeable I've created a new patch that applies to D10.1.x