Seven theme: style CKEditor-native dialogs to match Drupal-native dialogs

+1, makes sense. Are we able to modify the mark up for this dialog or do we have to duplicate styling?

Let's do this.

The variable (dialog template) that you mentioned Wim is private, so in order to make it overridable, we would need to move it to some public object. We could do that without any trouble, but... :D

I don't think that playing with the dialog system's underlaying DOM structure will be safe and useful. I've never worked a lot with this part of CKEditor, but I can already think of few reasons:

• The dialog system didn't even lay close to the MVC pattern, so there's no view layer separation. The template you found is the only so big chunk of HTML I found - the rest is scattered over the entire dialog and dialogui plugins. It wouldn't be possible now to make all them overridable.
• Some dialogs may depend on specific DOM structures, so changing them might break those dialogs. Also, where CSS's powers were too weak, some JS might be used.
• The dialog system originally supported IE6+ and while we got rid of the code that was directly related to those old browsers the major design decisions remained. For instance, the whole system is based on tables and if you don't want them... well :D.
• Any changes to the templates will need to be maintained in the future. We still improve dialogs accessibility or add new features.

Of course, I assumed that you want to change all the templates. If you just want to change the main one the situation may be much simpler, but is it still worth it? Therefore, I would change the stylesheets only to make the CKEditor's components resemble Drupal's ones.

I don't know what this means exactly. I'd think we'd only want to modify the wrapper markup, i.e. the "skeleton" of the dialog, not the "filler" of each dialog.

Ah, ok. I thought that you might want to modify the contents, because that's theoretically also doable. Dialog definitions do not (usually) contain any markup, but they use CKEditor's ui components definitions and these components can be overridden. However, that's doable only theoretically because of no view layer separation and thus huge amount of work.

As for the wrapper, if you'll find it hard to achieve some result without changing the skeleton, I don't see a problem in exposing (by the dialog plugin) the skeleton's template.

How hard is this to change?

@wim could you provde some guidance?

We don't have any documentation for styling dialogs. We have only a generic documentation regarding skins and creating a new skin: http://docs.ckeditor.com/#!/guide/skin_sdk_intro. I think it should be enough for you because it's just a CSS. I would propose two approaches:

• Overriding the default skin without touching the original files. This will make future upgrades super easy, but may be a bit cumbersome (because it's CSS...).
• Forking the original skin and making your modifications in dialog.css. This will allow for deeper modifications and will be more optimal in terms of the code size, but future upgrades will be trickier.

So it boils down to the question of how much you'll need to change the skin.

I can do it if it's not a critical turnaround, would probably be tomorrow or at the latest over the weekend.

Will try and move this forward. This is a first approach for the table plugin, which shows a CKEditor dialog. The "Source" button, which shows a CKEditor dialog when using CKEditor in in-place editing takes the current theme's styling so in this case it should be added at classy (tagging accordingly).

@Wim Leers: should I move on and do the same for classy?

+  background: url(/core/misc/icons/ffffff/ex.svg) 0 0 no-repeat;

This and all other urls like that will not work if drupal is in a folder.

@Wim Leers for the first one, I tried to stay inline with /core/assets/vendor/jquery.ui/themes/base/theme.css which still uses this, but yeah probably it's not needed anymore. The second one is extreme to have it in a CKEditor dialog, indeed.

@tic2000 I tried to stay inline with /core/themes/seven/css/components/dialog.css. I know that this will happen, but maybe it's a matter we should discuss for theming in general as it's used in core themes already.

New patch after #20 and #22 suggestions :)

Should these be added not with a library, but using the ckeditor settings in the info file, or is that deprecated?

Bartik still has it...

ckeditor_stylesheets:
  - css/base/elements.css
  - css/components/captions.css
  - css/components/table.css

Regarding Classy, I'm not sure that is the appropriate place for this. Classy, as far as I can tell, does not have any ckeditor specific styling. It only has the more generic dialog css, and extends core's dialog library, not the ckeditor one. If the argument is that all dialogs, regardless of implementation should have this styling, is there a way we can have quickedit load the library?

Sorry, didn't mean to ember those images.

Ah, ok. Good to know. (I should probably know these things. :P )

#27: Neither Classy nor Seven have CKEditor specific theming. It's about dialog's styling and unfortunately it's not just including the one that's created for Seven in Classy cause it has different theming.

I have to note here that even if we move on with classy too, if someone creates his own theming without classy as a base theme, it's inevitable to have different theming in Quick edit between links, images and ckeditor's modals (as it has different markup)

What is the argument against putting this in the ckeditor module?

I see. So we need this styling in Classy...

and this in Seven...

Yeap, exactly. The thing is that for the administration, where most people use Seven things are a bit easier, but for the quick edit part things can be trickier as it's depending on the current theme (in simple case, that's Classy).

