Bug 9268

Summary: Quirksmode: Backgrounds on table rows
Product: WebKit Reporter: Adele Peterson <adele>
Component: CSSAssignee: Daniel Bates <dbates>
Status: NEW ---    
Severity: Normal CC: aaronlevbugs, ahmad.saleem792, ap, bdakin, bfulgham, bugs-webkit, darin, dbates, dglazkov, eric, gjunge, gustavo, hyatt, ian, james, kyle.bavender, mathias, mitz, mupo, ossy, rniwa, robin, simon.fraser, trinler, webkit-bug-importer, webkit.review.bot, zalan, zcorpan
Priority: P2 Keywords: InRadar
Version: 420+   
Hardware: PC   
OS: OS X 10.4   
URL: http://agachi.name/tests/th-background.htm
Bug Depends on:    
Bug Blocks: 9610    
Attachments:
Description Flags
standalone test case
none
Patch
none
Patch
none
Patch with test cases
bdakin: review-
Patch
none
Patch with test cases
none
Patch 1 of 2
none
Patch 2 of 2: Test cases
none
new Qt specific expected files for tests introduced in r56655 none

Description Adele Peterson 2006-06-02 14:58:46 PDT
http://www.quirksmode.org/bugreports/archives/safari/index.html

"A background on a TR doesn't work. Instead the TDs inherit the TR's background image.

Test page http://agachi.name/tests/th-background.htm
Workaround is not included
Reported by: Valentin Agachi."
Comment 1 Joost de Valk (AlthA) 2006-06-24 14:04:16 PDT
Changing component, weird stuff :)
Comment 2 Robert Blaut 2009-01-01 23:32:44 PST
Created attachment 26361 [details]
standalone test case
Comment 3 Daniel Bates 2009-12-02 19:32:33 PST
As per section 14.2 of the CSS 2.1 spec <http://www.w3.org/TR/CSS2/colors.html#background>, "Background properties are not inherited...". Moreover, by (5) of section 17.5.1 <http://www.w3.org/TR/CSS2/tables.html#table-layers>, "...the background covers exactly the full area of all cells that originate in the row...".

As of this writing, Firefox (3.5.5) correctly handles backgrounds on <tr>s. But Safari (WebKit r51473), and IE 8 do not.
Comment 4 Daniel Bates 2009-12-02 19:35:14 PST
Created attachment 44202 [details]
Patch

Work in progress.

This patch correct the issue. However, there are some weird rendering artifacts that may appear when you try to select text within a column in a row with a background-image and/or the background of the selection will be some re-positioned portion of the row's background image.

In the standalone example <https://bugs.webkit.org/attachment.cgi?id=26361>, if you click anywhere to the right of the table you will see that the background of the first row starts to tile on the right-most side of the "TH 3" cell. Also, if you try to select the text "TH 3" then click somewhere to deselect the selection, the selection rectangle will visibly disrupt the diagonal in the background. That is, it looks like we didn't paint the same portion of background that was under the selection. Instead, we painted some other portion of the row background.

I suspect that the phase in x is wrong during the selection and I think the correct phase in x should be the default (the one we calculated using calculateBackgroundImageGeometry plus the bit phase += destRect.location() - destOrigin), but I haven't looked at the values of destRect.location() and destOrigin yet nor have a solid understanding how the selection code influences what we should re-paint. It looks like we issue a repaint of only the rect of the selection.
Comment 5 Daniel Bates 2009-12-02 19:36:26 PST
I am CC'ing David Hyatt, Beth Dakin, Dan Bernstein, and Eric Seidel in the hope that they may recognize what's going on with the selection and/or have some suggestions/insight on the patch/approach as a whole. Also, Adele, any insight would be appreciated.
Comment 6 Daniel Bates 2009-12-02 19:37:02 PST
*** Bug 28299 has been marked as a duplicate of this bug. ***
Comment 7 Daniel Bates 2009-12-02 19:37:23 PST
*** Bug 19315 has been marked as a duplicate of this bug. ***
Comment 8 Daniel Bates 2009-12-05 00:20:47 PST
Created attachment 44349 [details]
Patch

