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.
Comment | File | Size | Author |
---|---|---|---|
#103 | 2236789-103.patch | 1.74 KB | Neograph734 |
#89 | Create_Article___Site-Install_-_LTR.png | 77.18 KB | joelpittet |
Comments
Comment #1
Wim LeersClarifying title. This is crucial for the UX of the
EditorImageDialog
.Comment #2
Wim LeersComment #3
Wim LeersOops, sorry, probably not a novice issue.
Comment #4
mgiffordI'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.
Comment #5
sunWe 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.
Comment #6
mgiffordThanks @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...Comment #7
YesCT CreditAttribution: YesCT commented#2275373: Add function for shared logic between fieldset and form element (which might be "make radios and checkboxes form elements") might help here.
Comment #8
LewisNyman CreditAttribution: LewisNyman commentedComment #9
Wim LeersOthers 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.
Comment #10
catchWhat'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.
Comment #11
Wim LeersComment #12
Wim Leers#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.
Comment #13
cilefen CreditAttribution: cilefen commentedI just checked—this is still broken.
Comment #14
dpetruk CreditAttribution: dpetruk commentedComment #15
dpetruk CreditAttribution: dpetruk commentedComment #16
dpetruk CreditAttribution: dpetruk as a volunteer commentedHi, 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)
Comment #17
dpetruk CreditAttribution: dpetruk as a volunteer commentedAnother variant to fix this issue.
I know this code doesn't look nice, but it works)
Comment #18
lauriiiComment #19
eleleka CreditAttribution: eleleka commented#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.
Comment #20
Wim LeersWe 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.Comment #21
mgifford@Wim Leers, would adding the container-inline css address @eleleka concerns with the max width?
Just posting in @sun's CSS code from #5.
Comment #22
Wim LeersThe "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.
Comment #23
singh_haneet CreditAttribution: singh_haneet as a volunteer commentedHi all,
I applied the following changes:
1. Aligned the legend to left as per the discussion.
2. Removed the trailing colons from the radios.
Comment #24
Wim LeersWOOOT! Thanks :)
Comment #25
Truptti CreditAttribution: Truptti at Axelerant commentedVerified 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
Comment #26
Truptti CreditAttribution: Truptti at Axelerant commentedComment #27
LewisNyman CreditAttribution: LewisNyman at Wunder commentedWim asked me to review this.
Comment #28
LewisNyman CreditAttribution: LewisNyman at Wunder commentedFull stop is needed at the end of the comment.
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 thereWe can also remove the blank lines in between each group
Comment #29
singh_haneet CreditAttribution: singh_haneet as a volunteer commentedAdded new patch. Moved the code to Classy's form.css without using container-inline selector.
Comment #30
a_thakur CreditAttribution: a_thakur as a volunteer and at Srijan | A Material+ Company commentedAfter applying this patch the radio buttons on the install page are broken, could you please check this? Probably the global effect is breaking things.
I see similar issue on other pages with radio buttons.
Comment #31
Manjit.Singhlooking into this issue.
Comment #32
LewisNyman CreditAttribution: LewisNyman at Wunder commentedAh 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 thecontainer-inline
class.Comment #33
singh_haneet CreditAttribution: singh_haneet as a volunteer commentedAdded 'form-composite--inline' class for Editor's image dialog and used it as selector for styling radios so that it doesn't effect globally.
Comment #34
LewisNyman CreditAttribution: LewisNyman at Wunder commentedGreat stuff! Thanks.
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.Comment #35
singh_haneet CreditAttribution: singh_haneet as a volunteer commentedThat makes sense to wrap the spacing in a media query, so adding those in this patch. Hope that helps :)
Comment #36
singh_haneet CreditAttribution: singh_haneet as a volunteer commentedMissing ; added in a new patch. Don't know why it was missing earlier.
Comment #37
Manjit.SinghSorry, 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).
Comment #38
Manjit.SinghComment #39
singh_haneet CreditAttribution: singh_haneet as a volunteer commentedNew patch added.
Comment #40
Manjit.SinghLooks good now.
Before
After
Comment #41
Wim LeersThat 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 :) :)
Comment #42
meenakshi.r CreditAttribution: meenakshi.r as a volunteer and at Srijan | A Material+ Company commentedIt is fixed.
Before patch
After patch
Comment #43
Wim Leers#42:
Thanks for the visual confirmation, now we just need confirmation from a CSS expert.
Comment #44
Bojhan CreditAttribution: Bojhan as a volunteer commentedComment #45
Bojhan CreditAttribution: Bojhan as a volunteer commentedComment #46
singh_haneet CreditAttribution: singh_haneet as a volunteer commented@Wim Leers, @LewisNyman I was wondering if we could get this closed before D8 release it would be awesome :)
Comment #47
Wim LeersThanks for reminding us! I pinged LewisNyman on Twitter: https://twitter.com/wimleers/status/662665026084470784.
Comment #48
LewisNyman CreditAttribution: LewisNyman at Wunder commentedThe CSS looks good! Thanks
Comment #49
singh_haneet CreditAttribution: singh_haneet as a volunteer commentedGreat. Thanks :)
Comment #50
herom CreditAttribution: herom as a volunteer commentedThis also needed RTL styling.
Here are some screenshots:
RTL (before):
RTL (after, #39):
RTL (after, #50):
Comment #51
joelpittetThanks for the screenshots look great, just a review.
What is this 1px on the top for?
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.
nit: One space at the front of this line. (can be fixed on commit)
Comment #52
xjm@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.
Comment #53
Wim Leers+1
Comment #54
joelpittetWonder if someone can address the points in #51? Setting to needs work.
Comment #55
LewisNyman CreditAttribution: LewisNyman at Wunder commentedI compared before and after and this is legit. It aligned properly with 1px.
Fixed.
Comment #56
Wim LeersIt's fine to re-RTBC per your own RTBC conclusion if you're only changing whiespace :)
Comment #57
singh_haneet CreditAttribution: singh_haneet as a volunteer commentedIf everything is fine can we move ahead ?
Comment #58
Wim LeersSee #52. This will be committed in 8.0.1.
Comment #59
singh_haneet CreditAttribution: singh_haneet as a volunteer commentedOkay I missed that earlier. Travel lagging woes ;)
Comment #60
Wim Leersnp :)
Comment #61
xjmThanks 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.
' : '
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: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...
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.
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.
Comment #62
joelpittet@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
Comment #63
Wim Leers#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!
Comment #64
Wim Leers(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.)
Comment #65
emma.mariaI'm adding the @Bojhan bat signal tag also :)
PS. @Wim Leers if there is anything I can help with here, please let me know.
Comment #67
Bojhan CreditAttribution: Bojhan as a volunteer commentedI 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.
Comment #68
Bojhan CreditAttribution: Bojhan as a volunteer commentedComment #69
Wim LeersSure, let's talk at DrupalCon. But the issue summary's "original" vs "current" screenshots are still accurate: they still show the problem.
Comment #70
stpaultim CreditAttribution: stpaultim as a volunteer commentedRemoving the "Novice" tag, since there doesn't seem to be anything that a "novice" can do until decisions are made by some non-novices.
Comment #71
Wim LeersBump. 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.Comment #74
idebr CreditAttribution: idebr at ezCompany commentedConsidering 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):
Comment #75
idebr CreditAttribution: idebr at ezCompany commentedClosed #2880995: UI Clean Up as a duplicate.
Comment #76
Bojhan CreditAttribution: Bojhan as a volunteer commentedComment #77
Wim LeersThis looks like a much more elegant solution, thanks! :)
Comment #78
lauriiiI 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.
Comment #79
Wim LeersThe only change is the removal of colons. Surely that's a step forward?
Comment #80
Neograph734I have little experience with RTL languages, but I'd expect RTL to look like this:
LTR looks much better now.
Comment #81
Wim LeersSure 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?
Comment #82
jibranComment #83
yoroy CreditAttribution: yoroy at Roy Scholten commentedBut the "after" screenshot for RTL looks worse than the "before", so can we (double) check that this patch does not make things worse there?
Comment #86
Neograph734The 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:
@yoroy and Wim Leers, could you have a look?
Comment #87
Neograph734Comment #88
Neograph734Being such a minor patch, I think this should still apply for 8.4.
Comment #89
joelpittetLTR
RTL
Comment #90
Wim LeersCan't wait for this one to land! 😍 🎉
Comment #91
lauriiiCould 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.
Comment #92
joelpittetOk, so both files I assume, how about this. The screenshots are the same for seven.
Comment #93
Neograph734I 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.)
Comment #94
joelpittet@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.
Comment #95
Neograph734That 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.
Comment #96
ckrinaIt works for me!
Comment #97
joelpittet@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.
Comment #98
Neograph734@joelpittet, I agree that it is more important to get it in first. I will attempt the followup tomorrow.
Comment #99
joelpittet@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
Comment #101
Wim LeersRetesting. It failed with "CI job missing" — some DrupalCI internal thing gone wrong.
Comment #102
lauriiiComment #103
Neograph734Rerolled for 8.5.
#92 should still apply to 8.4.
Comment #104
Neograph734And 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.
Comment #105
lauriiiCommitted a2a199a and pushed to 8.5.x. Thanks!
Comment #107
Wim Leers🎉 ❤️