Problem/Motivation

While working on #2039163: Update CKEditor library to 4.4, I ran into a problem I'd been seeing for weeks: messed up styling in EditorImageDialog's "align" radios. They were no longer displayed on the same line as the "Align" label, and had trailing colons.

So I dug in.

This is a regression caused by #2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes. That patch completely changed the HTML structure of '#type' => 'radios', which breaks several selectors. At least the following selectors in system.theme.css are broken:

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

The <legend> also breaks the "general inlineness".

Possibly more selectors are broken than just these.

Proposed resolution

TBD, but it looks like this will be hard, because apparently (some) browsers don't allow a <legend> to be displayed inline: http://stackoverflow.com/questions/5818960/legend-not-displaying-inline.

Remaining tasks

TBD

User interface changes

Fix the regression.

API changes

None.

Comparison

Original

Drupal at commit 577a41d473afcb8276ddb8ead0ba6142199a9697. Try it at simplytest.me: http://simplytest.me/project/drupal/577a41d473afcb8276ddb8ead0ba6142199a....

<div class="container-inline form-item form-type-radios form-item-attributes-data-align">
 <label for="edit-attributes-data-align">Align</label> <div id="edit-attributes-data-align" class="form-radios container-inline"><div class="form-item form-type-radio form-item-attributes-data-align">
 <input class="container-inline form-radio" type="radio" id="edit-attributes-data-align-none" name="attributes[data_align]" value="none" checked="checked"> <label class="option" for="edit-attributes-data-align-none">None</label>
</div>
<div class="form-item form-type-radio form-item-attributes-data-align">
 <input class="container-inline form-radio" type="radio" id="edit-attributes-data-align-left" name="attributes[data_align]" value="left"> <label class="option" for="edit-attributes-data-align-left">Left</label>
</div>
<div class="form-item form-type-radio form-item-attributes-data-align">
 <input class="container-inline form-radio" type="radio" id="edit-attributes-data-align-center" name="attributes[data_align]" value="center"> <label class="option" for="edit-attributes-data-align-center">Center</label>
</div>
<div class="form-item form-type-radio form-item-attributes-data-align">
 <input class="container-inline form-radio" type="radio" id="edit-attributes-data-align-right" name="attributes[data_align]" value="right"> <label class="option" for="edit-attributes-data-align-right">Right</label>
</div>
</div>
</div>

Current

Drupal at commit 48ae9707f43bd36b305f97abd87d21a85c104d67. Introduced in 2c7b99d09c4b1d7811f8ad01584cc75813c259dc. #2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes. By trying Drupal at one commit before that one (35c3429c53bdf9f92e575aa12bbefa8997bff96c), you can see that all was well until that very commit.

<fieldset class="container-inline fieldgroup form-composite form-wrapper form-item" id="edit-attributes-data-align">
      <legend><span class="fieldset-legend">Align</span></legend>  <div class="fieldset-wrapper">
        <div id="edit-attributes-data-align" class="form-radios container-inline fieldgroup form-composite"><div class="form-item form-type-radio form-item-attributes-data-align">
      <input class="container-inline form-radio" type="radio" id="edit-attributes-data-align-none" name="attributes[data-align]" value="none">
        <label class="option" for="edit-attributes-data-align-none">None</label>
    </div>
<div class="form-item form-type-radio form-item-attributes-data-align">
      <input class="container-inline form-radio" type="radio" id="edit-attributes-data-align-left" name="attributes[data-align]" value="left">
        <label class="option" for="edit-attributes-data-align-left">Left</label>
    </div>
<div class="form-item form-type-radio form-item-attributes-data-align">
      <input class="container-inline form-radio" type="radio" id="edit-attributes-data-align-center" name="attributes[data-align]" value="center" checked="checked">
        <label class="option" for="edit-attributes-data-align-center">Center</label>
    </div>
<div class="form-item form-type-radio form-item-attributes-data-align">
      <input class="container-inline form-radio" type="radio" id="edit-attributes-data-align-right" name="attributes[data-align]" value="right">
        <label class="option" for="edit-attributes-data-align-right">Right</label>
    </div>
