Problem/Motivation

#2395853: Split system.module.css and system.theme.css files into SMACSS style components removed core/modules/system/css/system.module.css . But the CKEditor module 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 that really were needed in system.module.css, were the text alignment ones. Those now live in align.module.css, so that's the one the patch updates it to.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because CSS is creating 404
Issue priority Not critical because bug is not prohobitive
Unfrozen changes Unfrozen because it only changes CSS
CommentFileSizeAuthor
#7 wadawd___Site-Install.png961.34 KBLewisNyman
#6 2552187-6.patch1.56 KBWim Leers
#2 2552187-2.patch902 bytesWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Status: Active » Needs review
FileSize
2552187-2.patch902 bytes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Related issues: +#2552175: Follow-up for #2398463: Bartik is telling CKEditor to load a CSS file that no longer exists
willzyx’s picture

willzyx CreditAttribution: willzyx commented

probably we need to update Drupal\ckeditor\Tests\CKEditorTest::getDefaultContentsCssConfig() too..

Status: Needs review » Needs work

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Status: Needs work » Needs review
FileSize
2552187-6.patch1.56 KB

Indeed!

Done.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +frontend, +CSS
FileSize
wadawd___Site-Install.png961.34 KB

Thanks! I can confirm that the CSS file is loaded correctly. I also looked over the WYSIWYG functions and the system CSS files. I couldn't see any other CSS files that should be loaded apart from align.module.css

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented
Issue summary: View changes

Hmm. Looks like we broke a couple tests, having removed system.module.css and system.theme.css. They didn't fail because they are doing assertNoText.

I guess we could do that in another follow up.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2552187-6.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Status: Needs work » Reviewed & tested by the community

The failures here are another case of #2552687: Test failures in ConfigFormOverrideTest and ContainerRebuildWebTest on newly spun up testbot instances.

Wim Leers queued 6: 2552187-6.patch for re-testing.

alexpott’s picture

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

Committed 55f6f42 and pushed to 8.0.x. Thanks!

  • alexpott committed 55f6f42 on 8.0.x 
    Issue #2552187 by Wim Leers, LewisNyman: Follow-up for #2395853:...

Status: Fixed » Closed (fixed)

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