Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DLLs] DocumentListProperties' useAttribute is ignored due to styles in list.css #14613

Open
sbt99 opened this issue Jul 19, 2023 · 5 comments
Open

[DLLs] DocumentListProperties' useAttribute is ignored due to styles in list.css #14613

sbt99 opened this issue Jul 19, 2023 · 5 comments
Labels
domain:dx This issue reports a developer experience problem or possible improvement. domain:integration-dx This issue reports a problem with the developer experience when integrating CKEditor into a system. package:list squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@sbt99
Copy link

sbt99 commented Jul 19, 2023 •
edited

📝 Provide detailed reproduction steps (if any)

  1. Generate a package using https://ckeditor.com/docs/ckeditor5/latest/framework/plugins/package-generator/using-package-generator.html
  2. In the dll.html file of the generated package add the CKEditor5.list.DocumentList and CKEditor5.list.DocumentListProperties inside the plugins array where the editor is initialized (see full file posted below)
  3. Add the configuration for the list plugin as of follows:
list: {
  properties: {
    styles: {
      useAttribute: true
    },
      startIndex: true,
      reversed: true
  }
}
  1. Open the dll.html file in a browser

✔️ Expected result

Selecting a list style will also apply the corresponding list icon of the selected style to the list.

❌ Actual result

The corresponding list icon of the selected list style is not applied to the list inside the editor. This happens due to the styles in list.css which is included unconditionally and thus overrides the type attributes default style applied by the browser.

❓ Possible solution

  • Add a class to the <ul> and <ol> tags if the useAttribute config is true and adapt the CSS in list.css such that the styles won't apply if that class exists.
  • Include the list.css file only if the useAttribute config isn't set or false.

📃 Other details

Full dll.html file that reveals the problem when adding to a generated package and opening it in a browser:

<!DOCTYPE html>
<html lang="en">
<head>
    <link rel="icon" type="image/png" href="https://ckeditor.com/docs/ckeditor5/latest/assets/img/favicons/32x32.png" sizes="32x32">
    <meta charset="utf-8">
    <title>CKEditor 5 – DLL Sample</title>
    <style>
        body {
            max-width: 800px;
            margin: 20px auto;
        }
    </style>
</head>
<body>

<h1>CKEditor 5 – DLL Sample</h1>

<div id="editor">
    <h2>Production sample</h2>
    <p>
        This is a demo of the <a href="https://ckeditor.com/docs/ckeditor5/latest/builds/guides/overview.html#classic-editor">classic editor
        build</a>, initialized using the <a href="https://ckeditor.com/docs/ckeditor5/latest/builds/guides/development/dll-builds.html">DLL builds</a>.
    </p>
    <p>
        Your plugin (Listtest) generated by the tool is already loaded into the editor. By default, it has an example button that adds some text to the editor. Whenever you change the plugin's name or toolbar items, make sure to update the editor configuration in the <code>sample/dll.html</code> file.
    </p>

	<h3>Uncaught TypeError</h3>
    <p>If the editor is not initialized correctly and the browser console displays an error such as the following:</p>
    <pre><code>Uncaught TypeError: Cannot read properties of undefined (reading 'Listtest') at dll.html:85</code></pre>
    <p>it means that the DLL build of the <code>@ckeditor5/ckeditor5-listtest</code> package has not been created.</p>
    <p>Please call the <code>yarn run dll:build</code> command in the CLI, and after it finishes, refresh the page.</p>

    <h3>Anatomy of the DLL build</h3>
    <p>The source of the DLL build is located in the <code>src/index.js</code> file. It may export as many things as the package offers.</p>

    <h4>Maintaining the sample</h4>
    <p>Whenever you change objects exported by the DLL build, please review the content of the sample. Things to keep in mind:</p>
    <ul>
        <li>Review the list of loaded plugins in the configuration.</li>
        <li>Review names of items registered in toolbars.</li>
    </ul>

    <h3>The goal</h3>
    <p>The primary purpose of the sample is to verify whether the plugins in the package will work together with CKEditor 5 created with the DLL approach.</p>

    <h3>Publishing the package</h3>
    <p>
        By default, the <code>build/</code> directory is specified in the <a href="https://docs.npmjs.com/cli/v7/configuring-npm/package-json#files"><code>#files</code></a>
        array in the <a href="../package.json"><code>package.json</code></a> file. It means all its contents will be published when calling the <code>yarn publish</code> command.
    </p>
    <p>
        Unfortunately, it is easy to forget to refresh the DLL build before making a new release. Hence, we created the <a href="https://docs.npmjs.com/cli/v7/using-npm/scripts#pre--post-scripts">prescript</a> that would refresh the build automatically when calling the <code>yarn publish</code> command.
    </p>

    <h3>Reporting issues</h3>
    <p>If you found a problem with CKEditor 5 or the package generator, please, report an issue:</p>
    <ul>
        <li><a href="https://github.com/ckeditor/ckeditor5/issues/new/choose">CKEditor 5</a></li>
        <li><a href="https://github.com/ckeditor/create-ckeditor5-plugin/issues/new">The package generator</a></li>
    </ul>
