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

Reference: https://www.drupal.org/core/beta-changes
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
CommentFileSizeAuthor
#79 after-vertical.png64.48 KBemma.maria
#79 before-vertical.png60.85 KBemma.maria
#79 after-tabs.png23.65 KBemma.maria
#79 before-tabs.png27.18 KBemma.maria
#79 after-ul-ol-list.png133.67 KBemma.maria
#79 after-ul-list.png74.36 KBemma.maria
#79 before-ul-ol-list.png135.46 KBemma.maria
#79 Before-ul-list.png67.18 KBemma.maria
#67 replace_region_content_ul_ol_selector_with_text_formatted-2541252-67.patch3.27 KBpazhyn
#65 login-tabs.png79.25 KBpazhyn
#65 after.png72.22 KBpazhyn
#65 before.png74.51 KBpazhyn
#65 replace_region_content_ul_ol_selector_with_text_formatted-2541252-65.patch3.15 KBpazhyn
#64 replace_the-2541252-64.patch3.06 KBPapaGrande
#53 replace_the-2541252-53.patch2.95 KBpjbaert
#50 BackstopJS--replace_the-2541252-50.pdf1.67 MBLewisNyman
#50 replace_the-2541252-50.patch2.95 KBLewisNyman
#50 interdiff.txt966 bytesLewisNyman
#47 replace_the-2541252-47.patch2.41 KBalvar0hurtad0
#47 interdiff.txt387 bytesalvar0hurtad0
#44 replace_the-2541252-44.patch2.41 KBnlisgo
#44 interdiff-2541252-37-44.txt878 bytesnlisgo
#37 interdiff-27-37.txt1.29 KBsaki007ster
#37 replace_the_region_content-2541252-37.patch1.55 KBsaki007ster
#33 formatted_text.png120.8 KBemma.maria
#33 tabs.png108.4 KBemma.maria
#33 vertical-tabs.png98.54 KBemma.maria
#31 filtertips2-after.png39.36 KBbruvers
#31 filtertips2-before.png39.04 KBbruvers
#31 filtertips-after.png28.54 KBbruvers
#31 filtertips-before.png26.94 KBbruvers
#31 login-after.png34.66 KBbruvers
#31 login-before.png34.64 KBbruvers
#27 replace_the_region_content-2541252-1-22.patch680 bytesmaris.abols
#21 interdiff.txt2.19 KBHOG
#21 replace_the_region_content-2541252-1-21.patch2.04 KBHOG
#17 taxonomy-terms.png137.47 KBHOG
#17 tipts.png190.07 KBHOG
#17 links.png116.93 KBHOG
#17 comments.png123.27 KBHOG
#17 content-list.png196.52 KBHOG
#1 replace_the_region_content-2541252-1.patch651 bytesHOG
#1 interdiff.txt830 bytesHOG
#1 text-formatted-class.jpg417.04 KBHOG
#4 Text-formatted-comment.jpg182.25 KBHOG
#4 Comment-footer.jpg192.18 KBHOG
#4 Menu-block.jpg254.29 KBHOG
#7 2541252_home.png35.43 KBti2m
#7 2541252_filter_tips.png225.91 KBti2m
#8 interdiff-2541252-10167096-10180914.txt941 bytesfinnsky
#9 2541252-10180914.patch365.26 KBfinnsky
#11 2541252-10180914.patch941 bytesfinnsky
#12 ul-links.png115.53 KBHOG
#12 ul-tip.png184.28 KBHOG
#16 interdiff.txt2.18 KBHOG
#16 replace_the_region_content-2541252-1-16.patch2.01 KBHOG
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

HOG’s picture

Status: Active » Needs review
FileSize
651 bytes
830 bytes
417.04 KB

Edited class.

HOG’s picture

New text-formatted class

LewisNyman’s picture

We need to test a few pages closely for regressions because the last selector was so broad

HOG’s picture

FileSize
182.25 KB
192.18 KB
254.29 KB

Content in comment:
Text-formatted-comment

Comments footer:
Comments footer

Menu block:
Menu block

HOG’s picture

Status: Needs review » Reviewed & tested by the community
emma.maria’s picture

Status: Reviewed & tested by the community » Needs review

I 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.

ti2m’s picture

FileSize
35.43 KB
225.91 KB

I ran siteeffect on the patch and found regressions for lists concerning the margin and padding. Screenshots are of / and /filter/tips.

regressions home

regressions 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.

finnsky’s picture

Added styles for .links and .tips lists.

finnsky’s picture

emma.maria’s picture

Status: Needs review » Needs work

Hi @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.

finnsky’s picture

Sorry. wrong diff:)

HOG’s picture

Issue summary: View changes
FileSize
115.53 KB
184.28 KB

ul-links
ul-tip

Now, margin & padding present.

HOG’s picture

Status: Needs work » Needs review
HOG’s picture

Status: Needs review » Needs work

The last submitted patch, 11: 2541252-10180914.patch, failed testing.

HOG’s picture

Status: Needs work » Needs review
FileSize
2.01 KB
2.18 KB

Apply #1 patch & changes from #11 patch.

HOG’s picture

Issue summary: View changes
FileSize
196.52 KB
123.27 KB
116.93 KB
190.07 KB
137.47 KB

content-list
comments
links
tipts
taxonomy-terms

LewisNyman’s picture

Status: Needs review » Needs work

Cheers

+++ b/core/themes/bartik/css/components/tips.css
@@ -0,0 +1,8 @@
+/* ----------------- Tips ----------------- */
+ul.tips {

This requires a @file comment like other CSS files

ti2m’s picture

Ran the latest patch through siteeffect again and couldn't spot any regressions anymore.

LewisNyman’s picture

This needs a reroll after we moved some files around.

HOG’s picture

Status: Needs work » Needs review
FileSize
2.04 KB
2.19 KB

Rerolled patch #16.

HOG’s picture

Status: Needs review » Reviewed & tested by the community

Screens like #17

emma.maria’s picture

Status: Reviewed & tested by the community » Postponed
Issue tags: -Needs reroll

I 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 to text-formatted ul/ol without any extra fixes. I tested this out in the other issue and it works well.

emma.maria’s picture

Status: Postponed » Active

Now 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).

emma.maria’s picture

Issue tags: +Novice
maris.abols’s picture

Assigned: Unassigned » maris.abols
maris.abols’s picture

Created simple patch to solve this straightforward issue.

maris.abols’s picture

Assigned: maris.abols » Unassigned
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 27: replace_the_region_content-2541252-1-22.patch, failed testing.

Status: Needs work » Needs review
bruvers’s picture

I'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.

LewisNyman’s picture

Assigned: Unassigned » emma.maria
Status: Reviewed & tested by the community » Needs review

Assigning to emma for final sign off as she understands all the Bartik CSS really well.

emma.maria’s picture

Issue summary: View changes
FileSize
98.54 KB
108.4 KB
120.8 KB

I 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.

  1. We need to move the text-formatted styles out into their own file, text-formatted.css. Text-formatted is a separate component to main-content.
  2. As this markup can be creating within ckeditor, text-formatted.css also needs to also be included as one of the stylesheets that ckeditor loads on the edit page. This can be found within bartik.info.yml
emma.maria’s picture

Assigned: emma.maria » Unassigned
LewisNyman’s picture

Status: Needs review » Needs work
saki007ster’s picture

Assigned: Unassigned » saki007ster
saki007ster’s picture

Made the changes suggested by emma in comment #33 and updated the patch in #27

saki007ster’s picture

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

Status: Needs review » Needs work

The last submitted patch, 37: replace_the_region_content-2541252-37.patch, failed testing.

emma.maria’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 37: replace_the_region_content-2541252-37.patch, failed testing.

nlisgo’s picture

Assigned: Unassigned » nlisgo
nlisgo’s picture

I am going to investigate the failing test and address it if I can.

nlisgo’s picture

Assigned: nlisgo » Unassigned
Status: Needs work » Needs review
FileSize
878 bytes
2.41 KB
nlisgo’s picture

Issue tags: +Barcelona2015
LewisNyman’s picture

Status: Needs review » Needs work

Nice work!

+++ b/core/themes/bartik/css/components/text-formatted.css
@@ -0,0 +1,13 @@
+/**
+ * @file
+ * Visual styles for Bartik's text-formatted component.
+ */
+.text-formatted ul,