</div>

          </div>
</fieldset>

Remaining tasks

See #61.

CommentFileSizeAuthor
#103 2236789-103.patch1.74 KBNeograph734
#96 radio.png112.16 KBckrina
#96 radio-rtl.png135.27 KBckrina
#92 2236789-92.patch1.49 KBjoelpittet
#92 interdiff.txt2.81 KBjoelpittet
#89 Banners_and_Alerts_and_Create_Article___Site-Install_-_RTL.png65.5 KBjoelpittet
#89 Create_Article___Site-Install_-_LTR.png77.18 KBjoelpittet
#86 2236789-86.patch1.52 KBNeograph734
#80 2236789-80-rtl-expected.png10.83 KBNeograph734
#74 2236789-74-rtl-after.png11.6 KBidebr
#74 2236789-74-after.png11.51 KBidebr
#74 2236789-74-rtl-before.png11.75 KBidebr
#74 2236789-74-before.png12.6 KBidebr
#74 2236789-74.patch827 bytesidebr
#61 search.png4.64 KBxjm
#61 views_ui.png2.76 KBxjm
#61 Comments___Site-Install.png9.24 KBxjm
#61 date_inline.png4.76 KBxjm
#61 radio_button_mockup.jpg23.65 KBxjm
#55 styling_of_inline-2236789-55.patch2.04 KBLewisNyman
#55 interdiff.txt464 bytesLewisNyman
#50 modal-broken-inline-styling-2236789-50.patch2.04 KBherom
#50 interdiff-2236789-39-50.txt1.13 KBherom
#50 rtl-after.png21.8 KBherom
#50 rtl-now.png21.55 KBherom
#50 rtl-before.png21.94 KBherom
#42 radio buttons - with patch.png70.19 KBmeenakshi.r
#42 radio buttons - without patch.png69.84 KBmeenakshi.r
#40 radio_buttons_after.png22.85 KBManjit.Singh
#40 radio_buttons_before.png20.48 KBManjit.Singh
#39 modal-broken-inline-styling-2236789-39.patch1.7 KBsingh_haneet
#37 radio_buttons.png33.17 KBManjit.Singh
#36 modal-broken-inline-styling-2236789-36.patch1.57 KBsingh_haneet
#35 modal-broken-inline-styling-2236789-35.patch1.57 KBsingh_haneet
#34 Screenshot 2015-10-11 21.39.13.jpg62.46 KBLewisNyman
#33 modal-broken-inline-styling-2236789-33.patch1.35 KBsingh_haneet
#29 modal-broken-inline-styling-2236789-29.patch594 bytessingh_haneet
#25 patch.png129.17 KBTruptti
#25 Before_patch.JPG286.64 KBTruptti
#25 After_patch.JPG280.67 KBTruptti
#23 modal-broken-inline-styling-2236789-23.patch669 bytessingh_haneet
#23 2236789-23.png128.15 KBsingh_haneet
#17 dialog_form_image_align-2236789-17.patch691 bytesdpetruk
#16 dialog_form_image_align-2236789-16.patch631 bytesdpetruk
align-radios-current.png212.24 KBWim Leers
align-radios-original.png226.08 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Title: Styling of inline radios broken: inappropriate trailing colons » Styling of inline radios broken: inappropriate trailing colons (breaks EditorImageDialog)
Issue tags: +Usability

Clarifying title. This is crucial for the UX of the EditorImageDialog.

Wim Leers’s picture

Issue tags: +CSS, +Novice, +CSS novice
Wim Leers’s picture

Issue tags: -Novice, -CSS novice

Oops, sorry, probably not a novice issue.

mgifford’s picture

I'm not sure how to help here Wim. It seems like this is a known browser issue.

I can confirm that this is an issue in FireFox & Chrome on the Mac.

Other than the issue you raised, I think this is the closest I could find that is related:
https://bugzilla.mozilla.org/show_bug.cgi?id=653870

In looking at:

I couldn't find anything else related.

