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

RESOLVED FIXED in Firefox 57

Status

()

Core
Layout: Tables
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: bz, Assigned: neerja)

Tracking

53 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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?
(Assignee)

Updated

9 months ago
Assignee: nobody → npancholi

Updated

9 months ago
Flags: needinfo?(bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

9 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 6

9 months ago
mozreview-review-reply
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 7

9 months ago
mozreview-review
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+

Comment 8

9 months ago
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
Last Resolved: 9 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.