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.

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

Already on GitHub? Sign in to your account

GHS lists integration causes the editor to fire post-fixer directly after loading initial data #16227

Closed
scofalik opened this issue Apr 16, 2024 · 0 comments 路 Fixed by #16294
Closed
Labels
package:html-support squad:collaboration Issue to be handled by the Collaboration team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@scofalik
Copy link
Contributor

scofalik commented Apr 16, 2024

馃摑 Provide detailed reproduction steps (if any)

  1. Add GHS plugin to the editor.
  2. Load <ul class="myUl"><li>A<ol class="myOl"><li>B</li></ol></li></ul> as the editor data.
  3. Observe that two operations are in the document history. The first operation is the initial insert (expected), the second operation removes htmlUlAttributes from the second list item.
  4. This happens only for nested list items, that have different type than one of their ancestor.

What happened here:

  1. During upcast, the second (nested) list item received attributes from both its direct parent (<ol>) but also, incorrectly, from its ancestor (<ul>).
  2. This is incorrect situation.
  3. To fix such situations we have a post-fixer that removes htmlUlAttributes from a list item that has numbered type.

But this creates unnecessary operation and causes problems with real-time collaboration. Instead, we should build the correct model while upcasting.

A simple fix is to change this line of code:

https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-html-support/src/integrations/list.ts#L219

if ( item.hasAttribute( attributeName ) ) {

to:

if ( item.hasAttribute( 'htmlUlAttributes' ) || item.hasAttribute( 'htmlOlAttributes' ) ) {

However, maybe it would be good to refactor this whole conversion here.

Right now, when we have nested lists, we revisit the same model list items over and over again, for each nesting (e.g. if we have ul > ul > ol > li, model element for this li will be visited when processing ol, then when processing ul, and then when processing ul again, and this is why this condition was needed but was incorrectly implemented and didn't work for different list elements).

I understand difficulties of upcasting (we don't have mapper, we need to traverse data.modelRange as the original view element may got split, etc.) but maybe there is a chance to write it in a way that the integration will provide a converter only for element:li?

But this is only a bonus task. The performance isn't that problematic -- we add some extra iterations but they are limited only to number of indentations, which should be several at most.

@scofalik scofalik added type:bug This issue reports a buggy (incorrect) behavior. support:2 An issue reported by a commercially licensed client. package:html-support squad:collaboration Issue to be handled by the Collaboration team. labels Apr 16, 2024
scofalik added a commit that referenced this issue Apr 29, 2024
Other (html-support): GHS list integration will create proper model structure on upcast and not fire a redundant post-fixer during editor initialization. Closes #16227.
@CKEditorBot CKEditorBot added this to the iteration 74 milestone Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:html-support squad:collaboration Issue to be handled by the Collaboration team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants