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

#3274: Updated table layout post-fixer and upcast converters to properly fix empty rows. #6762

Merged
merged 6 commits into from
May 12, 2020

Conversation

niegowski
Copy link
Contributor

@niegowski niegowski commented May 7, 2020

Suggested merge commit message (convention)

Fix (table): Table layout post-fixer and upcast converters should properly handle empty rows . Closes #3274.


Additional information

@niegowski niegowski requested a review from jodator May 7, 2020 16:44
@niegowski niegowski changed the title Updated table layout post-fixer to properly fix empty rows. #3274: Updated table layout post-fixer to properly fix empty rows. May 7, 2020
@jodator
Copy link
Contributor

jodator commented May 8, 2020

I think that this fix is too late in the whole progress. We must always have valid model and all algorithms that operates on model expects that and must create valid table.

We have post-fixers for cases that general algorithms on the model could produce invalid table. In most of the cases it is due to collaboration changes. This might happen when applying colliding changes from an external editor. The most obvious case is: user A insert row user B inserts column. Such changes should only be the reason to fix improper model structure.

The table upcast converters should work with invalid HTML - just because not everybody reads the specs and can create something invalid. We will not fix them all and let the table blow-up :man_shrugging: . But in cases that an invalid HTML breaks the editor should be addressed there.

In other words we should create valid model from an invalid input during upcast conversion if feasible.

I think that the provided change is not in line with a valid table model and will yield table model error:

22. If there exists a row or column in the table containing only slots that do not have a cell anchored to them, then this is a table model error.

I think that we should remove empty row from the model or not allow it to land it in the model in the first place. The fixer for rowspan attributes should cover cases where all cells occupies slots of another row. So that could be another issue.

ps.: I like the idea of "slot" - I didn't probably noticed that before and maybe it would be a better to use this term for "spanned" cells.

@jodator
Copy link
Contributor

jodator commented May 8, 2020

I've forgot about this tool:  https://validator.w3.org/nu/#textarea.

Example set of errors:

Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above :point_up: .

@niegowski niegowski changed the title #3274: Updated table layout post-fixer to properly fix empty rows. #3274: Updated table layout post-fixer and upcast converters to properly fix empty rows. May 8, 2020
@niegowski niegowski requested a review from jodator May 8, 2020 15:36
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :+1: I'd change one thing - to be discussed on F2F before pushing.

@jodator jodator merged commit fb5fe8b into master May 12, 2020
@jodator jodator deleted the i/3274 branch May 12, 2020 12:19
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.

Editor crashes with a downcasting error when input contain an empty <tr></tr>
2 participants