Classy is fine. That base styling is there, not in Stable or core. Themes not using Classy are on their own. That is the point of sensible defaults versus not. We have to draw a line somewhere.

Thanks to @bojhan, following up this in DrupalCon Asia...

Ported the Seven dialog style to bartik theme as the dialog in "quick edit" mode is still using classy.

Updated in Bartik:

Would be great if anyone could do a review on this?

Current state in bartik:

I don't think the issue here is to change other themes' already styled dialogs, but make the ckeditor's core modals look like the drupal's. Maybe it would be better to create another issue if we think this is necessary for quick editor

Earlier patch failed, just updated patch file to pass testing.

@darketaine From WimLeers, David and a conversation with Lauri at DrupalconAsia - it is my understanding is that we do wish to change Bartik. We do not wish to adjust the standard look of CKeditor. This because the "raw" output of core, should not contain any opionated styling and just be purely functional.

@ironkiat can you please post the interdiff 35-41? It is always helpful to view the differences between patches and review them.

@Bojhan But Bartik is just a theme. If you change it with a custom theme for example the dialogs will stay not-themed again. The issue is to make Ckeditor's dialogs look like Drupal's in Seven and -maybe- Classy which is a base theme. Of course none said core's theme should be changed. Classy is only under discussion cause it has a minimum styling in dialogs which is different from CKEditor's. Bartik and any other -even core- theme is not in this issue's scope :/
That's what I get at least from the description of the issue

Anyway let's see what Wim Leers and Cottser think about it :)

@thpoul It's the comment 39 (the current state ie. comment 35) and the 41 (in the updated bartik).

@ironkiat oops, I misspelled 25 to 35, sorry about that. I meant the interdiff of patch 2090937-25.patch and 2090937-42.patch. Take a look here.

Thanks @thpoul! I've uploaded the interdiff file here ;)

I haven't read all the comments since I've been assigned but @Bojhan caught me up. I agree this should not go into Classy (or Stable for that matter) because Classy doesn't provide visual styling.

As for screenshots and manual testing I think we should also be covering when Bartik is used for content entry ("Use the administration theme when editing or creating content" unchecked on /admin/appearance).

Classy has a minimum styling (the one with the white header) though. That was my argument. Anyway I thought the issue was about styling the ckeditor dialogs like Drupal's only, not even changing the existing Drupal's dialogs look like Seven's (that's what the last patch does) but I guess I got it wrong :)

Btw #41 patch overrides completely all Bartik's dialogs even when "Use the administration theme when editing or creating content" is not checked to Seven's theming.

The fact that when you check the option Seven's theming doesn't appear instead of Bartik's in Quick Edit mode is another issue I think, not relevant to ckeditor's here. If that option worked like that it would take Seven's theme (which is changed for ckeditor from previous patches) and wouldn't need any more CSS. In that way any changes that would take place in Seven's dialogs for example would have immediate effect on Bartik's (and other theme's) quick edit too, not needing to duplicate CSS.

Yes it doesn't check whether is Administration theme being use to create content and it's meant to override it for all Bartik's dialog.
I'm not too sure right now what's the direction of this but I think it make sense that when "Use the Administration theme when creating content" is checked, it uses Seven's styling, or it is limited to just the content creation screens, which I could possibly make it more specific in the CSS.

I would suggest to wait for Wim Leers who is the reporter and maintainer of the relative component.

Classy provides the minimal styling in the first image I posted in #34. It is in the dialog.css.

@WimLeers he he, it's not about giving up or getting confused but pushing things to the right -for the issue- direction.

For the other issue that came up I don't think it's even a Bartik's problem, but a decision about how we want the "Use the Administration theme when creating content" option to work in any theme. If that option checked has effect on Quick Editor too, then the work done here will have instant effect on any theme's ckeditor modals too. So the solution would be to just include the right CSS files in that case, not duplicating the relative CSS in any theme.

Sorry if I contributed to the confusion!

Overall this looks very good! Before really reviewing the CSS I figured it would make sense to manually test and review the visual appearance.

I spotted a couple things that are a little bit off compared to the native dialogs and should be fixed:

The buttons are 18px high, should be 17px to be consistent with Seven's other buttons:

The typography of the dialog's title is a bit off, in the following screenshot the Drupal native dialog with the title renamed via inspector can be seen on the left. This affects the height as well:

Fixed height of button, but not clear for me what should i do with typography? Could you describe the task in more detail and would be great to see another screens as well. Thanks

button before:
button after:

@kostyashupenko the problem is not on the padding (which -after #63 patch- causes the button to be smaller than Seven's), it's on the rendering of the font in both cases (title and button).

@Cottser I will fix it asap. Thanx for the -first phase of- review :)

And here it is. Changed font-size and line-height on both cases.

Going to leave it at RTBC and review later but the CSS file should have a @file docblock or at least a docblock at the top. Forgot to mention that yesterday.

Oh I'm sorry, I totally forgot that.

Thanks :)

On my list to review.

Sorry for the long turnaround on this review, I was sick for a while. I think once 1 (and maybe 2) below are addressed this is ready to go into 8.2.x and 8.1.x after beta2 is cut.

1. I think .cke_dialog_contents needs:

   border-bottom-left-radius: 5px;
   border-bottom-right-radius: 5px;
   

