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)
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)
Reporter | ||
Updated•7 years ago
|
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•7 years ago
|
Assignee: nobody → npancholi
Updated•7 years ago
|
Flags: needinfo?(bugs)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
Try run for this patch looks ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab4fed7389956e42698e30d3526e5511ed06b50d
Comment hidden (mozreview-request) |
Comment 4•7 years 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•7 years 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•7 years 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+
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
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b3bd0baa4b33
Status: NEW → RESOLVED
Closed: 7 years 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.
Description
•