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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Markers should be properly downcasted and upcasted on bogus paragraphs #13287

Merged
merged 1 commit into from
Jan 21, 2023

Conversation

niegowski
Copy link
Contributor

@niegowski niegowski commented Jan 17, 2023

Suggested merge commit message (convention)

Fix (engine, list, table): Markers should not be lost on list items and in table cells in the data pipeline. Closes #13285.


Additional information

@niegowski niegowski marked this pull request as draft January 18, 2023 15:47
@niegowski niegowski changed the title markerToData should set data attributes on the AttributeElement Markers should be properly downcasted on bogus paragraphs Jan 18, 2023
@niegowski niegowski changed the base branch from master to release January 20, 2023 10:49
@niegowski niegowski marked this pull request as ready for review January 20, 2023 11:10
@niegowski niegowski changed the title Markers should be properly downcasted on bogus paragraphs Markers should be properly downcasted and upcasted on bogus paragraphs Jan 20, 2023
@niegowski niegowski requested a review from scofalik January 20, 2023 11:12
@Mgsy
Copy link
Member

Mgsy commented Jan 20, 2023

Not a big deal, but I can't reproduce it on release:

Steps to reproduce

  1. Go to http://localhost:8125/ckeditor5/external/collaboration-features/packages/ckeditor5-collaboration/tests/manual/all-features-rtc-document-list.html.
  2. Insert an empty table and put the selection inside an empty cell.
  3. Turn on Track Changes.
  4. Select Heading 1 from the headings dropdown to add a formatting suggestion.
  5. Call editor.data.set( editor.getData(), { suppressErrorInCollaboration: true } ).
  6. Reject the suggestion.
  7. Try to create an ordered list using autoformatting in the cell that had a formatting suggestion.

Actual result

It's impossible to insert a list using autoformatting.

Notes

Just noticed it could be reproduced only with editor.data.set( editor.getData(), { suppressErrorInCollaboration: true } ). Reconnecting to the document doesn't trigger this issue, so I guess we can ignore it.

@scofalik
Copy link
Contributor

AFAICS, this happens because there is a bogus nbsp; added to the table cell.

editor.getData() returns this:

<figure class="table"><table><tbody><tr><td><p data-suggestion-start-before="formatBlock:698dn3bun9mf:e321952b3e500000ea901c56954d7ddcd:e168fe479815a10c23f0c08a44996fe42"><suggestion-end name="formatBlock:698dn3bun9mf:e321952b3e500000ea901c56954d7ddcd:e168fe479815a10c23f0c08a44996fe42"></suggestion-end>&nbsp;</p></td></tr></tbody></table></figure>

@niegowski, can we do anything relatively simple to avoid these spaces?

@niegowski
Copy link
Contributor Author

AFAICS, this happens because there is a bogus nbsp; added to the table cell.

editor.getData() returns this:

<figure class="table"><table><tbody><tr><td><p data-suggestion-start-before="formatBlock:698dn3bun9mf:e321952b3e500000ea901c56954d7ddcd:e168fe479815a10c23f0c08a44996fe42"><suggestion-end name="formatBlock:698dn3bun9mf:e321952b3e500000ea901c56954d7ddcd:e168fe479815a10c23f0c08a44996fe42"></suggestion-end>&nbsp;</p></td></tr></tbody></table></figure>

@niegowski, can we do anything relatively simple to avoid these spaces?

This is a block filler that was not stripped because of another element in the same block (there is a marker-end element) and DomConverter is not able to strip this block filler because it does not know whether the marker element is a marker or any other element. We might teach DomConverter what element names are describing markers (by registering element names while registering a marker converter) to ignore such elements while checking block filler but I'm not sure if we could do it in such a short time window. This looks like sth out of the scope of this ticket and I would suggest extracting this to a separate issue. WDYT @scofalik

@scofalik
Copy link
Contributor

I thought so.

I briefly look whether this won't mess up with revision history, but it seems it is fine.

@scofalik scofalik merged commit 1fa7ffc into release Jan 21, 2023
@scofalik scofalik deleted the ck/13285 branch January 21, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Markers are not correctly downcasted when start before document list element
4 participants