Follow-up to #2539860: Add a class to field that contain user-generated formatted text
Problem/Motivation
It's common to provide styling for base elements, elements that don't have classes. This styling is used in situations where a user can enter formatted HTML text. This can often introduce conflicts with UI components, if they are using the elements that have styling applied.
Some themes get around this by prefixing the base selectors with a wrapper class a indicates they are free text, not a UI component.
Bartik has an attempt to do this:
.region-content ul,
.region-content ol {
margin: 1em 0;
padding: 0 0 0.25em 15px; /* LTR */
}
The logic of this selector is flawed because anything can go inside the content region, not just free text. Any UI component placed in .region-content that includes ul could inherit this styling by mistake.
Proposed resolution
In #2539860: Add a class to field that contain user-generated formatted text we added a class called 'text-formatted' that only applies to fields that include free text. This reduces the scope of the selector.
Remaining tasks
Patch
Test for visual regressions
User interface changes
None
API changes
None
Data model changes
None
Beta phase evaluation
Issue category | Task because it's improves code quality |
---|---|
Issue priority | Not critical because it's code quality |
Unfrozen changes | Unfrozen because it only changes CSS |
Comment | File | Size | Author |
---|---|---|---|
#64 | replace_the-2541252-64.patch | 3.06 KB | PapaGrande |
#53 | replace_the-2541252-53.patch | 2.95 KB | pjbaert |
Comments
Comment #1
HOG CreditAttribution: HOG at Skilld commentedEdited class.
Comment #2
HOG CreditAttribution: HOG at Skilld commentedComment #3
LewisNyman CreditAttribution: LewisNyman at Wunder commentedWe need to test a few pages closely for regressions because the last selector was so broad
Comment #4
HOG CreditAttribution: HOG at Skilld commentedContent in comment:
Comments footer:
Menu block:
Comment #5
HOG CreditAttribution: HOG at Skilld commentedComment #6
emma.mariaI am setting this back to Needs Review. I would like to double check this more closely as I'm very aware that this will affect a lot of components. I will get to this task as soon as I can.
Comment #7
ti2m CreditAttribution: ti2m commentedI ran siteeffect on the patch and found regressions for lists concerning the margin and padding. Screenshots are of / and /filter/tips.
Italic text seems to be off as well, but I think that has to do with the font rendering in phantomjs and isn't related to the issue.
Comment #8
finnsky CreditAttribution: finnsky at Skilld commentedAdded styles for .links and .tips lists.
Comment #9
finnsky CreditAttribution: finnsky at Skilld commentedComment #10
emma.mariaHi @finnsky your patch contains a huge amount of things not relevant to this issue. Please look at the contents of your patch before uploading.
Also each time please create interdiff files along with the patch so we can see the differences between the last patch and the one you are posting - instructions here. Thanks.
Comment #11
finnsky CreditAttribution: finnsky at Skilld commentedSorry. wrong diff:)
Comment #12
HOG CreditAttribution: HOG at Skilld commentedNow, margin & padding present.
Comment #13
HOG CreditAttribution: HOG at Skilld commentedComment #14
HOG CreditAttribution: HOG at Skilld commentedComment #16
HOG CreditAttribution: HOG at Skilld commentedApply #1 patch & changes from #11 patch.
Comment #17
HOG CreditAttribution: HOG at Skilld commentedComment #18
LewisNyman CreditAttribution: LewisNyman at Wunder commentedCheers
This requires a @file comment like other CSS files
Comment #19
ti2m CreditAttribution: ti2m commentedRan the latest patch through siteeffect again and couldn't spot any regressions anymore.
Comment #20
LewisNyman CreditAttribution: LewisNyman at Wunder commentedThis needs a reroll after we moved some files around.
Comment #21
HOG CreditAttribution: HOG at Skilld commentedRerolled patch #16.
Comment #22
HOG CreditAttribution: HOG at Skilld commentedScreens like #17
Comment #23
emma.mariaI would like to postpone this issue. I do not want to add fixes here to other components because we are making a big change to the markup.
There is a seperate issue that tackles all of the list instances in Bartik, sets sensible default styling and fixes a lot of flaws in Bartik's styles here... #2548805: Add sensible base layout styles for lists in Bartik..
Once this issue is committed we can strip back this patch to just have the straightforward switch of
.region-content ul/ol
totext-formatted ul/ol
without any extra fixes. I tested this out in the other issue and it works well.Comment #24
emma.mariaNow that #2548805: Add sensible base layout styles for lists in Bartik. is in, we should just be able to have a fresh new patch with just the straightforward class swap and putting the styles of the new classes in their own component file (text-formatted).
Comment #25
emma.mariaComment #26
maris.abols CreditAttribution: maris.abols commentedComment #27
maris.abols CreditAttribution: maris.abols commentedCreated simple patch to solve this straightforward issue.
Comment #28
maris.abols CreditAttribution: maris.abols commentedComment #31
bruvers CreditAttribution: bruvers at Wunder commentedI've tested the patch from #27 with the visual regression testing tool BackstopJS and found just 2 differences rather than regressions.
Probably unintentional this patch fixes an apparent styling bug on the user/login page on small devices where the action tabs were offset to the right and top. See attached screenshots login-before.png and login-after.png.
The other difference was that Bartik's styling of ol and ul tags sets a smaller padding-left and no margin-top, margin-bottom at all. This is not a regression but a conscious styling decision from Bartik and should be dealt with there if at all.
Comment #32
LewisNyman CreditAttribution: LewisNyman at Wunder commentedAssigning to emma for final sign off as she understands all the Bartik CSS really well.
Comment #33
emma.mariaI carried out some visual regression testing - full report attached. Most of the fails are actually fixes of visual problems within Bartik, which makes me very happy :)
This issue fixes visual issues for: Tabs at mobile & tablet width, and vertical tabs.
The content this issue directly deals with is user formatted text, for eg. HTML output from ckeditor. I found 0 visual regressions for this content.
There is also a slight visual difference for the lists within Tips as mentioned also in #31. But I am happy to allow this as the positive impact of this work far outweighs a small amount of extra margin.
I did notice two things that we need to improve within the patch.
text-formatted
styles out into their own file, text-formatted.css. Text-formatted is a separate component to main-content.Comment #34
emma.mariaComment #35
LewisNyman CreditAttribution: LewisNyman at Wunder commentedComment #36
saki007sterComment #37
saki007sterMade the changes suggested by emma in comment #33 and updated the patch in #27
Comment #38
saki007sterComment #40
emma.mariaComment #42
nlisgo CreditAttribution: nlisgo commentedComment #43
nlisgo CreditAttribution: nlisgo commentedI am going to investigate the failing test and address it if I can.
Comment #44
nlisgo CreditAttribution: nlisgo commentedComment #45
nlisgo CreditAttribution: nlisgo commentedComment #46
LewisNyman CreditAttribution: LewisNyman at Wunder commentedNice work!
All we need to do here is add a blank line under the @file comment
Comment #47
alvar0hurtad0Here's the patch with #46 applyed
Comment #50
LewisNyman CreditAttribution: LewisNyman at Wunder commentedAs I was testing this I noticed that the library wasn't being loaded on the frontend. Here's the updated patch and the visual regression testing report.
Comment #51
emma.mariaI am happy with the results of the visual testing. The patch in #50 has the same test results as what was found in #33.
This task has helped fix visual problems with tabs and vertical tabs which have been visually broken for months within Bartik.
Thank you to everyone who worked on this issue, setting this to RTBC.
Comment #53
pjbaertRerolled the patch
Comment #54
andypostback to rtbc
Comment #55
alexpottI think these types of changes should be made to Bartik in 8.1.x and not in 8.0.x because whilst Bartik is @internal this is not a bugfix and 8.0.x is only for bug fixes to stable code.
Comment #56
alexpottCommitted ba82fc6 and pushed to 8.1.x. Thanks!
Comment #59
tstoeckler#2643020: Remove left margin on Bartik mobile tabs was opened against 8.0.x which mentions the problem that was already found in #33 of this issue. That definitely makes this a bug fix so I think it should apply to 8.0.x, thus re-opening.
Comment #60
Chi CreditAttribution: Chi commentedMarked #2643196: Fix vertical tabs appearance in Bartik theme as duplicate.
Comment #61
claudiu.cristeaComment #62
pazhyn CreditAttribution: pazhyn at AnyforSoft commentedComment #63
emma.mariaThanks @tstoeckler. I can confirm that this issue fixes a lot of visual bugs in Bartik including the Tabs and has only been applied to 8.1.x. It would be great for this to be added to 8.0.x.
Can the patch in #53 be re-rolled to apply on the 8.0.x branch please.
Comment #64
PapaGrandeHere is a reroll of #53 against the 8.0.x branch.
This also solves the formatting issue for vertical tabs in #2102659: Add new Form API example module for Drupal 8.
Comment #65
pazhyn CreditAttribution: pazhyn as a volunteer and at AnyforSoft commentedPatch of #64 was successfully applied and solving the issue.
But I've noticed problem of adding margins and padding in multilevel lists.
So patch from #64 was complemented with fixing this issue.
See before and after screenshots.
Login tabs are also fixed with #64 patch.
Review, please.
Comment #66
andypostI'd better put that library into classy theme so all themes could use this
Comment #67
pazhyn CreditAttribution: pazhyn as a volunteer and at AnyforSoft commentedReview please.
Comment #69
emma.mariaAssigning this back to 8.0.x and Bartik as this is what the issue is currently for. Also un-assigning to get reviews on @pazhyn's patch.
Comment #70
pazhyn CreditAttribution: pazhyn as a volunteer and at AnyforSoft commentedReview #65 patch or consider #67 patch (making the fix wider then only Bartik theme, Classy is base for Bartik as well, so the issue will also be fixed anyway).
Comment #71
pazhyn CreditAttribution: pazhyn as a volunteer and at AnyforSoft commentedComment #72
claudiu.cristeaComment #73
lauriiiThanks everyone for your work here! It seems like there is some confusion what should be done since there is multiple things laying around this issue. So what I think we should do is to fix #65 and #2659956: [regression] Bartik multi-level list styles are broken first for 8.1.x and then backport the fixed version for 8.0.x.
We also should make this change only for Bartik for 2 reasons:
Comment #74
lauriiiComment #75
LewisNyman CreditAttribution: LewisNyman at Wunder commentedComment #76
emma.mariaComment #77
emma.mariaComment #78
emma.mariaComment #79
emma.mariaThe patch in #64 is a reroll of the #53 that was committed to 8.1.x. It cleanly applies and does not cause any obvious regressions to lists....
Region-content lists
BEFORE
AFTER
and also fixes the following bugs...
Vertical tabs
(these bugs have been raised in many seperate duplicate issues - see referenced and related issues at the top)
BEFORE
AFTER
Tabs
BEFORE
AFTER
Setting this to be RTBC as a fix on the 8.0.x branch - the visual bugs shown in this comment have been raised a lot in seperate issues.
Comment #80
star-szrAfter the sprint yesterday I think I understand the bug fixing aspect of this now so would like to take a look :)
Comment #82
star-szrThis is a bug fix so it makes sense to include in 8.0.x and overall is a very good change I think. Going forward we can make good use of
.text-formatted
as a convention for content styling to prevent weirdness.Committed 7a905d5 (#64) and pushed to 8.0.x. Thanks!
Comment #83
alvar0hurtad0Should this issue be ported to 8.1.x?
Comment #84
star-szrIt was already committed to 8.1.x, see #56. Now it's been back ported to 8.0.x because it's a bug fix after all :)
Comment #85
alvar0hurtad0:D
Ok, thanks!!