It is a requirement of WCAG to wrap these in a fieldset. We could opt out for this instance if required. I'll ask to see if there are other solutions.

sun’s picture

We only ever had problems with styling fieldset legends in a way to "detach" them from the fieldset (so they appear like a summary of a details element). That's why we converted fieldsets to details. :-)

I wasn't aware of this "inline radios" use-case, but that is certainly possible to resolve.

http://jsfiddle.net/sun/m6FYT/ shows the gist.

Tested + works in Firefox 27, Chrome 34, and IE9.

mgifford’s picture

Thanks @sun I'd heard that float: left; would work but hadn't tried it yet. This would need to be adjusted for RTL too, but that's easy...

YesCT’s picture

LewisNyman’s picture

Issue summary: View changes
Issue tags: +frontend
Wim Leers’s picture

Others are now also noticing the brokenness; see #2271791: Image alignment radio buttons are confusing, which I've just closed as a duplicate.

Please fix this.

catch’s picture

What's the issue that introduced the regession? The reference in the issue summary points to #2039163: Update CKEditor library to 4.4 as both where the error was discovered and introduced.

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

#10: Sorry about that, that was a copy/paste error. The issue that caused the regression was amongst the related issues already; it's #2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes.

cilefen’s picture

Issue summary: View changes

I just checked—this is still broken.

dpetruk’s picture

Assigned: Unassigned » dpetruk
dpetruk’s picture

Issue tags: +dcuacs2015
dpetruk’s picture

Priority: Major » Minor
Status: Active » Needs review
FileSize
631 bytes

Hi, all.

I propose to change the task status. Because, as for me, it is no so major task)

Also I've attached patch where I made some changes to fix this issue. But this is make the other same legend tags.
So this fix makes change view in "Image size" field.
Please, share your opinion)

dpetruk’s picture

Another variant to fix this issue.

I know this code doesn't look nice, but it works)

lauriii’s picture

Priority: Minor » Normal
eleleka’s picture

#16 Applying this patch, we probably need to increase popup max-width up to 520px as it affect on "Images size" field as well. This will prevent line break for "pixels" word in Images size field.

Wim Leers’s picture

Status: Needs review » Needs work

We shouldn't fix this specifically for this form, we should solve it generically, i.e. for any place that uses this. So, we should use .container-inline in the selector. See e.g. #5 for a more generic solution.

mgifford’s picture

Assigned: dpetruk » Unassigned

@Wim Leers, would adding the container-inline css address @eleleka concerns with the max width?

Just posting in @sun's CSS code from #5.

/* Current system.module.css */
.container-inline div,
.container-inline label {
  display: inline;
}
.container-inline .details-wrapper {
  display: block;
}
/* New/additional */
fieldset.container-inline {
    border: 0;
}
fieldset.container-inline > legend {
    float: left;
    margin: 0;
    padding: 0;
    font-weight: bold;
}
fieldset.container-inline > legend:after {
    content: ':';
}
Wim Leers’s picture

Issue tags: +Novice, +CSS novice

The "images size" form items are gone. But my concerns/points in #20 still stand, and there I already referred to #5, so, yes, let's do that.

We should really fix this before release.

singh_haneet’s picture

Status: Needs work » Needs review
FileSize
128.15 KB
669 bytes

Hi all,

I applied the following changes:

1. Aligned the legend to left as per the discussion.
2. Removed the trailing colons from the radios.

Wim Leers’s picture

WOOOT! Thanks :)

Truptti’s picture

FileSize
280.67 KB
286.64 KB
129.17 KB

Verified the patch in #23, the radio buttons on Insert Image popup are aligned to the left on applying the patch.
Steps:
1.Apply the patch
2.Navigate to Add article page
3.Try to add a image in the body section
4.Verified that radio buttons are aligned to the left.
Snapshots attached for reference

Truptti’s picture

Status: Needs review » Reviewed & tested by the community
LewisNyman’s picture

Status: Reviewed & tested by the community » Needs review

Wim asked me to review this.

