Make HTML table frames (including row/cell/etc. frames) create their own display list items

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout
P2
normal
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: jwatt, Assigned: mtseng)

Tracking

(Blocks: 4 bugs, {perf})

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(7 attachments)

(Reporter)

Description

4 years ago
Matt tells me that, in the general case, we create a single display item to paint the background/borders of the entire HTML table (nsDisplayTableBorderBackground). This paints the background and border for all rows, columns, cells, etc. (Any actual content within the table still get normal display items created.)

One exception is if any of the table frames builds a 'stacking context' (for example, if it is transformed). Then the transformed row/cell builds a display item to paint the background/borders of itself and all children (nsDisplayTableCellBackground, nsDisplayTableRowBackground, nsDisplayTableRowGroupBackground).

We then have a bunch of DLBI hacks in place so that invalidations to a frame also get propagated to the frame that is going to draw their background.

We would really like to have every table frame paint it's own background regardless. This would fix a lot of table over-invalidation bugs, let us remove the DLBI hacks, and allow us to simplify reflow by having frames set their own position and size rather than having their parent do that in most cases.
(Reporter)

Updated

4 years ago
Blocks: 929489
We probably also need this to make position:sticky work on table parts.
Blocks: 975644
Depends on: 1130817

Comment 2

2 years ago
I hate to be the squeaky-wheel but is anyone working on this so we finally can have position:sticky tables (#975644)?
Blocks: 1187745
Any news on this?
Assignee: nobody → andrew
FWIW, there are some testcases for this stuff in bug 4510, which was the original bug associated with https://github.com/mozilla/gecko-dev/commit/ea9e726b8bd3b06bb1dc21b6933651dd89c788db

I think the behavior is pretty well specified for stuff that doesn't involve stacking contexts on internal table items, but it's a little more unclear for cases that do involve stacking contexts for internal table items (which might be slightly related to bug 688556).
Created attachment 8803788 [details] [diff] [review]
WIP factored collapsed table border display item

Attached is the root of my series of patches to isolate collapsed table borders into their own display item. The idea was to allow layerization of collapsed borders via nsDisplayTableBorderCollapse, drawing all collapsed borders into a single table-sized display item. In this patch, the stacking order is incorrect; our borders are positioned on top of content display items as a hack to get things off the ground.

Later patches attempted to construct the display list for a table irrespective of collapsed borders by recursing into child nsFrames, making use of nsContainerFrame::BuildDisplayListForNonBlockChildren, having each child be responsible for constructing the display list for its borders, collapsed or not. Unfortunately, that approach proved inviable as we don't structure our nsFrames like the W3C table border stacking order.

At the moment, if performance on pages with collapsed borders is an issue, I feel it is worth using an approach similar to nsDisplayTableBorderCollapse where we can use much of the existing logic. The work necessary to implement proper collapsed border construction is nontrivial.

Updated

9 months ago
Priority: -- → P3
Blocks: 1342854

Updated

5 months ago
Blocks: 1344082

Updated

5 months ago
Flags: needinfo?(aschen)
Priority: P3 → P2

Comment 6

5 months ago
Andrew, are you still working on this bug ?
Flags: needinfo?(aschen) → needinfo?(andrew)

Comment 7

5 months ago
Assign to morris who is working on this bug.
Assignee: andrew → mtseng
Flags: needinfo?(andrew)
(Assignee)

Comment 8

5 months ago
I have a patch that almost pass all tests. Once it pass all tests, I'll clean it up and start review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d585a40d7daa8682ab594e6badf5abee122c8113
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

5 months ago
Blocks: 1342343
Attachment #8852397 - Flags: review?(matt.woodrow) → review?(mstange)
Attachment #8852398 - Flags: review?(matt.woodrow) → review?(mstange)
(Assignee)

Comment 11

4 months ago
Markus, can you review those patches? Thanks.
Flags: needinfo?(mstange)
Sorry for the delay, I will review these patches soon.
Flags: needinfo?(mstange)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

4 months ago
mozreview-review
Comment on attachment 8852397 [details]
Bug 929484 - Draw each table's background on their own display item.

https://reviewboard.mozilla.org/r/124646/#review133984

::: layout/tables/nsTableCellFrame.cpp
(Diff revision 2)
>  nsTableCellFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
>                                     const nsRect&           aDirtyRect,
>                                     const nsDisplayListSet& aLists)
>  {
>    DO_GLOBAL_REFLOW_COUNT_DSP("nsTableCellFrame");
> -  if (IsVisibleInSelection(aBuilder)) {

What was the point of this condition and why can it be removed?

::: layout/tables/nsTableColFrame.h:55
(Diff revision 2)
>    /**
>     * Table columns never paint anything, nor receive events.
>     */

This comment is no longer accurate, is it?
Attachment #8852397 - Flags: review?(mstange) → review+

Comment 16

4 months ago
mozreview-review
Comment on attachment 8852398 [details]
Bug 929484 - Remove nsTableBorderBackground displayitem and nsTablePainter.

https://reviewboard.mozilla.org/r/124648/#review133988
Attachment #8852398 - Flags: review?(mstange) → review+

Comment 17

4 months ago
mozreview-review
Comment on attachment 8852397 [details]
Bug 929484 - Draw each table's background on their own display item.

https://reviewboard.mozilla.org/r/124646/#review133992

::: layout/tables/nsTableFrame.cpp:1425
(Diff revision 2)
> +      PaintRowBackground(row, aFrame, aBuilder, aLists);
> +    } else if (aFrame->GetType() == nsGkAtoms::tableColGroupFrame) {
> +      // Compute background rect by iterating all cell frame.
> +      nsTableColGroupFrame* colGroup = static_cast<nsTableColGroupFrame*>(aFrame);
> +      // Collecting column index.
> +      nsTArray<int32_t> colIdx;

This should probably be an nsAutoTArray.

::: layout/tables/nsTableFrame.cpp:1440
(Diff revision 2)
> +        PaintRowGroupBackgroundByColIdx(rowGroup, aFrame, aBuilder, aLists, colIdx, offset);
> +      }
> +    } else if (aFrame->GetType() == nsGkAtoms::tableColFrame) {
> +      // Compute background rect by iterating all cell frame.
> +      nsTableColFrame* col = static_cast<nsTableColFrame*>(aFrame);
> +      nsTArray<int32_t> colIdx;

same here
This is great, but I think it's not going quite far enough to fix all the bugs that we were hoping for it to fix. It is a very good first step, though.

In particular, the separate border collapse item means that:
 - We still need to have frame repaint hints for invalidation and can't let DLBI handle everything
 - We probably still need to invalidate to handle transform and opacity changes?
 - Not sure how this plays with relative positioning
 - It probably doesn't fix position:sticky on table parts either

However, this should fix a number of bugs, and I think we should add tests for those fixed bugs. I would imagine that this affects opacity in the presence of colgroup backgrounds, and makes background-attachment:fixed with async scrolling work in the presence of border collapse.
(Assignee)

Comment 19

4 months ago
mozreview-review-reply
Comment on attachment 8852397 [details]
Bug 929484 - Draw each table's background on their own display item.

https://reviewboard.mozilla.org/r/124646/#review133984

> What was the point of this condition and why can it be removed?

In the past, we draw cell only when cell is in selection. But now we should paint cell no matter it in selection or not. So I removed the condition.

> This comment is no longer accurate, is it?

Yes, I removed it. Also remove inaccurate comment in nsTableColGroupFrame::BuildDisplayList.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

4 months ago
(In reply to Markus Stange [:mstange] from comment #18)
> This is great, but I think it's not going quite far enough to fix all the
> bugs that we were hoping for it to fix. It is a very good first step, though.
> 
> In particular, the separate border collapse item means that:
>  - We still need to have frame repaint hints for invalidation and can't let
> DLBI handle everything
>  - We probably still need to invalidate to handle transform and opacity
> changes?
>  - Not sure how this plays with relative positioning
>  - It probably doesn't fix position:sticky on table parts either
> 
> However, this should fix a number of bugs, and I think we should add tests
> for those fixed bugs. I would imagine that this affects opacity in the
> presence of colgroup backgrounds, and makes background-attachment:fixed with
> async scrolling work in the presence of border collapse.

Yap, thanks for the suggestions. Can we create several follow-up bugs for tracking above issues? In the mean time, I'll submit a try to see if we fix any bugs and add test case for those bugs.

try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bf82921fd8b67a05a0e7a5cd063c7e765b862f8
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

4 months ago
Fix build error, submit an another try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=24f7928b99e672d04dcd00b314346dd9890c6adb
(Assignee)

Comment 26

4 months ago
Looks like 2 reftests which are related to colgroup background are passed. see https://treeherder.mozilla.org/#/jobs?repo=try&revision=24f7928b99e672d04dcd00b314346dd9890c6adb&selectedJob=92632583

Markus, do you have more details about the bug for background-attachment:fixed with async scrolling and border collapse? Looks like I should add more test case for this. Thanks.
Flags: needinfo?(mstange)
(In reply to Morris Tseng [:mtseng] [:Morris] from comment #26)
> Markus, do you have more details about the bug for
> background-attachment:fixed with async scrolling and border collapse? Looks
> like I should add more test case for this. Thanks.

You can look at the reftest layout/reftests/async-scrolling/bg-fixed-child-clip-1.html and basically do the same but with a border-collapse table instead of a div. It tests that the background stays fixed during async scrolling (reftest-async-scroll-y="50") .
Flags: needinfo?(mstange)
(Assignee)

Comment 28

4 months ago
Created attachment 8862710 [details]
bg-fixed-child-clip-3.html

I tested border collapsed table with background-attachment:fixed, and I didn't notice any difference with and without my patch. Attached is my testing file.
(Assignee)

Comment 29

4 months ago
I found R8 would trigger many assertions because that if the table row group has fixed background, we'll call AppendBackgroundImagesToTop for each cell in the table row group. Then in the AppendBackgroundImageToTop will create many nsDisplayFixedPosition and all this nsDisplayFixedPosition reference to same table row group frame. In nsDisplayFixedPosition::BuildLayer, it always return same layer because the recycle mechanism. But we don't want to recycle layer here, we should create different container layer even if the same frame. I'll try to fix this.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

4 months ago
mozreview-review
Comment on attachment 8863656 [details]
Bug 929484 - Turn on some passed tests.

https://reviewboard.mozilla.org/r/135450/#review138650
Attachment #8863656 - Flags: review?(mstange) → review+
Attachment #8863654 - Flags: review?(mstange) → review?(matt.woodrow)
Attachment #8863655 - Flags: review?(mstange) → review?(matt.woodrow)
How do we handle display item key collisions? The secondary reference frame pointer won't fit into the uint32_t, especially not if it's shifted some more.
If they actually collide, then we don't handle them, it confuses FrameLayerBuilder to have duplicates and causes all sorts of bugs.

It's possible that we could get something unique out of the pointer by shifting/masking, but we'd be relying on properties of the allocator that are hard to guarantee across all platforms (current and future).

We could assign unique numbers to each descendant of the owning frame as we descend into them, but we need to make sure we manually invalidate things if the number ever changes.

Appending a new cell to the start of the table would renumber everything, and cause us to invalidate the whole table.

Maybe we could do something more complex where the cells all remember their number, and we only allocate new numbers to new frames. Figuring out where to store those numbers for fast lookup is a pain though.

Why can't we have the table cell as the primary frame for the display item?

The other option is to make dual frames a supported option for all display items, and make FLB's hashing include both frames, plus the key.
(Assignee)

Comment 38

4 months ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #37)
> Why can't we have the table cell as the primary frame for the display item?
We cannot. For example, if row group and col group both has fixed background image, we'll create 2 nsDisplayFixedPosition with same table cell frame as primary frame. So this doesn't work.
> 
> The other option is to make dual frames a supported option for all display
> items, and make FLB's hashing include both frames, plus the key.
This might work. I'll try this solution. Thanks.
Can't you make the cell frame the primary frame for the item, and then use an enum for the secondary type (rowgroup=1, colgroup=2 etc) to include in the per-frame key?
To expand:

Create a subclass of nsDisplayFixedPosition, nsDisplayTableFixedPosition. Override GetPerFrameKey to return the type + the rowgroup/colgroup enum value. Override IsInvalid() to check for invalid bits on the appropriate ancestor as well as the current frame.
(Assignee)

Comment 41

4 months ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #39)
> Can't you make the cell frame the primary frame for the item, and then use
> an enum for the secondary type (rowgroup=1, colgroup=2 etc) to include in
> the per-frame key?

I can do this for nsDisplayFixedPosition. But for the nsDisplayBackgroundImage, I cannot replace its primary frame because the rendering result will wrong. Matt, do you have any suggestion for nsDisplayBackgroundImage?
Flags: needinfo?(matt.woodrow)
So, I think you can store both the frame pointers on nsDisplayBackgroundImage. Set mFrame to the cell frame (so that's what we use for generating a unique key for FLB), and then store the row/col frame as a new member and change the painting code to use that one for looking up style data.

You could maybe add a new virtual to nsDisplayBackgroundImage like GetStyleFrame() { return mFrame; }, and then have nsDisplayTableBackgroundImage override that with the row/col frame.
Flags: needinfo?(matt.woodrow)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 51

4 months ago
mozreview-review
Comment on attachment 8863654 [details]
Bug 929484 - Create nsDisplayTableFixedPosition to avoid display list collision when processing background image of table.

https://reviewboard.mozilla.org/r/135446/#review138786

::: layout/painting/nsDisplayList.h:4159
(Diff revision 2)
> +                                                               nsDisplayBackgroundImage* aImage,
> +                                                               uint32_t aIndex,
> +                                                               nsIFrame* aAncestorFrame);
> +
> +  virtual uint32_t GetPerFrameKey() override {
> +    return (mIndex << (nsDisplayItem::TYPE_BITS + sizeof(TableType))) |

What does sizeof(TableType) evaluate to? We really want it to be 8, but it's not obvious that it would be (on all compilers).

Can we just do the same as TYPE_BITS and just manually define it as 3 and use a static_assert that we can fit the largest value in TableType within 3 bits.

::: layout/painting/nsDisplayList.cpp:6531
(Diff revision 2)
> +
> +  return new (aBuilder) nsDisplayTableFixedPosition(aBuilder, aFrame, &temp, aIndex + 1, aAncestorFrame);
> +}
> +
> +bool
> +nsDisplayTableFixedPosition::IsInvalid(nsRect& aRect)

We shouldn't need to worry about invalidation for a container type as everything ends up on the layer and LayerTreeInvalidation will detect changes.

It's only for items that do PaintedLayer drawing that might change drawing without it being visible on the layer properties.

I think if we drop IsInvalid, then we can remove mAncestorFrame and GetAncestorFrameByTableType too.
Attachment #8863654 - Flags: review?(matt.woodrow) → review+

Comment 52

4 months ago
mozreview-review
Comment on attachment 8863655 [details]
Bug 929484 - Create nsDisplayTableBackgroundImage to avoid display item collision when processing background image of table.

https://reviewboard.mozilla.org/r/135448/#review138790

::: layout/painting/nsDisplayList.h:3194
(Diff revision 3)
>  TableType GetTableTypeFromFrame(nsIFrame* aFrame);
>  nsIFrame* GetAncestorFrameByTableType(nsTableCellFrame* aCellFrame, TableType aType);
>  
> +class nsDisplayTableBackgroundImage : public nsDisplayBackgroundImage {
> +public:
> +  nsDisplayTableBackgroundImage(const InitData& aInitData, nsIFrame* aStyleFrame);

aCellFrame, right?

Might be worth adding a comment about why this class exists and how we override mFrame to be aCellFrame (even though it's not the frame that has the background style) because it gives us a (more) unique frame for FLB.

::: layout/painting/nsDisplayList.cpp:3759
(Diff revision 3)
> +nsDisplayTableBackgroundImage::IsInvalid(nsRect& aRect)
> +{
> +  bool result = mFrame ? mFrame->IsInvalid(aRect) : false;
> +  MOZ_ASSERT(mFrame->IsTableCellFrame() || mFrame->IsBCTableCellFrame());
> +  nsTableCellFrame* cell = static_cast<nsTableCellFrame*>(mFrame);
> +  result = result && (mStyleFrame == GetAncestorFrameByTableType(cell, mTableType));

I don't think we can change the ancestor without marking the frame itself as invalid (We usually rebuild all descendant frames which will make them invalid).

We do need to check mStyleFrame->IsInvalid() though.
Attachment #8863655 - Flags: review?(matt.woodrow) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 58

4 months ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=347f0c764c290f797258d0c07a77c171527e4f28

Comment 59

4 months ago
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93acd240b578
Draw each table's background on their own display item. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dc8c2aea9bf
Remove nsTableBorderBackground displayitem and nsTablePainter. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/e41fa1543360
Create nsDisplayTableFixedPosition to avoid display list collision when processing background image of table. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8ade5137ae
Create nsDisplayTableBackgroundImage to avoid display item collision when processing background image of table. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c902365910c
Turn on some passed tests. r=mstange
backed out for reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=96583234&repo=mozilla-inbound
Flags: needinfo?(mtseng)

Comment 61

4 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c8a2b1db381
Backed out changeset 7c902365910c 
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bc03e02687c
Backed out changeset 9c8ade5137ae 
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a8f25229da
Backed out changeset e41fa1543360 
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9cdb47c1b9
Backed out changeset 3dc8c2aea9bf 
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f788935a3a4
Backed out changeset 93acd240b578 for reftest failures in border-separate-opacity-table-column-group.html
(Assignee)

Comment 62

4 months ago
We should add some fuzzy-if on those tests.

Here is try result with fuzzy:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b964bdf5b5db16f00b1bc6b738e8ff1d788fab36
Flags: needinfo?(mtseng)

Comment 63

4 months ago
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31cfb8c2491f
Draw each table's background on their own display item. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f7e0529ee3f
Remove nsTableBorderBackground displayitem and nsTablePainter. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8b2a8bbe3e6
Create nsDisplayTableFixedPosition to avoid display list collision when processing background image of table. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd72f6913f92
Create nsDisplayTableBackgroundImage to avoid display item collision when processing background image of table. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a15fd71cf35
Turn on some passed tests. r=mstange

Comment 64

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/31cfb8c2491f
https://hg.mozilla.org/mozilla-central/rev/0f7e0529ee3f
https://hg.mozilla.org/mozilla-central/rev/f8b2a8bbe3e6
https://hg.mozilla.org/mozilla-central/rev/dd72f6913f92
https://hg.mozilla.org/mozilla-central/rev/2a15fd71cf35
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

4 months ago
Blocks: 1362551
Depends on: 1363483

Updated

3 months ago
Depends on: 1364186
No longer depends on: 1364186

Comment 65

3 months ago
This bug may have contributed to a sudden change in the Telemetry probe DISPLAY_ITEM_USAGE_COUNT[1] which seems to have occured in Nightly 20170506[2][3].

It seems as though display items changed their frequencies quite dramatically all of a sudden.

Is this a good thing?

Is this intentional? Is this expected?

Is this probe measuring something useful?

[1]: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/2286/alerts/?from=2017-05-06&to=2017-05-06
[2]: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b255199db9d6a6f189b89b7906f99155bde3726&tochange=37a5b7f6f101df2eb292b1b6baaf1540c9920e20
[3]: https://mzl.la/2rn9abO
Flags: needinfo?(mtseng)
Yes to all of the above, I'd say.

Comment 67

3 months ago
Spiffy! I've filed bug 1365357 to fix the alert_emails field so that hopefully someone with domain knowledge can look at any future changes before I stumble across them :)
(Assignee)

Comment 68

3 months ago
Markus already answered it.
Flags: needinfo?(mtseng)

Updated

3 months ago
See Also: → bug 1365357
Depends on: 1367747
Depends on: 1369141

Updated

2 months ago
Duplicate of this bug: 1373605

Updated

2 months ago
Depends on: 1375518

Updated

2 months ago
Blocks: 1375795
You need to log in before you can comment on or make changes to this bug.