Fixed selection issue as well as an issue when scrolling a web page whose window was smaller than the row width.

I am open to suggestions on how to better name variables and functions.

Will work on a layout test tomorrow.
Comment 9 Daniel Bates 2009-12-06 11:10:39 PST
Created attachment 44362 [details]
Patch with test cases

Re-factored code to store phase in x and painted background rectangle in RenderTableCell, instead of trying to calculate this on the fly.

Added render tree/pixel tests results for the Mac and Win builds.
Comment 10 WebKit Review Bot 2009-12-06 11:11:22 PST
style-queue ran check-webkit-style on attachment 44362 [details] without any errors.
Comment 11 Darin Adler 2009-12-08 09:32:39 PST
Hyatt, Mitz, Beth, are one of you going to review this?
Comment 12 Beth Dakin 2009-12-08 16:56:56 PST
I'm still drinking this patch in, and I don't think I feel ready to review it. I will talk to Dan Bernstein, and perhaps we can go over this together.
Comment 13 Darin Adler 2010-01-05 14:57:13 PST
(In reply to comment #11)
> Hyatt, Mitz, Beth, are one of you going to review this?

Ping.
Comment 14 Beth Dakin 2010-01-05 15:38:44 PST
Comment on attachment 44362 [details]
Patch with test cases

Hi Daniel! I am sorry it took so long to get back to you on this. Dan and I talked this over today, and here are some of out thoughts.

First, the specification that you link to outlines a whole list of how tables should be painted, and this patch addresses one of the items on the list. Do you know if we do all of these things correctly right now? I suspect that we do not, but I could be wrong. It would be good to have a set of tests to know exactly where we stand on all of these issues before we try to fix one of the issues since we may want to take a different approach entirely if we want to try to fix them all at once. At the very least, it would be good to have bugs representing any of the other issues and to have a vague plan about how they might be fixed just to feel confident that all of the code to fix this issue wouldn't have to be re-written to fix the others. 


> Index: WebCore/rendering/RenderBoxModelObject.cpp
> ===================================================================> @@ -457,11 +459,32 @@ void RenderBoxModelObject::paintFillLaye
>          IntPoint phase;
>          IntSize tileSize;
>  
> +        RenderTableCell* cell = isTableCell() ? toRenderTableCell(this) : 0;
> +        RenderTableRow* row = cell ? toRenderTableRow(cell->parent()) : 0;
> +        bool isBackgroundImageForRow = row && bgLayer == row->style()->backgroundLayers();
> +


I don't like having all of this table-specific code in paintFillLayerExtended(). Generally, this is something we try to avoid in RenderBoxModelObject via polymorphism. I know that paintFillLayerExtended() is not virtual right now, but it might be worth considering making it virtual. Dan and I doubt there would be a performance hit for that. On the other hand, you could add a new virtual function, something like doNewBackgoundStuff() (but with a better name) that would do nothing in the base class (RenderBoxModelObject) but all kinds of things in RenderTable.

> +        if (isBackgroundImageForRow && !cell->col()) {
> +            // We calculate and store the background image geometry for the table-row with respect to the
> +            // first cell's (tx, ty) coordinates. We will use this geometry to determine which table cells
> +            // need to be painted (via intersection test) and what part of the background to paint in them.
> +            calculateBackgroundImageGeometry(bgLayer, tx, ty, row->width(), row->height(), destRect, phase, tileSize);
> +            row->setBackgroundDestRect(destRect);
> +            row->setBackgroundPhase(phase);
> +        }

Dan pointed out to me that this is insufficient. A lot of painting occurs incrementally and only certain cells get repainted. When a subset of cells is being repainted that doesn't include the first cell in the row, this will go awry. 


> +        if (cell && cell->paintedBackgroundRect().width() < destRect.width())

I don't understand this comparison.Can you explain it to me? This probably deserves a comment.

> +    IntRect m_paintedBackgroundRect;
> +    int m_paintedBackgroundPhaseX;
>  };


