505184 - Paint table backgrounds with a dedicated background display item when possible

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In bug 339548 we clip plugins to exclude opaque content that should appear to float over the plugin. Youtube, and probably other sites, uses tables with opaque backgrounds to cover plugins (they currently make it work in Firefox using an IFRAME hack to get a Gecko native widget in the right place, but that will become irrelevant). Currently nsDisplayTableBorderBackground does not ever advertise that it's opaque. Instead of hacking intelligence into nsDisplayTableBorderBackground (which paints all table part borders and backgrounds in one go), dbaron suggested separating the table background out and drawing it using an nsDisplayBackground, which already knows whether it's opaque etc.

I'll post a series of patches here to do that. I suspect these may overlap a bit with the rework of border-collapsing that's going on --- sorry, but I need this ASAP.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: remove unused aBorder parameter to PaintBackgroundLayer

Analyzing how PaintBackgroundWithSC uses aBorder, I noticed that aBorder isn't used by PaintBackgroundLayer.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Simplify PaintTableFrame

PaintTableFrame computes a bunch of BC-border data which isn't actually used by PaintBackgroundWithSC. Removing this makes it clearer how table background painting differs from normal background painting (not much, only the 'deflate' parameter).

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: Create DisplayBackgroundUnconditional

I want to call this from nsTableFrame::BuildDisplayList

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Separate out table background drawing

When no "deflation" due to protruding BC borders is required, we can use the normal background painting display item.

Comment 5

[Bernd]

>I suspect these may overlap a bit with the rework of border-collapsing that's >going on 
There is no overlap at all.

Comment 6

[fantasai]

Comment on attachment 389440 [details] [diff] [review]
Part 2: Simplify PaintTableFrame

The code here isn't useless, the data's just getting lost on the way to the painting code (which seems to be causing a regression, and in any case should be evaluated in bug 505358, not here).

Comment 7

[fantasai]

Comment on attachment 389442 [details] [diff] [review]
Part 4: Separate out table background drawing

+nsMargin
+nsTableFrame::GetBorderDeflationForBackground(nsPresContext* aPresContext) const

Instead of what you're doing here, I'd like to see a GetDeflationForBackground method that accepts an nsMargin reference and returns a PRBool saying whether it's set the margin to anything other than zero. Then PaintTable should call that function itself instead of taking a boolean parameter.

This makes your checks less messy, and it also gives us a good place to expand later. (We'll probably want it to calculate up a border nsMargin, too, kinda like PaintTable is doing currently.)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #7)
> Instead of what you're doing here, I'd like to see a GetDeflationForBackground
> method that accepts an nsMargin reference and returns a PRBool saying whether
> it's set the margin to anything other than zero. Then PaintTable should call
> that function itself instead of taking a boolean parameter.

Why return a PRBool when we can just compare the output margin to nsMargin(0,0,0,0)? One output seems better than two.

Comment 9

[fantasai]

A) Because I don't like comparisons to nsMargin literals when a. B) Because I expect we'll eventually need two outputs (one deflation, one border-substitute), one of which can't be compared to a literal anyway.

If you don't care about B, then add an IsZero() method to nsMargin, that works for me, too.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd kind of like to keep the boolean parameter to PaintTable, so that the control over who paints the background is centralized in nsTableFrame.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4 v2: Using nsMargin::IsZero and GetDeflationForBackground

I've left the boolean parameter in TableBackgroundPainter::PaintTable, because I still think that it's best to have the decision localized to nsTableFrame.

Comment 12

[fantasai]

Comment on attachment 389597 [details] [diff] [review]
Part 4 v2: Using nsMargin::IsZero and GetDeflationForBackground

And I think it's best to have the decision localized to a function like GetDeflationForBackground so that it's only in one place, which is why I wanted it to return a PRBool.

        A call to PaintTable will normally paint all of the table's
-       elements (except the cells in non-BC).
+       elements (except the cells in non-BC, and except the table
+       background in non-BC).

Since the logic you're describing in this comment isn't actually in the function, don't describe it here. It's not even accurate based on what you're doing.

-                                   nsMargin*     aDeflate)
+                                   nsMargin*     aDeflate,
+                                   PRBool        aPaintTableBackground)

Since aDeflate is always passed in now, please make it a reference rather than a pointer. Same for PaintTableFrame().

r=fantasai if you fix those last two

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK thanks

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 389438 [details] [diff] [review]
Part 1: remove unused aBorder parameter to PaintBackgroundLayer

Given the number of times I refactored this code while merging the multiple backgrounds patch with other changes, I'm not surprised it ended up with unused parameters.  r=dbaron

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 389441 [details] [diff] [review]
Part 3: Create DisplayBackgroundUnconditional

r=dbaron

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/c907934cfeb3
http://hg.mozilla.org/mozilla-central/rev/b5bc3aa9111b
http://hg.mozilla.org/mozilla-central/rev/52021f21d6c4
This commit tests that table backgrounds can identify as opaque:
http://hg.mozilla.org/mozilla-central/rev/5b0d9f36c0b3

Comment 17

[Julien "_FrnchFrgg_" RIVAUD]

Table cells bacgrounds (and cell-like, e.g. column backgrounds, which are painted cliped) are still most of the time painted by the TablePainter, right ? So this patch would fix the opacity advertisment of the table bg, but not for the row, col, rowgroup, cells, etc. ?

Or am I mistaken ?

IIUC, this could be one of the adverse effects of the TablePainter which predates and doesn't fit in the new display lists system; it would justify the removal of the backgrounds painting from the TablePainter jurisdiction and generate separate display lists for every cell --- and potentially every layer of the table bg spec.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #17)
> Table cells bacgrounds (and cell-like, e.g. column backgrounds, which are
> painted cliped) are still most of the time painted by the TablePainter, right ?
> So this patch would fix the opacity advertisment of the table bg, but not for
> the row, col, rowgroup, cells, etc. ?

That is correct.

The table background is the most important case I've found so far (other than plain blocks) where we need to know its opacity to get plugin clipping right. The cell backgrounds won't worry me here unless we find a real page that positions a table cell with an opaque background over a plugin. I'd be surprised if we did find one.

Comment 19

[Julien "_FrnchFrgg_" RIVAUD]

IIRC, positionned cells paint their background/border themselves, at least in non-BC, but it probably doesn't solve the problem because they use a custom display item (as I remember it).

I was worrying more about tables used to make a menu (so positionned over plugins), with alternating opaque row/column bacgrounds for effects, and no table background (because it's unneeded). But sure, until we find a real page using that, we don't have to worry (especially since the workaround of putting a dummy opaque bg on the table is easy to use, far easier than what youtube & co do today for Firefox 3)