should only append display items for table borders when there are borders to draw

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout: Tables
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: dbaron, Assigned: mtseng)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

I noticed, looking at the new table display item generation code from bug 929484, that it appends display items for table borders whether or not there are actually borders to draw.  This causes creation of more display items than needed, and isn't the way we normally generate border display items.  It may well make the various display item optimizations less effective.

It seems like the code that generates these display items in nsTableFrame::DisplayGenericTablePart should only generate them when there are actually borders.

(The code in nsTableCellFrame::ProcessBorders does the right thing for table cell borders.)

For the non-border-collapse case this should be easy, since it requires only checking the border on the table.

For the border-collapse case, it's harder since it requires testing for border on every table part.  However, I think such testing would also be useful for other things.  A few days ago I saw a bug with discussion of making the special table invalidation code that was needed because we couldn't depend on DLBI conditional on not being border-collapse.  (I can't find the bug right now, though!)  If we had a boolean for whether the table actually *had* borders, we could make it conditional on being border-collapse and having borders.
Flags: needinfo?(mtseng)
Sorry for late reply, thanks for the suggestion. I'll do this.
Assignee: nobody → mtseng
Flags: needinfo?(mtseng)
Created attachment 8873335 [details] [diff] [review]
Create nsDisplayBorder when there are borders to draw.

MozReview-Commit-ID: H5PYj8ErRto
Attachment #8873335 - Flags: review?(mstange)
Attachment #8873335 - Flags: review?(mstange) → review+
Are you planning to address the border-collapse part in this bug, or should there be a separate bug for that?
The codes that generating border are only consider table frame and not other table parts (see [1]). And we also create border-collapse display item in [2]. Do we need address border-collapse here?


[1]: https://searchfox.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#1489-1498
[2]: https://searchfox.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#1492-1494
Flags: needinfo?(dbaron)
See comment 0 about the value.  The border-collapse case is one where we have to disable a bunch of optimization, so I'd prefer not to do so for a border-collapse table that doesn't actually have borders.
Flags: needinfo?(dbaron)
Ah, I understood. I'll fix the border-collapse part in this bug as well.
Created attachment 8873731 [details] [diff] [review]
Create nsDisplayBorder when there are borders to draw.

Markus, I slightly modified this patch. Can you review it again?

MozReview-Commit-ID: H5PYj8ErRto
Attachment #8873731 - Flags: review?(mstange)
Attachment #8873335 - Attachment is obsolete: true
Created attachment 8873732 [details] [diff] [review]
Create nsDisplayTableBorderCollapse when there is collapse border to draw.

MozReview-Commit-ID: 29g6HqAn0ej
Attachment #8873732 - Flags: review?(mstange)
Looks like some tests failed, pending review request.
Attachment #8873731 - Flags: review?(mstange)
Attachment #8873732 - Flags: review?(mstange)
Created attachment 8874185 [details] [diff] [review]
Create nsDisplayBorder when there are borders to draw.

MozReview-Commit-ID: H5PYj8ErRto
Attachment #8874185 - Flags: review?(mstange)
Attachment #8873731 - Attachment is obsolete: true
Created attachment 8874186 [details] [diff] [review]
Create nsDisplayTableBorderCollapse when there is collapse border to draw.

MozReview-Commit-ID: 29g6HqAn0ej
Attachment #8874186 - Flags: review?(mstange)
Attachment #8873732 - Attachment is obsolete: true
Comment on attachment 8874185 [details] [diff] [review]
Create nsDisplayBorder when there are borders to draw.

Review of attachment 8874185 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tables/nsTableFrame.cpp
@@ +1501,1 @@
>    if (aFrame->IsVisibleForPainting(aBuilder)) {

Doesn't this condition already check the visibility?

I think you can go back to the patch I r+ed previously and just create a separate patch to remove the dead function FrameHasBorderOrBackground.
Attachment #8874185 - Flags: review?(mstange)
Comment on attachment 8874186 [details] [diff] [review]
Create nsDisplayTableBorderCollapse when there is collapse border to draw.

Review of attachment 8874186 [details] [diff] [review]:
-----------------------------------------------------------------

Iterating over everything seems a bit expensive but it's probably what we need to do. It would be nice if we could reuse some work that we're already doing in order to compute the result here, but I don't know the code well enough to assess whether that's possible.

::: layout/tables/nsTableFrame.cpp
@@ +1424,5 @@
>    return false;
>  }
>  
> +void nsTableFrame::CalcHasBCBorders()
> +{

Can you add an early check here that handles the case where IsBorderCollape() is false? I know that at the moment, this function is only called if IsBorderCollape() is true, but new callers without such a check might be added in the future.

@@ +1425,5 @@
>  }
>  
> +void nsTableFrame::CalcHasBCBorders()
> +{
> +  if (FrameHasBorder(this)) {

I see now, so this is where you need the visibility check. You can put the FrameHasBorder function from the previous patch into this patch :)
Attachment #8874186 - Flags: review?(mstange) → review+
Created attachment 8874670 [details] [diff] [review]
Create nsDisplayBorder when there are borders to draw. (carry r+: mstange)

Use previous r+ed patch.

MozReview-Commit-ID: H5PYj8ErRto
Attachment #8874670 - Flags: review+
Attachment #8874185 - Attachment is obsolete: true
Created attachment 8874671 [details] [diff] [review]
Create nsDisplayTableBorderCollapse when there is collapse border to draw. (carry r+: mstange)

Addressed comments.

MozReview-Commit-ID: 29g6HqAn0ej
Attachment #8874671 - Flags: review+
Attachment #8874186 - Attachment is obsolete: true
(In reply to Markus Stange [:mstange] from comment #15)
> Comment on attachment 8874186 [details] [diff] [review]
> Create nsDisplayTableBorderCollapse when there is collapse border to draw.
> 
> Review of attachment 8874186 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Iterating over everything seems a bit expensive but it's probably what we
> need to do. It would be nice if we could reuse some work that we're already
> doing in order to compute the result here, but I don't know the code well
> enough to assess whether that's possible.
> 

I hope we can reuse this function in some frame invalidation check. Or we can remove this function once border collapses are rendered by each cell.
Try looks ok.

Comment 21

a year ago
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/370809b43a59
Create nsDisplayBorder when there are borders to draw. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ab3d69a2cf0
Create nsDisplayTableBorderCollapse when there is collapse border to draw. r=mstange

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/370809b43a59
https://hg.mozilla.org/mozilla-central/rev/4ab3d69a2cf0
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox57: affected → ---

Updated

11 months ago
Depends on: 1394249
You need to log in before you can comment on or make changes to this bug.