</div>

<!-- DLL builds are served from the `node_modules/` directory -->
<script src="../node_modules/ckeditor5/build/ckeditor5-dll.js"></script>
<script src="../node_modules/@ckeditor/ckeditor5-editor-classic/build/editor-classic.js"></script>
<script src="../node_modules/@ckeditor/ckeditor5-code-block/build/code-block.js"></script>
<script src="../node_modules/@ckeditor/ckeditor5-essentials/build/essentials.js"></script>
<script src="../node_modules/@ckeditor/ckeditor5-basic-styles/build/basic-styles.js"></script>
<script src="../node_modules/@ckeditor/ckeditor5-heading/build/heading.js"></script>
<script src="../node_modules/@ckeditor/ckeditor5-autoformat/build/autoformat.js"></script>
<script src="../node_modules/@ckeditor/ckeditor5-block-quote/build/block-quote.js"></script>
<script src="../node_modules/@ckeditor/ckeditor5-image/build/image.js"></script>
<script src="../node_modules/@ckeditor/ckeditor5-link/build/link.js"></script>
<script src="../node_modules/@ckeditor/ckeditor5-indent/build/indent.js"></script>
<script src="../node_modules/@ckeditor/ckeditor5-media-embed/build/media-embed.js"></script>
<script src="../node_modules/@ckeditor/ckeditor5-list/build/list.js"></script>
<script src="../node_modules/@ckeditor/ckeditor5-table/build/table.js"></script>

<!-- The "@ckeditor5/ckeditor5-listtest" package DLL build is served from the `build/` directory -->
<script src="../build/listtest.js"></script>

<script>
	console.log( 'Objects exported by the DLL build:' );

	CKEditor5.editorClassic.ClassicEditor
		.create( document.querySelector( '#editor' ), {
			plugins: [
				CKEditor5.essentials.Essentials,
				CKEditor5.autoformat.Autoformat,
				CKEditor5.blockQuote.BlockQuote,
				CKEditor5.basicStyles.Bold,
				CKEditor5.heading.Heading,
				CKEditor5.image.Image,
				CKEditor5.image.ImageCaption,
				CKEditor5.image.ImageStyle,
				CKEditor5.image.ImageToolbar,
				CKEditor5.image.ImageUpload,
				CKEditor5.indent.Indent,
				CKEditor5.basicStyles.Italic,
				CKEditor5.link.Link,
				// CKEditor5.list.List,
				// CKEditor5.list.ListProperties,
				CKEditor5.list.DocumentList,
				CKEditor5.list.DocumentListProperties,
				CKEditor5.mediaEmbed.MediaEmbed,
				CKEditor5.paragraph.Paragraph,
				CKEditor5.table.Table,
				CKEditor5.table.TableToolbar,
				CKEditor5.codeBlock.CodeBlock,
				CKEditor5.basicStyles.Code,
				CKEditor5.upload.Base64UploadAdapter
			],
			toolbar: [
				'listtestButton',
				'|',
				'heading',
				'|',
				'bold',
				'italic',
				'link',
				'code',
				'bulletedList',
				'numberedList',
				'|',
				'outdent',
				'indent',
				'|',
				'uploadImage',
				'blockQuote',
				'insertTable',
				'mediaEmbed',
				'codeBlock',
				'|',
				'undo',
				'redo'
			],
			image: {
				toolbar: [
					'imageStyle:inline',
					'imageStyle:block',
					'imageStyle:side',
					'|',
					'imageTextAlternative'
				]
			},
			table: {
				contentToolbar: [
					'tableColumn',
					'tableRow',
					'mergeTableCells'
				]
			},
			list: {
				properties: {
					styles: {
						useAttribute: true
					},
					startIndex: true,
					reversed: true
				}
			}
		} )
		.then( editor => {
			window.editor = editor;
		} )
		.catch( error => {
			console.error( 'There was a problem initializing the editor.', error );
		} );
</script>
</body>
</html>

If you'd like to see this fixed sooner, add a 👍 reaction to this post.
@sbt99 sbt99 added the type:bug This issue reports a buggy (incorrect) behavior. label Jul 19, 2023
@Witoso Witoso added package:list squad:core Issue to be handled by the Core team. labels Jul 19, 2023
@Witoso Witoso changed the title DocumentListProperties' useAttribute is ignored due to styles in list.css [DLLs] DocumentListProperties' useAttribute is ignored due to styles in list.css Jul 19, 2023
@Witoso Witoso added domain:dx This issue reports a developer experience problem or possible improvement. domain:integration-dx This issue reports a problem with the developer experience when integrating CKEditor into a system. labels Jul 19, 2023
@sbt99
Copy link
Author

sbt99 commented Jul 21, 2023

I think making the following changes inside the ckeditor5-list plugin could fix this issue:

documentlistpropertiesediting.js

...
            setAttributeOnDowncast( writer, listStyle, element ) {
                if ( listStyle && listStyle !== DEFAULT_LIST_TYPE ) {
                    if ( useAttribute ) {
                        const value = getTypeAttributeFromListStyleType( listStyle );

                        if ( value ) {
                            writer.setAttribute('class', 'ckeditor-list-type-attribute', element);
                            writer.setAttribute( 'type', value, element );
                            return;
                        }
                    } else {
                        writer.setAttribute('class', 'ckeditor-list-type-style', element);
                        writer.setStyle( 'list-style-type', listStyle, element );
                        return;
                    }
                }

                writer.removeStyle( 'list-style-type', element );
                writer.removeAttribute( 'type', element );
            },
...

list.css

.ck-content ol.ckeditor-list-type-style {
    list-style-type: decimal;

    & ol {
        list-style-type: lower-latin;

        & ol {
            list-style-type: lower-roman;

            & ol {
                list-style-type: upper-latin;

                & ol {
                    list-style-type: upper-roman;
                }
            }
        }
    }
}

.ck-content ul.ckeditor-list-type-style {
    list-style-type: disc;

    & ul {
        list-style-type: circle;

        & ul {
            list-style-type: square;

            & ul {
                list-style-type: square;
            }
        }
    }
}

@sbt99 sbt99 mentioned this issue Jul 24, 2023
Open
@wimleers
Copy link

wimleers commented Jul 26, 2023 •
edited

Thanks for the detailed bug report and suggested fix, @sbt99! 👏

This blocks Drupal from adopting CKEditor 5's native (and AWESOME!) <ul type> and <ol type> UX: https://www.drupal.org/project/drupal/issues/3274635 😞

@wimleers
Copy link

wimleers commented Nov 2, 2023

FYI: we have some pretty crazy live-CSS-rewriting in that Drupal issue to work around this bug:

  Drupal.behaviors.editorStyleFix = {
    attach(context) {
      if (context.querySelector('style[data-cke]')) {
        // CKEditor's DLL injects a style tag that overrides native list
        // type styling. The following find the style(s) causing the problem
        // and removes them.
        // @todo remove this entire behavior when this issue is fixed
        //  https://github.com/ckeditor/ckeditor5/issues/14613
        [...document.styleSheets]
          .filter((sheet) => sheet.ownerNode.hasAttribute('data-cke'))
          .forEach((sheet) => {
            [...sheet.cssRules].forEach((rule, ruleIndex) => {
              if (
                rule?.selectorText &&
                (rule.selectorText.includes(' ol') ||
                  rule.selectorText.includes(' ul')) &&
                !rule.selectorText.includes('type')
              ) {
                sheet.cssRules[ruleIndex].style['list-style-type'] = null;
              }
            });
          });
      }
    },
  };

— https://git.drupalcode.org/project/drupal/-/merge_requests/5079/diffs#46810af0b8d5994e8a9abd7221714b4836cb298d

@niegowski
Copy link
Contributor

Suggested fix in content css:

ckeditor5/packages/ckeditor5-list/theme/list.css

Line 6 in 67e0609

.ck-content ol { 

.ck-content ol:not([type]) {
	list-style-type: decimal;

	& ol:not([type]) {
		list-style-type: lower-latin;

		& ol:not([type]) {
			list-style-type: lower-roman;

			& ol:not([type]) {
				list-style-type: upper-latin;

				& ol:not([type]) {
					list-style-type: upper-roman;
				}
			}
		}
	}
}

.ck-content ul:not([type]) {
	list-style-type: disc;

	& ul:not([type]) {
		list-style-type: circle;

		& ul:not([type]) {
			list-style-type: square;

			& ul:not([type]) {
				list-style-type: square;
			}
		}
	}
}

@niegowski
Copy link
Contributor

The above is breaking to-do list styling. This needs to be double-checked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
domain:dx This issue reports a developer experience problem or possible improvement. domain:integration-dx This issue reports a problem with the developer experience when integrating CKEditor into a system. package:list squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@wimleers @niegowski @Witoso @sbt99