-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Rethink table model (or table atomic conversion) #6630
Comments
Similar case - merging cells when other cells have rowspan: #6667. |
Closing as dup of #6506. There's a conversation on the matter already. |
Another problem (#7454) with @niegowski is that the "fix" with double At this stage I'm sure that current API lacks something - making big structural changes. At this point @niegowski will try to remove the |
I'll try to call |
📝 Provide a description of the improvement
I've been struggling with recent bugs concerning removing rows (#6502, #6437, #6544, #6502, #6391, #6406) and I've came to a conclusion that we have a problem in table model definition.
In model we do not have "redundant" elements from HTML:
<tbody>
and<thead>
. This allows to have the model simpler. In most scenarios model operations are quite simple. The problem get more complex if we addcolspan
androwspan
attributes. Table cells that are merged (thus are spanned by row or column) must be treated carefully when modifying model because:<thead>
boundary in the model (well heading section).<thead>
must be<th>
so must be renamed when moved from<tbody>
.Most of the table atomic converters for table structure operates in the scope of its knowledge:
headingsRows
change downcast converter will move table rows from<thead>
to<tbody>
and rename cells as needed. In particular it will fix cells that could break the rule of not spanning over<thead>
.removeRow
downcast converter will remove rows from the view and will fixrowspan
attribute of cells that overlapped the removed row so the table geometry is intact.This works fine in most scenarios but for some operations on tables if fails. This happens because most of the table operations requires a broad knowledge of a table structure for particular change.
Consider table:
In (simplified) HTML the table is rendered as:
Atomic converters works in on operation at a time basis. In order to work out some of the above constrains they calculate operations based on the model state. This fails if more than two operations happens on a table.
Consider changing header rows from
2
to1
. The downcast converter:H2
(because it is at index which is after a heading section) finds its view element and move it to<tbody>
section.th
.Now consider removing multiple rows (
H2
andB1
). TheRemoveRow
command will:H2
row.1
.B2
.Now this will fail because heading rows converter sees the same attribute change and will assume that table is valid. But what this converter will do is:
B2
) and move it's view to<tbody>
.After table's attributes change conversion the remove row converter will step in. Removing elements from model is done on positions (offset in parent). So it will:
table
).2
).B2
to be removed instead ofH2
.B2
was removed instead ofH2
.So the spotted problems are:
enqueueChange()
- I don't see other option for the problem described here. The thing withenqueueChange()
is that run conversion on each step so it acts like step-by-step conversion but it looks like more work is done than necessary.headingRows
conversion. One idea is to have<tableRow isHeading="true">
- this will require some other checks (noisHeading
gaps have to be checked) than now but conversion "should" be simple for this case. And I don't see how we can make similar change with columns.If you'd like to see this improvement implemented, add a 👍 reaction to this post.
The text was updated successfully, but these errors were encountered: