Bug 9268

Changing component, weird stuff :)

Created attachment 26361 [details] standalone test case

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.

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.

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.

*** Bug 28299 has been marked as a duplicate of this bug. ***

*** Bug 19315 has been marked as a duplicate of this bug. ***

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.

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.

style-queue ran check-webkit-style on attachment 44362 [details] without any errors.

Hyatt, Mitz, Beth, are one of you going to review this?

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.

(In reply to comment #11) > Hyatt, Mitz, Beth, are one of you going to review this? Ping.

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.

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

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.

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.

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.

Created attachment 51243 [details] Patch 2 of 2: Test cases Layout tests.

Comment on attachment 51242 [details] Patch 1 of 2 Looks good!

*** Bug 25284 has been marked as a duplicate of this bug. ***

*** Bug 15036 has been marked as a duplicate of this bug. ***

Committed r56655: <http://trac.webkit.org/changeset/56655>

Created attachment 51889 [details] new Qt specific expected files for tests introduced in r56655

(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

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.

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

Hmmm... Sorry. I'll tweak my script to ignore new tests.

I've fixed the new command (bug 36911) to ignore new tests.

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.

Rollout of change set 56655 committed in change set 57287 <http://trac.webkit.org/changeset/57287>.

Comment on attachment 51242 [details] Patch 1 of 2 Clearing review flag on patch 1 of 2.

Comment on attachment 51243 [details] Patch 2 of 2: Test cases Clearing review flag on patch 2 of 2.

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 on attachment 51889 [details] new Qt specific expected files for tests introduced in r56655 Clearing review flag on this.

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.

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

*** Bug 34392 has been marked as a duplicate of this bug. ***

*** Bug 50909 has been marked as a duplicate of this bug. ***

<rdar://problem/11446455>

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/

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!

Pull request: https://github.com/WebKit/WebKit/pull/31975

Committed 282122@main (d59f182d669b): <https://commits.webkit.org/282122@main> Reviewed commits have been landed. Closing PR #31975 and removing active labels.