2. I could go either way on this (it's a very slight difference so I would be fine committing without this change) but we could add a bit more precision to the font-size in at least one case:

   .cke_reset_all .cke_dialog_title could use a font-size of 1.3344em (one digit more precise than the 1.334em in #70) so that the computed font-size value matches that of .ui-dialog .ui-dialog-title.

The only other things I can think of are that we could disable the CKEditor moving functionality (where you can move the modal around by dragging the title bar) and just in general handle the positioning and such more like the core dialog (for example scroll the contents of the dialog when the viewport isn't tall enough to show it all), but:

• I'm not sure we even want to do either of these things.
• If we do decide we want to pursue either or both of these, we should do so in a separate issue after this is in.

edited as per #73

@himanshugautam interdiff please!

Thanks @himanshugautam. I also just realized I haven't yet reviewed the code itself thoroughly, so I will still work on that :)

Since this is a user interface change, unfortunately I think it actually is too late for 8.1.x now that we are well into the beta phase per https://www.drupal.org/core/d8-allowed-changes#minor and the fact that this is an improvement, not a bug fix. Bumping to 8.2.x (where the patch still applies).

Sorry for dropping off, I got lost in the land of resetting up my local dev environment post computer wipe, thanks to everyone who pushed it through in a timely manner anyway. :)

Putting on my list, don't want to forget about this one :)

I love the attention to detail here :)

Apologies for being picky but from looking through the code I've noticed some small CSS formatting standard nits...

1. +++ b/core/themes/seven/css/theme/ckeditor-dialog.css
   @@ -0,0 +1,202 @@
   +  padding: .3em .4em .3em .5em;
   ...
   +  box-shadow: inset 0 1px 2px rgba(0,0,0,.125);
   ...
   +  box-shadow: inset 0 1px 3px rgba(0,0,0,.05),0 0 8px #40b6ff;
   

   A few decimal values within the CSS are missing leading zeros.

2. +++ b/core/themes/seven/css/theme/ckeditor-dialog.css
   @@ -0,0 +1,202 @@
   +/**
   + * @file
   + * CKEditor-native dialogs theming.
   + */
   +.cke_dialog_background_cover {
   

   The file comment needs a blank line after it as per the Drupal CSS file comment standards...

   Note that a blank line should follow a file comment. And keep line-lengths to 80 columns, when possible. For more information, see the PHP file comment standards.

Thanks @emma.maria appreciate it :)

Here you go as per #83

@emma.maria I saw that core/themes/seven/css/components/form.css has the same issue with leading zeros. Look at textarea.form-textarea. Perhaps we should fix that in an other issue?

Switching to NR. Sorry for the noise.

Sorry for the review-in-many-parts, grabbing minutes when I can here and there.

The biggest thing I noticed with this review is after finally running this through http://csslint.net it pulled out an overly specific selector:

.cke_reset_all a.cke_dialog_close_button

It wanted this to be more like (remove the a element from the selector):

.cke_reset_all .cke_dialog_close_button

In doing so I noticed that CKEditor has LTR and RTL styles for the positioning of the close modal button (X). Nobody has tested RTL so far!

So I think we need to:

1. Listen to CSSLint and make the selector less specific (remove a).
2. Add an RTL style to position the X appropriately.

To test RTL with the table plugin, I:

1. Installed the Language and Interface Translation modules
2. Added the Arabic language
3. Set Arabic as the default language

And some nits below.

1. +++ b/core/themes/seven/css/theme/ckeditor-dialog.css
   @@ -0,0 +1,203 @@
   +  box-shadow: inset 0 1px 2px rgba(0,0,0,0.125);
   +  -webkit-transition: border linear 0.2s,box-shadow linear 0.2s;
   +  transition: border linear 0.2s,box-shadow linear 0.2s;
   ...
   +  box-shadow: inset 0 1px 3px rgba(0,0,0,0.05),0 0 8px #40b6ff;
   ...
   +  background-image: -webkit-linear-gradient(top,#f6f6f3,#e7e7df);
   +  background-image: linear-gradient(to bottom,#f6f6f3,#e7e7df);
   ...
   +  text-shadow: 0 1px hsla(0,0%,100%,0.6);
   ...
   +  background-image: -webkit-linear-gradient(top,#fcfcfa,#e9e9dd);
   +  background-image: linear-gradient(to bottom,#fcfcfa,#e9e9dd);
   ...
   +  box-shadow: 0 1px 2px hsla(0,0%,0%,0.125)
   ...
   +  box-shadow: 0 0 0.5em 0.1em hsla(203,100%,60%,0.7);
   ...
   +  background-image: -webkit-linear-gradient(top,#f6f6f3,#e7e7df);
   +  background-image: linear-gradient(to bottom,#f6f6f3,#e7e7df);
   +  box-shadow: inset 0 1px 3px hsla(0,0%,0%,0.2);
   ...
   +  text-shadow: 0 1px hsla(0,0%,100%,0.6);
   ...
   +  background-image: -webkit-linear-gradient(top,#007bc6,#0071b8);
   +  background-image: linear-gradient(to bottom,#007bc6,#0071b8);
   ...
   +  text-shadow: 0 1px hsla(0,0%,0%,0.5);
   ...
   +  background-image: -webkit-linear-gradient(top,#0c97ed,#1f86c7);
   +  background-image: linear-gradient(to bottom,#0c97ed,#1f86c7);
   ...
   +  box-shadow: 0 1px 2px hsla(203,10%,10%,0.25);
   ...
   +  box-shadow: 0 0 0.5em 0.1em hsla(203,100%,60%,0.7);
   ...
   +  background-image: -webkit-linear-gradient(top,#08639b,#0071b8);
   +  background-image: linear-gradient(to bottom,#08639b,#0071b8);
   ...
   +  box-shadow: inset 0 1px 3px hsla(0,0%,0%,0.2);
   ...
   +  text-shadow: 0 1px hsla(0,0%,0%,0.5);
   

   While we're picking nits all of the commas on the lines I've selected need a space after them per https://www.drupal.org/node/1887862#properties.

2. +++ b/core/themes/seven/css/theme/ckeditor-dialog.css
   @@ -0,0 +1,203 @@
   +  border: 1px solid #1280DF;
   

   This hex value should be lowercase per https://www.drupal.org/node/1887862#properties.

Here is the updated patch updated as per #88. Nits were fixed and RTL support has been added.

A note on why .cke_rtl has been chosen instead of [dir="rtl"]:
If someone skips the installation of the Interface Translation module then CKEditor will not have RTL support (I was quite surprised to see this) and if [dir="rtl"] is applied the table dialog will look like this:

which is wrong since all the CKEditor dialogs should follow the same theming rules (check the image dialog for example and you will see that it has complete styling.

if .cke_rtl is used instead then we will have the following:

.

After reconsidering the logic above, if a user does not activate the Interface Translation module the dialog will look like the following screenshot:

.

Activating the Interface Translation module will result in the following which should be the expected one:

.

I really wish to see this getting moved forward.

PS: Major credit of this should go to @darketaine who was the one pointing me the RTL css rules.

Thanks, will review again soon :)

The patch is green and still applies so I'm setting this back to RTBC *tiptoes away*

Thanks everyone. We are getting closer!

#73.1 (bottom corner radii) has not been fixed - there is a specificity issue because I think the CKEditor skin dialog.css is loaded after the ckeditor-dialog.css being added here.

I also tested on a high density screen and found that CKEditor has specific styles for high density displays (screenshot below), we'll need to add to our selector for the close icon and potentially others (refresh, lock, unlock - not sure if those would be used in our case). I think we need to take a good look at the CKEditor moono theme dialog.css to see what else we may need to account for.

Uploading the missing screenshot.

After @Cottser's recommendations in #95 I checked out CKEditor moono theme dialog.css and apart from high density displays there were lots of overrides from high contrast displays selectors. Taking that into account here are some more specific selectors (that's a bit ugly :/ )

Hope this is in the right direction :)

Thanks, will review as soon as I can!

Other things have been getting in the way but this is at the top of my list to review :)

This looks great, I ran through a bunch of tests again and can't find any faults with the code or visuals.

My only other thought at this time is whether we should be doing some grouping (and possible commenting) of the CSS rules per "Blank lines" in https://www.drupal.org/node/1887862#whitespace. It's relatively minor but since we're adding a new ~250 line CSS file I thought I would bring it up.

I 'll give this a try.
I hope comments are in the right direction, I tried to not overdo it.

Thanks to everyone who worked on this especially @darketaine for sticking with us :) so happy to be committing this.

Committed 21c135f and pushed to 8.2.x. Thanks! :D

Great work!

This has traditionally been a big enough annoyance that it might be worth explicitly calling out in the release notes.

• CKEditors dialogs are not ugly anymore.

Thanks a lot .. was waiting for this for so long.
We need to find a way to let other admin themes use this library.

I will try adding this in some other admin themes.

 libraries-extend:
  core/ckeditor:
    - seven/ckeditor-dialog

And this :

ckeditor-dialog:
  version: VERSION
  css:
    theme:
      css/theme/ckeditor-dialog.css: {}

Rewarded work :)