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.

We probably also need this to make position:sticky work on table parts.

I hate to be the squeaky-wheel but is anyone working on this so we finally can have position:sticky tables (#975644)?

Any news on this?

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).

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.

Andrew, are you still working on this bug ?

Assign to morris who is working on this bug.

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

Markus, can you review those patches? Thanks.

Sorry for the delay, I will review these patches soon.

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?

Comment on attachment 8852398 [details] Bug 929484 - Remove nsTableBorderBackground displayitem and nsTablePainter. https://reviewboard.mozilla.org/r/124648/#review133988

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.

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.

(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

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

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.

(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") .

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.

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 on attachment 8863656 [details] Bug 929484 - Turn on some passed tests. https://reviewboard.mozilla.org/r/135450/#review138650

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.

(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.

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

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.

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.

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.

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

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

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

We should add some fuzzy-if on those tests. Here is try result with fuzzy: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b964bdf5b5db16f00b1bc6b738e8ff1d788fab36

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

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

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

Yes to all of the above, I'd say.

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 :)

Markus already answered it.