I don't like the idea of adding these bits to RenderTableCell. I don't like it because generally it's good to use fewer bits, and because very few RenderTableCells will actually even use these bits. I think it would be better to do these computations on the fly.
Comment 15 Daniel Bates 2010-01-19 12:44:09 PST
(In reply to comment #14)
> (From update of attachment 44362 [details])
> Hi Daniel! I am sorry it took so long to get back to you on this. Dan and I
> talked this over today, and here are some of out thoughts.
> 
> First, the specification that you link to outlines a whole list of how tables
> should be painted, and this patch addresses one of the items on the list. Do
> you know if we do all of these things correctly right now? I suspect that we do
> not, but I could be wrong. It would be good to have a set of tests to know
> exactly where we stand on all of these issues before we try to fix one of the
> issues since we may want to take a different approach entirely if we want to
> try to fix them all at once. At the very least, it would be good to have bugs
> representing any of the other issues and to have a vague plan about how they
> might be fixed just to feel confident that all of the code to fix this issue
> wouldn't have to be re-written to fix the others. 

Yes. I am aware of related issues with respect to background images for column and row groups.
I agree, the preferred solution is to fix everything in one shot. So, I will look into this.

> 
> > Index: WebCore/rendering/RenderBoxModelObject.cpp
> > ===================================================================
> …
> > @@ -457,11 +459,32 @@ void RenderBoxModelObject::paintFillLaye
> >          IntPoint phase;
> >          IntSize tileSize;
> >  
> > +        RenderTableCell* cell = isTableCell() ? toRenderTableCell(this) : 0;
> > +        RenderTableRow* row = cell ? toRenderTableRow(cell->parent()) : 0;
> > +        bool isBackgroundImageForRow = row && bgLayer == row->style()->backgroundLayers();
> > +
> 
> 
> I don't like having all of this table-specific code in
> paintFillLayerExtended(). Generally, this is something we try to avoid in
> RenderBoxModelObject via polymorphism. I know that paintFillLayerExtended() is
> not virtual right now, but it might be worth considering making it virtual. Dan
> and I doubt there would be a performance hit for that. On the other hand, you
> could add a new virtual function, something like doNewBackgoundStuff() (but
> with a better name) that would do nothing in the base class
> (RenderBoxModelObject) but all kinds of things in RenderTable.

Yes, I will look to clean this up.

> > +        if (isBackgroundImageForRow && !cell->col()) {
> > +            // We calculate and store the background image geometry for the table-row with respect to the
> > +            // first cell's (tx, ty) coordinates. We will use this geometry to determine which table cells
> > +            // need to be painted (via intersection test) and what part of the background to paint in them.
> > +            calculateBackgroundImageGeometry(bgLayer, tx, ty, row->width(), row->height(), destRect, phase, tileSize);
> > +            row->setBackgroundDestRect(destRect);
> > +            row->setBackgroundPhase(phase);
> > +        }
> 
> Dan pointed out to me that this is insufficient. A lot of painting occurs
> incrementally and only certain cells get repainted. When a subset of cells is
> being repainted that doesn't include the first cell in the row, this will go
> awry. 

Will look into this.

> 
> > +        if (cell && cell->paintedBackgroundRect().width() < destRect.width())
> 
> I don't understand this comparison.Can you explain it to me? This probably
> deserves a comment.

If I recall correctly (been I while since I looked at this), I did this to handle the case where the destRect is the rectangle that represents that scroll difference when you scroll a page that overflow vertically. Notice, the width of this rectangle <= the width of the rectangle that represents the table cell whose background we want to paint (*). To paint the background of table cell i, I calculate the phase in X to shift the background image, call it P_i = (\sum_{j=0}^{i-1} P_j) + "painted background width of cell i - 1", where "painted background width of cell i - 1" reflects the width of the largest area painted of cell i - 1 so far (**). Notice, the largest such area is the area of the entire table cell. Suppose the current vertical scroll position is such that table cell i is completely outside of the view and table cell i - 1 is slightly outside of the view. Assume I call cell->setPaintedBackgroundRect(destRect) when destRect.width() < cell->paintedBackgroundRect().width(). Then (**) is violated. In particular, this can happen whenever you scroll (by (*)).

> > +    IntRect m_paintedBackgroundRect;
> > +    int m_paintedBackgroundPhaseX;
> >  };
> 
> 
> I don't like the idea of adding these bits to RenderTableCell. I don't like it
> because generally it's good to use fewer bits, and because very few
> RenderTableCells will actually even use these bits. I think it would be better
> to do these computations on the fly.

