Closed Bug 1389295 Opened 7 years ago Closed 7 years ago

What should the border-collapse damage area be when appending a rowspanning cell that spans into "dead" rows?

Categories

(Core :: Layout: Tables, enhancement)

53 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: bzbarsky, Assigned: neerja)

References

Details

Attachments

(1 file)

layout/tables/crashtests/460637-2.xhtml asserts with the patches for bug 1384602 because we end up with a border-collapse damage area of height 9, but only have one "real" row, in spite of having a rowspan="9" cell.

It's not clear to me which units the damage area is supposed to be in, and hence whether the assert needs adjusting or the damage area calculation under cellmap's AppendCell, or maybe the assert and the union bit in AddBCDamageArea?

Mats, do you know whether you'll have time to look at this?  I'm not sure who all else is really familiar with the cellmap...

Jet, do we have anyone who's been wanting to learn about tables?  ;)
Flags: needinfo?(bugs)
Summary: What should the border-collapse damage area be when appending a rowspanning cell that spans into "dead" rows → What should the border-collapse damage area be when appending a rowspanning cell that spans into "dead" rows?
Blocks: 1384602
Assignee: nobody → npancholi
Flags: needinfo?(bugs)
Comment on attachment 8902444 [details]
Bug 1389295 - Confine BCDamageArea while appending cells to within the table area and remove assert for test 460637-2.xhtml

https://reviewboard.mozilla.org/r/172770/#review180048

::: layout/tables/nsCellMap.cpp:1495
(Diff revision 2)
> -                                    1 + endRowIndex - aRowIndex;
> +      zeroRowSpan ? endRowIndex - aRowIndex :
> +                    std::min(GetRowGroup()->GetRowCount() - aRowIndex,
> +                             rowSpan);

Currently, the two alternative terms are expressed as "X" and "1 + X".
I think the preceding code comment is slightly clearer if we continue
to do that, so after the first step: changing
"1 + endRowIndex - aRowIndex" to the equivalent "rowSpan", we get
"zeroRowSpan ? rowSpan - 1  : rowSpan".

Secondly, I don't see why we should only clamp the last part of
that expression.  I tend to think we should clamp the whole thing.

Thirdly, indirecting through the row-group frame seems unnecessary -
it seems slightly more correct to use GetRowCount() (of nsCellMap)
instead of GetRowGroup()->GetRowCount().

So I'd suggest something like this:

    uint32_t height = std::min(zeroRowSpan ? rowSpan - 1  : rowSpan,
                               GetRowCount() - aRowIndex);

WDYT?
Comment on attachment 8902444 [details]
Bug 1389295 - Confine BCDamageArea while appending cells to within the table area and remove assert for test 460637-2.xhtml

https://reviewboard.mozilla.org/r/172770/#review180048

> Currently, the two alternative terms are expressed as "X" and "1 + X".
> I think the preceding code comment is slightly clearer if we continue
> to do that, so after the first step: changing
> "1 + endRowIndex - aRowIndex" to the equivalent "rowSpan", we get
> "zeroRowSpan ? rowSpan - 1  : rowSpan".
> 
> Secondly, I don't see why we should only clamp the last part of
> that expression.  I tend to think we should clamp the whole thing.
> 
> Thirdly, indirecting through the row-group frame seems unnecessary -
> it seems slightly more correct to use GetRowCount() (of nsCellMap)
> instead of GetRowGroup()->GetRowCount().
> 
> So I'd suggest something like this:
> 
>     uint32_t height = std::min(zeroRowSpan ? rowSpan - 1  : rowSpan,
>                                GetRowCount() - aRowIndex);
> 
> WDYT?

Agreed! :) Everything you pointed out makes sense. I've updated the patch with this change and a successful try run is at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41cadef59451ec7ccb50821749171676108fbcd2&selectedJob=127546289
Comment on attachment 8902444 [details]
Bug 1389295 - Confine BCDamageArea while appending cells to within the table area and remove assert for test 460637-2.xhtml

https://reviewboard.mozilla.org/r/172770/#review180208
Attachment #8902444 - Flags: review?(mats) → review+
Pushed by npancholi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b3bd0baa4b33
Confine BCDamageArea while appending cells to within the table area and remove assert for test 460637-2.xhtml r=mats
https://hg.mozilla.org/mozilla-central/rev/b3bd0baa4b33
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.