Problem/Motivation

#2398463: Clean up the "content" component in Bartik removed core/themes/bartik/css/components/content.css. But bartik.info.yml's ckeditor_stylesheets section is still referring to it. Which causes CKEditor to try to load this file, resulting in a 404.

Steps to reproduce

  1. Create a node.
  2. Go to that node's full page
  3. Ensure the comment form is loaded
  4. Look in the browser's developer console, you'll see a 404.

Proposed resolution

Fix it, by stopping to tell CKEditor to load this file. AFAICT the only selectors left in content.css were already irrelevant to any HTML in CKEditor, so its contents must've changed significantly since #2064379: Remove ckeditor-iframe.css and load relevant Bartik CSS files for CKEditor's iframe mode. added that file to be loaded.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#5 follow_up_for_2398463-2552175-5.patch1.31 KBnlisgo
#5 interdiff-2552175-2-5.txt939 bytesnlisgo
#2 2552175-2.patch473 byteswim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Priority: Major » Normal
Status: Active » Needs review
StatusFileSize
new 2552175-2.patch473 bytes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2552187: Follow-up for #2395853: CKEditor is loading system.modules.css, which no longer exists

Status: Needs review » Needs work

The last submitted patch, 2: 2552175-2.patch, failed testing.

nlisgo’s picture

nlisgo commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2552175-2-5.txt939 bytes
new follow_up_for_2398463-2552175-5.patch1.31 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Thanks, @nlisgo!

stefan.r’s picture

stefan.r commented
Status: Needs review » Reviewed & tested by the community

Manually confirmed this gets rid of the 404. All references to this file seem to have been removed, so RTBC!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: follow_up_for_2398463-2552175-5.patch, failed testing.

willzyx’s picture

willzyx commented
Status: Needs work » Reviewed & tested by the community

Another case of #2552687: Test failures in ConfigFormOverrideTest and ContainerRebuildWebTest on newly spun up testbot instances back to RTBC

emma.maria queued 5: follow_up_for_2398463-2552175-5.patch for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Is this the right fix? Shouldn't we be loading some of the files added by #2398463: Clean up the "content" component in Bartik

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs work » Reviewed & tested by the community

It doesn't look like it. All the styling in the old content.css file was themed output. None of it was base element styling.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed a3542ee and pushed to 8.0.x. Thanks!

+++ /dev/null
@@ -1,241 +0,0 @@
-#content h2 {
-  margin-bottom: 2px;
-  font-size: 1.429em;
-  line-height: 1.4;
-}
...
-.region-content ul,
-.region-content ol {
-  margin: 1em 0;
-  padding: 0 0 0.25em 15px; /* LTR */
-}
-[dir="rtl"] .region-content ul,
-[dir="rtl"] .region-content ol {
-  padding: 0 15px 0.25em 0;
-}

Perhaps the intention was to have things like this apply. It would not have worked though. I guess another issue should explore what the ckeditor styles for bartik should be.

  • alexpott committed a3542ee on 8.0.x 
    Issue #2552175 by nlisgo, Wim Leers: Follow-up for #2398463: Bartik is...

Status: Fixed » Needs work

The last submitted patch, 5: follow_up_for_2398463-2552175-5.patch, failed testing.

willzyx’s picture

willzyx commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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