All we need to do here is add a blank line under the @file comment

alvar0hurtad0’s picture

Status: Needs work » Needs review
FileSize
387 bytes
2.41 KB

Here's the patch with #46 applyed

Status: Needs review » Needs work

The last submitted patch, 47: replace_the-2541252-47.patch, failed testing.

Status: Needs work » Needs review
LewisNyman’s picture

As 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.

emma.maria’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 50: replace_the-2541252-50.patch, failed testing.

pjbaert’s picture

Status: Needs work » Needs review
FileSize
2.95 KB

Rerolled the patch

andypost’s picture

alexpott’s picture

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

I 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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ba82fc6 and pushed to 8.1.x. Thanks!

  • alexpott committed ba82fc6 on 8.1.x
    Issue #2541252 by HOG, finnsky, LewisNyman, nlisgo, saki007ster,...

Status: Fixed » Closed (fixed)

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

tstoeckler’s picture

Version: 8.1.x-dev » 8.0.x-dev
Category: Task » Bug report
Status: Closed (fixed) » Active

#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.

Chi’s picture

claudiu.cristea’s picture

Issue tags: -Barcelona2015 +#SprintLUX2016, +SprintWeekend2016
pazhyn’s picture

Assigned: Unassigned » pazhyn
emma.maria’s picture

Status: Active » Needs work
Issue tags: +Needs reroll

Thanks @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.

PapaGrande’s picture

Status: Needs work » Needs review
FileSize
3.06 KB

Here 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.

pazhyn’s picture

Patch 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.

before

after

Login tabs are also fixed with #64 patch.

login-tabs

Review, please.

andypost’s picture

Issue tags: -Needs reroll

I'd better put that library into classy theme so all themes could use this

pazhyn’s picture

Version: 8.0.x-dev » 8.1.x-dev
Component: Bartik theme » Classy theme
FileSize
3.27 KB

Review please.

Status: Needs review » Needs work
emma.maria’s picture

Version: 8.1.x-dev » 8.0.x-dev
Component: Classy theme » Bartik theme

Assigning 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.

pazhyn’s picture

Status: Needs work » Needs review

Review #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).

pazhyn’s picture

Assigned: pazhyn » Unassigned
claudiu.cristea’s picture

Issue tags: -#SprintLUX2016
lauriii’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Postponed

Thanks 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:

  • This is Bartik specific issue
  • We are not allowed to change CSS or Markup in Classy because it is pretty much frozen for BC reasons.
lauriii’s picture

LewisNyman’s picture

Status: Postponed » Active
emma.maria’s picture

Category: Bug report » Task
Status: Active » Needs review
Issue tags: -Novice
emma.maria’s picture

Assigned: Unassigned » emma.maria
emma.maria’s picture

Version: 8.1.x-dev » 8.0.x-dev
emma.maria’s picture

Title: Replace the .region-content ul/ol selector with text-formatted » Replace the .region-content ul/ol selector with text-formatted to refactor code + fix visual bugs.
Assigned: emma.maria » Unassigned
Category: Task » Bug report
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
67.18 KB
135.46 KB
74.36 KB
133.67 KB
27.18 KB
23.65 KB
60.85 KB
64.48 KB

The 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.

star-szr’s picture

Assigned: Unassigned » star-szr

After the sprint yesterday I think I understand the bug fixing aspect of this now so would like to take a look :)

  • Cottser committed 7a905d5 on 8.0.x
    Issue #2541252 by HOG, pazhyn, finnsky, LewisNyman, nlisgo, saki007ster...
star-szr’s picture

Title: Replace the .region-content ul/ol selector with text-formatted to refactor code + fix visual bugs. » Replace the .region-content ul/ol selector with text-formatted to refactor code + fix visual bugs
Assigned: star-szr » Unassigned
Status: Reviewed & tested by the community » Fixed

This 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!

alvar0hurtad0’s picture

Should this issue be ported to 8.1.x?

star-szr’s picture

It 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 :)

alvar0hurtad0’s picture

:D

Ok, thanks!!

Status: Fixed » Closed (fixed)

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