Will look into this.
Comment 16 Daniel Bates 2010-03-07 20:42:48 PST
Created attachment 50185 [details]
Patch

Work in progress, need additional layout tests.

This patch works for table rows, row groups, cells, columns and column groups (*).

(*) Background positioning for columns and column groups does not work because we need to implement or implement equivalents of RenderBox::absoluteContentBox(), RenderBox::width(), and RenderBox::height() with respect to <col> elements. See FIXME comment in method RenderBoxModelObject::calculateBackgroundImageGeometry in the patch. Moreover, implementing these methods or similar methods would bring us closer to resolving other bugs regarding <col> elements, including bug #15277.
Comment 17 Daniel Bates 2010-03-14 20:52:05 PDT
Created attachment 50680 [details]
Patch with test cases

Added additional layout tests. Separated out column group and column code into separate cases.

With regards to the background positioning issues for columns and column groups, I believe it is best to resolve this in a separate bug since, as aforementioned, computing the absolute content box (or equivalent) for the column/column group is useful to resolve other bugs. So, I filed bug #36104 and noted this in the ChangeLog and FIXME comment in method RenderBoxModelObject::calculateBackgroundImageGeometry.

Will look this patch over some more tomorrow to finalize. I am open to any suggestions on this patch.
Comment 18 Daniel Bates 2010-03-21 00:14:29 PDT
Created attachment 51242 [details]
Patch 1 of 2

Resolved issues with borders, simplified expressions for xPosition, and yPosition, and removed unnecessary variables.

Split the code changes from the layout tests since the patch was becoming large.
Comment 19 Daniel Bates 2010-03-21 00:15:16 PDT
Created attachment 51243 [details]
Patch 2 of 2: Test cases

Layout tests.
Comment 20 Beth Dakin 2010-03-25 12:29:10 PDT
Comment on attachment 51242 [details]
Patch 1 of 2

