Problem/Motivation

Bartik uses ckeditor-iframe.css to load CKEditor styles when Bartik is the admin theme, but it only includes the styles for the caption.
Instead we could load Bartik's frontend CSS stylesheets in the CKEditor. This would be simpler and easier to maintain as it is loading stylesheets for the frontend and we will not have two sets of styles to upkeep.

Proposed resolution

Remove ckeditor-iframe.css and replace with applicable Bartik CSS stylesheets.

Remaining tasks

Wait until #2375673: Split Bartik's CSS into SMACSS style components is committed.
Un-postpone this issue.
Write a patch replacing the loaded CSS files in bartik.info.yml.
Test patch in CKEditor in Bartik.
Add test screenshots.
Review patch.
Add review screenshots.

User interface changes

API changes

Original report by @Wim Leers

Reported by Dharmesh Mistry.

Bartik should ship with a CKEditor stylesheet, so that sites using Bartik (all default Drupal 8 installations) have as close a match as possible between what is seen in CKEditor on the node add/edit form and what is seen on the front-end.

In bartik.info.yml:

ckeditor_stylesheets:
  - css/ckeditor-iframe.css

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task as CKEditor functions fine as it is but it lacks styles to show how elements are represented on the frontend.
Unfrozen changes Unfrozen because it only changes which CSS files are used to show visual styles in CKEditor
CommentFileSizeAuthor
#9 remove-ckeditoriframe-css-2064379-9.patch407 bytestadityar
#10 remove-ckeditoriframe-css-2064379-10.patch1.34 KBtadityar
#13 remove-ckeditoriframe-css-2064379-13.patch2.22 KBtadityar
#16 remove-ckeditoriframe-css-2064379-16.patch2.4 KBtadityar
#18 remove-ckeditoriframe-css-2064379-18.patch2.88 KBtadityar
#18 interdiff-13-18.txt1.72 KBtadityar
#28 2064379-27-frontend.png1.35 MBbneil
#28 2064379-27-WYSIWYG-before.png1.21 MBbneil
#28 2064379-27-WYSIWYG-after1.png1.19 MBbneil
#28 2064379-27-WYSIWYG-after2.png1.16 MBbneil
#24 interdiff-18-23.txt1.17 KBtadityar
#24 remove-ckeditoriframe-css-2064379-23.patch2.77 KBtadityar
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#2027181: Use a CKEditor Widget to create a stellar UX for captioning and aligning images added core/themes/bartik/css/ckeditor-iframe.css, but it only includes the styles for the caption.

I just thought of something: is there any reason why we couldn't just load Bartik's entire style.css? That'd be heavily overkill, but it'd also be simpler, and easier to maintain.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue summary: View changes

Updated issue summary.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes
Status: Active » Postponed

We are currently in the process of splitting styles.css up into SMACCS style stylesheets #2375673: Split Bartik's CSS into SMACSS style components.
Once this change has been committed I agree with @Wim Leers that we should load Bartik frontend CSS files and take out ckeditor-iframe.css.
We will be able to pick and choose which CSS files we need as everything will be split into components and having one version of the CSS for frontend and the Bartik CKEditor backend will be easier to maintain.
We also will not need ckeditor-iframe.css as captions.css file has been created as part of #2375673: Split Bartik's CSS into SMACSS style components and we can load that with the other relevant files.

Currently postponing the issue as we are working hard on #2375673: Split Bartik's CSS into SMACSS style components and any work done on this now will not be a good use of time.
I also have updated the issue summary to represent better what we hope to achieve.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes
emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

Agreed — and awesome! :) Can't wait to review a patch :)

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Status: Postponed » Active

And I officially declare this issue active again now that #2375673: Split Bartik's CSS into SMACSS style components has been committed :)

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Title: Bartik should provide a stylesheet for CKEditor's iframe mode » Replace ckeditor-iframe.css with relevant Bartik CSS for CKEditor's iframe mode.
Issue tags: +CSS, +frontend
emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Title: Replace ckeditor-iframe.css with relevant Bartik CSS for CKEditor's iframe mode. » Remove ckeditor-iframe.css and load relevant Bartik CSS files for CKEditor's iframe mode.
tadityar’s picture

tadityar CreditAttribution: tadityar commented
Status: Active » Needs review
FileSize
remove-ckeditoriframe-css-2064379-9.patch407 bytes
tadityar’s picture

tadityar CreditAttribution: tadityar commented
FileSize
remove-ckeditoriframe-css-2064379-10.patch1.34 KB

forgot to add the deleted file.. sorry

The last submitted patch, 9: remove-ckeditoriframe-css-2064379-9.patch, failed testing.

tadityar’s picture

tadityar CreditAttribution: tadityar commented
tadityar’s picture

tadityar CreditAttribution: tadityar commented
FileSize
remove-ckeditoriframe-css-2064379-13.patch2.22 KB

Seems like one more file needed to be changed.

The last submitted patch, 10: remove-ckeditoriframe-css-2064379-10.patch, failed testing.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Status: Needs review » Needs work

