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

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2552187-2.patch902 bytes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
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 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 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 2552187-6.patch1.56 KB

Indeed!

Done.

lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +frontend, +CSS
StatusFileSize
new 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
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 🇧🇪🇪🇺
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 🇪🇺🌍
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.