LewisNyman’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/css/components/container-inline.module.css
    @@ -11,3 +11,17 @@
    +/* Legends to be inline with content */
    

    Full stop is needed at the end of the comment.

  2. +++ b/core/modules/system/css/components/container-inline.module.css
    @@ -11,3 +11,17 @@
    +.container-inline.form-composite legend {
    ...
    +.container-inline.form-composite .fieldset-legend::after {
    ...
    +.container-inline.form-composite label::after {
    

    Can we remove the container-inline class from this selector?

    Also, can we move this into Classy's form.css file? There is already form-composite styling in there

  3. +++ b/core/modules/system/css/components/container-inline.module.css
    @@ -11,3 +11,17 @@
    +}
    +
    ...
    +}
    +
    

    We can also remove the blank lines in between each group

singh_haneet’s picture

Status: Needs work » Needs review
FileSize
594 bytes

Added new patch. Moved the code to Classy's form.css without using container-inline selector.

a_thakur’s picture

Status: Needs review » Needs work

After applying this patch the radio buttons on the install page are broken, could you please check this? Probably the global effect is breaking things.
Only local images are allowed.

I see similar issue on other pages with radio buttons.

Manjit.Singh’s picture

Assigned: Unassigned » Manjit.Singh

looking into this issue.

LewisNyman’s picture

Ah I see. In this situation, can we add another class to the image alignment radio buttons that is .form-composite--inline and use that to add the styling? I'd rather we didn't combine the container-inline class.

singh_haneet’s picture

Assigned: Manjit.Singh » Unassigned
Status: Needs work » Needs review
FileSize
1.35 KB

Added 'form-composite--inline' class for Editor's image dialog and used it as selector for styling radios so that it doesn't effect globally.

LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
62.46 KB

Great stuff! Thanks.

+++ b/core/themes/classy/css/components/form.css
@@ -53,6 +53,16 @@ label.option {
+  margin-right: 1em;;

A double ;

I noticed that we would need to add a little bit of spacing between the form-item elements to match the original design in the issue summary. Maybe we should wrap that styling in a media query? The spacing would cause the elements to wrap and then it would look weird.

singh_haneet’s picture

Status: Needs work » Needs review
FileSize
1.57 KB

That makes sense to wrap the spacing in a media query, so adding those in this patch. Hope that helps :)

singh_haneet’s picture

Missing ; added in a new patch. Don't know why it was missing earlier.

Manjit.Singh’s picture

FileSize
33.17 KB

Sorry, not get a time to look into this issue.

@singh_haneet thanks for the contribution !! We are almost close enough. there is minor alignment issue. Please check the screenshot. I have tested it on windows (Chrome).

radios

Manjit.Singh’s picture

Status: Needs review » Needs work
singh_haneet’s picture

Status: Needs work » Needs review
FileSize
1.7 KB

New patch added.

Manjit.Singh’s picture

Looks good now.

Before
sdas





After
sdas

Wim Leers’s picture

That looks great! Many thanks :)

If @LewisNyman signs off on the CSS side of things, then it looks like we finally have a fix for this :) :)

meenakshi.r’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
69.84 KB
70.19 KB

It is fixed.

Before patch

After patch

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review

#42:

If @LewisNyman signs off on the CSS side of things, then it looks like we finally have a fix for this :) :)

Thanks for the visual confirmation, now we just need confirmation from a CSS expert.

Bojhan’s picture

Assigned: Unassigned » LewisNyman
Bojhan’s picture

singh_haneet’s picture

@Wim Leers, @LewisNyman I was wondering if we could get this closed before D8 release it would be awesome :)

Wim Leers’s picture

Thanks for reminding us! I pinged LewisNyman on Twitter: https://twitter.com/wimleers/status/662665026084470784.

LewisNyman’s picture

Assigned: LewisNyman » Unassigned
Status: Needs review » Reviewed & tested by the community

The CSS looks good! Thanks

singh_haneet’s picture

Great. Thanks :)

herom’s picture

This also needed RTL styling.
Here are some screenshots:

RTL (before):