Good start, but we need to add more than just captions.css, as suggested in #1 and then adapted for the new Bartik file structure in #2.

Include as many files as you can see that provide Bartik styles to elements that a user could include in the ckeditor. So basic element styling for text, tables etc etc.

tadityar’s picture

tadityar CreditAttribution: tadityar commented
Status: Needs work » Needs review
FileSize
remove-ckeditoriframe-css-2064379-16.patch2.4 KB

still not sure what's more to add..

Status: Needs review » Needs work

The last submitted patch, 16: remove-ckeditoriframe-css-2064379-16.patch, failed testing.

tadityar’s picture

tadityar CreditAttribution: tadityar commented
Status: Needs work » Needs review
FileSize
remove-ckeditoriframe-css-2064379-18.patch2.88 KB
interdiff-13-18.txt1.72 KB

updating the test to see if every stylesheets needs to be included there

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Needs work

Looking good!

  1. +++ b/core/themes/bartik/bartik.info.yml
@@ -8,7 +8,13 @@ core: 8.x
+  - css/components/buttons.css

    buttons.css is unnecessary IMHO; content should never contain buttons. 

  2. +++ b/core/themes/bartik/bartik.info.yml
@@ -8,7 +8,13 @@ core: 8.x
+  - css/components/content.css

    Might as well have been called node.css, because it contains node-related styling. Irrelevant inside CKEditor. Actually, if there's an entity reference that we render inside CKEditor (in a CKEditor Widget), then this *would* be useful! But if we have this, then we'll also want user.css.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue tags: +Novice
Related issues: +#2398463: Clean up the "content" component in Bartik

@Wim Leers Agree with renaming content.css to node.css. We have this issue #2398463: Clean up the "content" component in Bartik for cleaning up content.css and it was suggested to change the file name to node.css also over there. We will do that over there with all the CSS/template work for content/node.

So we only need to implement 1. from #19 in this issue.

tadityar’s picture

tadityar CreditAttribution: tadityar commented

Thank you for the review! I'll update my patch in #18 when I get home.
Btw, whoever that do this #2398463: Clean up the "content" component in Bartik will replace all occurences of content.css with node.css right? (So I don't have to worry someone might have to re-do this)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

Indeed; you don't have to worry about content.css. :)

tadityar’s picture

tadityar CreditAttribution: tadityar commented
Status: Needs work » Needs review

Ok, added suggestion from #19.1. I'll leave 19.2 for the other issue :)

tadityar’s picture

tadityar CreditAttribution: tadityar commented
FileSize
interdiff-18-23.txt1.17 KB
remove-ckeditoriframe-css-2064379-23.patch2.77 KB

Oh my form got outdated.. the real patch now.

bneil’s picture

bneil CreditAttribution: bneil commented
Status: Needs review » Reviewed & tested by the community

Patch from comment #24 applies successfully. I manually tested the patch by creating a node and testing out various HTML elements and styles in the WYSIWYG and cross referencing them with viewing the saved node.

Setting to RTBC as I believe the patch satisfies the issue summary and the css it loads is the pertinent css files from Bartik.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +Quickfix, +SprintWeekend2015

RTBC +1, all my remarks from #19 have been addressed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs screenshots

Would it be possible to get before/after screenshots here, please?

bneil’s picture

bneil CreditAttribution: bneil commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots
FileSize
2064379-27-WYSIWYG-after2.png1.16 MB
2064379-27-WYSIWYG-after1.png1.19 MB
2064379-27-WYSIWYG-before.png1.21 MB
2064379-27-frontend.png1.35 MB

Here are screenshots of a basic page of content using the default set of button configuration for the Full HTML text format.

Frontend - Bartik

CKEditor - Before patch

CKEditor - After patch

I had to split this up into two screenshots, due to some of the text spacing changes. CKEditor wouldn't expand long enough for my test content. Too many cat photos?

Summary of changes

The following changes are visibly more consistent with the frontend Bartik theme:

  • General font styles
  • Block quote
  • Table styles
  • Heading styles
alexpott’s picture

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

Theme changes are not blocked by beta. Committed 8b9b0d7 and pushed to 8.0.x. Thanks!

  • alexpott committed 8b9b0d7 on 8.0.x 
    Issue #2064379 by tadityar, bneil: Remove ckeditor-iframe.css and load...

Status: Fixed » Closed (fixed)

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

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented

Finally tracked this issue down to here.

I noticed that after this patch came in the editor lost the nice 8px margin that it once had. But now I fear that we've lost even more.

Can you please explain to me how a developer can control the styles that appear in the text editor in such a way that doesn't break the styles that appear when rendered.

Historically, having styles that work in the text editor AND the markup that it produces have been a terribly difficult thing to master. I had been hopeful that having a designated stylesheet would make it easy for front end developers to place their editor styles into a file and be done with it. Now it sounds like one would need to have knowledge of Drupal's internal handling of styles in order to accomplish the same result.

Can you please point me to a reference implementation of how to do this the right way now? Does the ckeditor_stylesheets directive still work as advertised? Is there are good reason why we're overriding the 8px margin inside the editor?