Looks good!
Comment 21 Daniel Bates 2010-03-26 18:57:13 PDT
*** Bug 25284 has been marked as a duplicate of this bug. ***
Comment 22 Daniel Bates 2010-03-26 18:58:13 PDT
*** Bug 15036 has been marked as a duplicate of this bug. ***
Comment 23 Daniel Bates 2010-03-26 22:09:23 PDT
Committed r56655: <http://trac.webkit.org/changeset/56655>
Comment 24 Csaba Osztrogonác 2010-03-29 02:23:36 PDT
Created attachment 51889 [details]
new Qt specific expected files for tests introduced in r56655
Comment 25 Csaba Osztrogonác 2010-03-29 05:25:28 PDT
(In reply to comment #24)
> Created an attachment (id=51889) [details]
> new Qt specific expected files for tests introduced in r56655

Committed: http://trac.webkit.org/changeset/56717
Comment 26 Eric Seidel (no email) 2010-03-31 17:22:10 PDT
Build 4423 (r56703) was the first to show failures: set([u'fast/table/table-row-group-background.html', u'fast/table/table-row-group-background-positioned.html', u'fast/table/table-row-background.html'])

The Gtk bot was offline when this went in, but it appears that this checkin broke the Gtk 32-bit Debug bot.  Please fix.
Comment 27 Daniel Bates 2010-03-31 17:36:02 PDT
(In reply to comment #26)
> Build 4423 (r56703) was the first to show failures:
> set([u'fast/table/table-row-group-background.html',
> u'fast/table/table-row-group-background-positioned.html',
> u'fast/table/table-row-background.html'])
> 
> The Gtk bot was offline when this went in, but it appears that this checkin
> broke the Gtk 32-bit Debug bot.  Please fix.

The result for build 56875 (the latest as of the time of writing) show that these tests are new. That is, they are not failing. I do not have a GTK build, so as of right now I can only checkin the render tree results for these tests from the build bot. However, these tests need pixel test results and these test results need to be verified by hand to confirm that they match the descriptive text in the test case. I'll look to try to get a GTK setup shortly.
Comment 28 Eric Seidel (no email) 2010-03-31 17:37:29 PDT
Hmmm... Sorry.  I'll tweak my script to ignore new tests.
Comment 29 Eric Seidel (no email) 2010-03-31 17:57:04 PDT
I've fixed the new command (bug 36911) to ignore new tests.
Comment 30 Daniel Bates 2010-04-08 12:20:28 PDT
This patch caused some regressions in the mozilla and mozilla expected failure test cases. An attempt to fix this regression was proposed in bug #37075.

We decided to rollout this patch for now so that we can resolve these issues.
Comment 31 Daniel Bates 2010-04-08 12:22:30 PDT
Rollout of change set 56655 committed in change set 57287 <http://trac.webkit.org/changeset/57287>.
Comment 32 Daniel Bates 2010-04-08 12:23:12 PDT
Comment on attachment 51242 [details]
Patch 1 of 2

Clearing review flag on patch 1 of 2.
Comment 33 Daniel Bates 2010-04-08 12:23:30 PDT
Comment on attachment 51243 [details]
Patch 2 of 2: Test cases

Clearing review flag on patch 2 of 2.
Comment 34 Daniel Bates 2010-04-08 12:48:32 PDT
Rollout of change set 56717 (*) committed in change set 57289
<http://trac.webkit.org/changeset/57289>.

(*) These were the Qt expected results for the layout test included in attachment <https://bugs.webkit.org/attachment.cgi?id=51243>.
Comment 35 Daniel Bates 2010-04-08 12:48:56 PDT
Comment on attachment 51889 [details]
new Qt specific expected files for tests introduced in r56655

Clearing review flag on this.
Comment 36 Daniel Bates 2010-04-08 13:47:32 PDT
For completeness, we also need to rollout changes made in bug #36799. In particular, a partial rollout of change set 56766 <http://trac.webkit.org/changeset/56766> and a complete rollout of change set 56771 <http://trac.webkit.org/changeset/56771>. At the time of writing, rollout patches are pending review and I have talked with both Ojan Vafai and Dimitri Glazkov on IRC.
Comment 37 Daniel Bates 2010-04-08 14:15:10 PDT
(In reply to comment #36)
> For completeness, we also need to rollout changes made in bug #36799. In
> particular, a partial rollout of change set 56766
> <http://trac.webkit.org/changeset/56766> and a complete rollout of change set
> 56771 <http://trac.webkit.org/changeset/56771>. At the time of writing, rollout
> patches are pending review and I have talked with both Ojan Vafai and Dimitri
> Glazkov on IRC.

Committed fix for bug #36799 in change set 57291 <http://trac.webkit.org/changeset/57291>. See bug #36799 for more details.
Comment 38 Daniel Bates 2011-06-02 06:44:14 PDT
*** Bug 34392 has been marked as a duplicate of this bug. ***
Comment 39 Daniel Bates 2011-07-04 06:09:19 PDT
*** Bug 50909 has been marked as a duplicate of this bug. ***
Comment 40 Simon Fraser (smfr) 2012-08-10 14:34:45 PDT
<rdar://problem/11446455>
Comment 41 Kyle Bavender 2017-09-12 06:14:19 PDT
I came here after seeing that Chrome has fixed their version of this bug. Linear-gradients for backgrounds of table rows now work ( https://bugs.chromium.org/p/chromium/issues/detail?id=35697 ). 

Noticed that the bug's URL no longer works. Here's a demo that could be used: http://jsfiddle.net/u246egu5/
Comment 42 Ahmad Saleem 2022-07-12 07:34:57 PDT
I am able to reproduce this bug in Safari 15.5 and Safari Technical Preview 148 on macOS 12.4 based on JSFiddle from Comment 41. All other browsers (Chrome Canary 105 and Firefox Nightly 104) match with each other. Thanks!