RTL (after, #39):

RTL (after, #50):

joelpittet’s picture

Thanks for the screenshots look great, just a review.

  1. +++ b/core/themes/classy/css/components/form.css
    @@ -53,6 +53,44 @@ label.option {
    +  margin-top: 1px;
    

    What is this 1px on the top for?

  2. +++ b/core/themes/classy/css/components/form.css
    @@ -53,6 +53,44 @@ label.option {
    +  content: ':';
    

    We had a bit of a moment with this. I wanted to use it for something else but it turns out it can be translated in french to have a space before the colon. It's a bit of visual fluff and I don't know how many translators are going to go through the effort for that... but thought I'd let you know.

  3. +++ b/core/themes/classy/css/components/form.css
    @@ -53,6 +53,44 @@ label.option {
    +.form-composite--inline input[type="radio"],
    + .form-composite--inline label {
    

    nit: One space at the front of this line. (can be fixed on commit)

xjm’s picture

@Wim Leers, @webchick, and I discussed this and this does not need to be an RC target. It's safe to fix during a patch release.

Wim Leers’s picture

+1

joelpittet’s picture

Status: Reviewed & tested by the community » Needs work

Wonder if someone can address the points in #51? Setting to needs work.

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
464 bytes
2.04 KB
+++ b/core/themes/classy/css/components/form.css
@@ -53,6 +53,44 @@ label.option {
+  margin-top: 1px;

I compared before and after and this is legit. It aligned properly with 1px.

+++ b/core/themes/classy/css/components/form.css
@@ -53,6 +53,44 @@ label.option {
+.form-composite--inline input[type="radio"],
+ .form-composite--inline label {

Fixed.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

It's fine to re-RTBC per your own RTBC conclusion if you're only changing whiespace :)

singh_haneet’s picture

If everything is fine can we move ahead ?

Wim Leers’s picture

See #52. This will be committed in 8.0.1.

singh_haneet’s picture

Okay I missed that earlier. Travel lagging woes ;)

Wim Leers’s picture

np :)

xjm’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup
FileSize
23.65 KB
4.76 KB
9.24 KB
2.76 KB
4.64 KB

Thanks for the thorough manual testing on both LTR and RTL!

Originally we planned to commit this to both 8.0.x and 8.1.x, but @alexpott pointed out to me that adding CSS classes does introduce a theme disruption, and by default, we would commit it to 8.1.x straightaway for release in 8.1.0 but not necessarily to 8.0.1. (So we were possibly wrong in #52.) That said, I'm leaving this against 8.0.x for now, because I'm increasingly not sure this is the right fix, for several reasons.

  1. The point about translatability that @joelpittet mentions in #51. We are hardcoding English punctuation there. In French, this would need to be ' : ' instead of ':'; in Hindi, I believe it would need to be :-.

    This is also an existing problem in HEAD with .container-inline itself, which has:

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

    If that were the only issue I would suggest committing this as-is and having a separate followup for the incompatibility of the CSS with localization. However...

  2. I don't think this is actually the best UI pattern for this form anyway. Having the group legend/description and the individual field labels for radios on the same line is not actually a pattern we use elsewhere in core that I could find. I can see the desire to have the four radios on one line to save space in the modal, but I think it actually makes the form harder to understand for the "Align" to be on the same line as well. That is, I think it should look like this instead:

    The legend is on its own line, as some browsers expect, but the individual buttons and their labels are still together. That would also probably allow us to fix this without adding so much CSS.

  3. The issue summary also suggests that while the CKeditor dialog regression looks weird, it might not be the only example in core that regressed due to #2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes. However, that doesn't seem to have been discussed further on the issue.

For more context, I went looking for other examples of the pattern of an inline label. Neither the UI guidelines for grouping form elements nor the guidelines for radio buttons offer specific advice one way or the other. Most of the examples of container-inline in core still do have the overall label on a separate line:

One place where the label colon is actually used is--guess where!--the Views UI, but in that case it's the only label on the line:

Then in the search form, the colon is added by CSS, but it appears to be bugged so it displays on a separate line anyway, and it is also just the one label:

Based on all that, I think we should keep the legend on its own line for this form, and simply get rid of the buggy/misplaced colon in this markup structure, plus possibly improve the padding. Then, we create a followup to consider removing the pattern in general and leaving it up to individual themes to implement an "inline radio legend" pattern if desired.

joelpittet’s picture

@xjm since it's a theme disruption to commit to classy, can we just copy the fixes to seven and bartik as they are not API?

Re: That annoying ":" translateability I think we should consciously ignore it. It makes more work for translators without giving much in the way of meaning and it's a PITA for us. It's in CSS as "presentational" colon and it would actually be much easier for a translator to provide a French or Hindi translation CSS file to solve all of those in one shot including the ones we missed! Than it would be for a translator to find all the instances and add some space or dashes around it. FWIW

Wim Leers’s picture

Assigned: Unassigned » Bojhan
Status: Needs work » Needs review

#61.1 + #62: the localization of the colon: pre-existing problem, this doesn't change that at all.

#61.2: I disagree that that looks better or is more usable. We decided on that a long, long time ago with careful designing. I'd rather not rehash all that here. This issue is solely about fixing something that worked fine for a long time.

#61.3: As long as we don't have automated visual regression testing of every single UI/form/response, I fear that we will always be at the mercy of just discovering visual regressions on a case-by-case basis.

#61.rest: Most examples of container-inline have multiple form items: two separate inputs, an input and a button, etc. In this case, it's the different radios that all belong together. And yes, this particular instance was highly optimized for usability reasons.


All that being said, I could also live with getting rid of those horrid colons and the horrible lack of spacing between the radios — i.e. as shown in your screenshot. Let's let a usability maintainer decide. To you, Bojhan!

Wim Leers’s picture

(i.e. what I care about most is that this finally gets fixed, after >1.5 year of bikeshedding. The current UI leaves a very bad impression, is hugely distracting.)

emma.maria’s picture

Issue tags: +Needs usability review

I'm adding the @Bojhan bat signal tag also :)

PS. @Wim Leers if there is anything I can help with here, please let me know.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Bojhan’s picture

I am unsure what the actual debate is about? WimLeers could you explain me at Drupalcon.

Are we just talking about showing it "next" to the radio's or above it? I know we did it because we wanted this form to be very compact. In core we have very few good practices about making compact forms, which is why many of our forms look very 2009'.

I am not inclined to drop this compact style, because we don't have it elsewhere in core - we should, but we haven't really tried.

Bojhan’s picture

Issue tags: -Needs usability review
Wim Leers’s picture

Sure, let's talk at DrupalCon. But the issue summary's "original" vs "current" screenshots are still accurate: they still show the problem.

stpaultim’s picture

Issue tags: -Novice, -CSS novice

Removing the "Novice" tag, since there doesn't seem to be anything that a "novice" can do until decisions are made by some non-novices.

Wim Leers’s picture

Issue tags: -dcuacs2015

Bump. Can we please fix this? This has been a regression for more than 2 years now. It'd be great if EditorImageDialog didn't look anymore like it was designed by a developer who'd never seen a UI before.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

idebr’s picture

Considering the changed markup for radios to a fieldset in #2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes, it would make sense to update the original styling to match the new markup because it would fix all instances of this occurrence and not only this particular one.

Keeping in the mind the preference of xjm in #62.2 to keep the legend above the radios, I have updated the original CSS that currently never applies in HEAD.

A follow-up for the correct localization of the colons is available in #2877462: Inline form element colons are incompatible with localization

Container inline before:

Container inline before (rtl):

Container inline after:

Container inline after (rtl):

idebr’s picture

Closed #2880995: UI Clean Up as a duplicate.

Bojhan’s picture

Assigned: Bojhan » Unassigned
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

This looks like a much more elegant solution, thanks! :)

lauriii’s picture

Assigned: Unassigned » yoroy

I think this looks still really confusing to me, especially the RTL version. However, code-wise this looks good. I will let @yoroy to make a decision if the UI is acceptable like this.

Wim Leers’s picture

The only change is the removal of colons. Surely that's a step forward?

Neograph734’s picture

FileSize
10.83 KB

I have little experience with RTL languages, but I'd expect RTL to look like this:

expected RTL

LTR looks much better now.

Wim Leers’s picture

Sure but the margin/padding is a separate problem. This issue is only about the colons.

It's already taken >3 years. Can we please get this in now, and worry about other problems in other issues?

jibran’s picture

yoroy’s picture

Version: 8.3.x-dev » 8.4.x-dev
Assigned: yoroy » Unassigned

But the "after" screenshot for RTL looks worse than the "before", so can we (double) check that this patch does not make things worse there?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 74: 2236789-74.patch, failed testing. View results

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Neograph734’s picture

Status: Needs work » Needs review
FileSize
1.52 KB

The main issue with RTL was that the labels had to be set to 'inline-block' rather than 'inline'. Attached patch results in LTR(after) from #74, and the following RTL:

Only local images are allowed.

@yoroy and Wim Leers, could you have a look?

Neograph734’s picture

Neograph734’s picture

Version: 8.5.x-dev » 8.4.x-dev

Being such a minor patch, I think this should still apply for 8.4.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
77.18 KB
65.5 KB

LTR

LTR

RTL

RTL

Wim Leers’s picture

Can't wait for this one to land! 😍 🎉

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/themes/stable/css/system/components/container-inline.module.css
@@ -5,7 +5,7 @@
 .container-inline div,
 .container-inline label {
-  display: inline;
+  display: inline-block;

Could override this in Seven theme instead so we don't break existing themes.

Other than that the patch looks good. I also confirmed with @yoroy that he's happy with the UX of this patch.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
2.81 KB
1.49 KB

Ok, so both files I assume, how about this. The screenshots are the same for seven.

Neograph734’s picture

I know that I am not the one to judge here, and that other themes should not break... But by moving this to seven, classy will still have inline radio's with trailing colons, which does not make sense.

The choice to move the code from stable to seven makes more sense. (Though visually inline and inline-block make no difference, but changing it is a mere requirement for correctly grouping labels and radio's.) Other downstream themes could still have issues with RTL and labels. (Then again that would be the issues of those themes.)

joelpittet’s picture

@Neograph734 It's a problem but there isn't much we can do about it... We can fix things without breaking code API in contrib/custom themes by making changes in Core/Bartik/Seven, but just like D7, the CSS doesn't change in stable & classy. We are lucky that unlike D7 we can do it in Core, Seven and Bartik because they are @internal.

Neograph734’s picture

That is unfortunate, I would say this is a bug in classy. But fair enough. I can test your patch tomorrow, if someone else beats me to it it is fine as well. Thanks for the reroll.

ckrina’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
135.27 KB
112.16 KB

It works for me!
radio ltr
radio ltr

joelpittet’s picture

@Neograph734 we could try a follow-up to try and make a case for a bug in classy and make as minimal changes to fix the bug. I'd support that, but this is the path of least resistance.

Neograph734’s picture

@joelpittet, I agree that it is more important to get it in first. I will attempt the followup tomorrow.

joelpittet’s picture

@Neograph734 I've made the issue, tried giving it an issue summary. Feel free to update that and add a patch when you're around. #2912824: Inappropriate trailing colons in Claro and Umami

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 92: 2236789-92.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Retesting. It failed with "CI job missing" — some DrupalCI internal thing gone wrong.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
Neograph734’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.74 KB

Rerolled for 8.5.

#92 should still apply to 8.4.

Neograph734’s picture

Status: Needs review » Reviewed & tested by the community

And back to RTBC, the only reason this failed to apply was because one line in seven.libraries.yml had been moved. Nothing functional was changed.

lauriii’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed a2a199a and pushed to 8.5.x. Thanks!

  • lauriii committed a2a199a on 8.5.x
    Issue #2236789 by singh_haneet, Neograph734, petruk.dmitriy, idebr,...
Wim Leers’s picture

🎉 ❤️

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.