Integrate backgroundImageStrip into BackgroundPainter #44228
Conversation
|
EWS run on previous version of this PR (hash 0daa300) |
|
@kohler - can you write more in commit message on why you are introducing this change (e.g., This patch do ABC to fix eventually webkit.org/b/xxx and this is to help in xyz way)? |
0daa300 to
6625eca
Compare
|
EWS run on previous version of this PR (hash 6625eca) |
|
@Ahmad-S792 @nt1m This change is ready for review! |
|
@smfr Sorry, I hand-edited the comment at the head of this PR. Is the actual commit message badly formatted? I think this is well-tested by reference tests. Tests I added for #31975 test that the background images on rows & sections still span those dimensions. |
|
Also, tests included in |
|
Ping on this? @smfr |
6625eca to
ca52497
Compare
|
EWS run on previous version of this PR (hash ca52497) |
ca52497 to
a026905
Compare
|
EWS run on previous version of this PR (hash a026905) |
|
This PR remains ready for review and I'd love to know how to move it forward. |
a026905 to
699d448
Compare
|
EWS run on current version of this PR (hash 699d448) |
🧪 style|
EWS run on previous version of this PR (hash 699d448) |
| auto imageRect = backgroundImageStrip.isEmpty() ? scrolledPaintRect : backgroundImageStrip; | ||
| auto paintOffset = backgroundImageStrip.isEmpty() ? rect.location() : backgroundImageStrip.location(); | ||
| auto geometry = calculateBackgroundImageGeometry(m_renderer, m_paintInfo.paintContainer, bgLayer, paintOffset, imageRect, m_overrideOrigin); | ||
| // m_imageRect, if provided, is used for sizing the image. It is set for table rows and for inline boxes (multiline inline boxes paint like the image was one long strip spanning lines). |
There was a problem hiding this comment.
This comment should be next to the member variable declaration.
699d448 to
621f14f
Compare
|
EWS run on current version of this PR (hash 621f14f) |
|
EWS run on previous version of this PR (hash 621f14f) |
|
@smfr Thanks for the review!! I re-pushed, moving the comment you indicated next to the member variable declaration. Sorry for being thick, but how should the commit message be formatted? Is it wrapped to too narrow a width? I've made some changes to the commit message in the latest push; the result looks like other commit messages in recent history. |
We usually don't wrap the commit message fwiw, but honestly it doesn't matter too much |
|
Buildbot blocked the merge because of two 0.01%-off pixel tests on iOS WPT. The failing tests involve table row backgrounds, which is relevant for this commit. The same tests pass on Mac, and in buildbot's iOS results, they also pass on the third try (step 42). I've looked at code differences in WebCore/rendering between the current version and the last version with an all-green test run (a026905), and nothing relevant jumped out. Any advice? |
|
I've spent some time tracking this down and would appreciate some guidance (see end). The problem isn’t visible to my eye. The diff reports small pixel differences (maxDifference=2) exclusively on the edges of some table cells. I focused on this line, in BackgroundPainter.cpp, which clips the painted geometry to the relevant cell boundary: Line 489 The failing version looks like this. auto paintOffset = m_imageRect ? m_imageRect->location() : rect.location();
auto geometry = calculateBackgroundImageGeometry(m_renderer, m_paintInfo.paintContainer, bgLayer, paintOffset, scrolledPaintRect, m_imageRect);
auto& clientForBackgroundImage = backgroundObject ? *backgroundObject : m_renderer;
bgImage->setContainerContextForRenderer(clientForBackgroundImage, geometry.tileSizeWithoutPixelSnapping, m_renderer.style().usedZoom());
geometry.clip(LayoutRect(pixelSnappedRect)); // ***********
RefPtr<Image> image;
bool isFirstLine = inlineBoxIterator && inlineBoxIterator->lineBox()->isFirst();
if (!geometry.destinationRect.isEmpty() && (image = bgImage->image(backgroundObject ? backgroundObject : &m_renderer, geometry.tileSize, isFirstLine))) {But this clipping has a slightly different effect than clipping in the graphics context (i.e., basically at the pixel level) and painting a larger geometry. Apparently, the WPT test expects the more expensive graphics-context clip. I tried a couple versions of the highlighted line that pass the if (m_imageRect) {
backgroundClipStateSaver.ensureSave();
context.clip(LayoutRect(pixelSnappedRect));
} else
geometry.clip(LayoutRect(pixelSnappedRect));This one makes the most sense to me; it paints a slightly-larger geometry: geometry.clip(LayoutRect(pixelSnappedRect));
if (m_imageRect) {
backgroundClipStateSaver.ensureSave();
context.clip(LayoutRect(pixelSnappedRect));
geometry.destinationRect.inflate(1_lu);
}I'm checking in that version so other tests kick off. So the original patch, which clipped only the painted geometry, seems like it should work, but it subtly violates a WPT test because of something like sub-pixel bleeding at cell edges. Expanding the painted geometry, as the patch above does, passes the test. Is that a satisfying solution? |
621f14f to
c8d7ece
Compare
|
EWS run on previous version of this PR (hash c8d7ece) |
c8d7ece to
0ee1625
Compare
https://bugs.webkit.org/show_bug.cgi?id=287503 Reviewed by Simon Fraser. There are several cases in CSS where a image sized for one box should be painted clipped to another box. One case is a background for an inline element that spans multiple lines (the background image should be sized to the union of the line-boxes, but painted into the line-boxes individually). Another case is the background for table rows and table sections (the background image should be sized to the row and/or section, but painted into the individual cells). But previous code handled these two cases differently. Here we unify the implementations in a BackgroundPainter member, imageRect, which defines the sizing box for the background image. Both inline backgrounds and row/section backgrounds call BackgroundPainter::setImageRect to set the sizing box. This does not change behavior on its own, but is an important simplification towards fixing bug 287503. This commit also removes unnecessary graphics state saving and clipping from RenderTableCell::paintBackgroundsBehindCell. It suffices to shrink the rectangle being painted; additional clipping is redundant. * Source/WebCore/platform/graphics/GraphicsContextStateSaver.h: (WebCore::GraphicsContextStateSaver::ensureSave): New function. * Source/WebCore/rendering/BackgroundPainter.cpp: (WebCore::BackgroundPainter::paintFillLayers const): (WebCore::BackgroundPainter::paintFillLayer const): (WebCore::BackgroundPainter::calculateBackgroundImageGeometry): The BackgroundPainter::imageRect member, if set, defines the dimensions of a background image. * Source/WebCore/rendering/BackgroundPainter.h: (WebCore::BackgroundPainter::setImageRect): New function. (WebCore::BackgroundPainter::setOverrideOrigin): Deleted. (WebCore::BackgroundPainter::paintFillLayer): Deleted. * Source/WebCore/rendering/InlineBoxPainter.cpp: (WebCore::InlineBoxPainter::paintFillLayer): Use setImageRect. * Source/WebCore/rendering/RenderTableCell.cpp: (WebCore::RenderTableCell::paintBackgroundsBehindCell): Use setImageRect.
0ee1625 to
302e2b2
Compare
|
EWS run on current version of this PR (hash 302e2b2) |
|
EWS run on previous version of this PR (hash 0ee1625) |
|
The current commit, 0ee1625, passes all tests. (The clipped-filter failure seems unrelated; it has been failing on ios-wk2 on recent commits.) The relevant part of LayoutRect clipRect(pixelSnappedRect);
if (m_imageRect) {
backgroundClipStateSaver.ensureSave();
context.clip(clipRect);
clipRect.inflate(1_lu);
}
geometry.clip(clipRect);I added a However, I think that it would be better to use the original code instead (which simply does |
|
|
||
| auto& clientForBackgroundImage = backgroundObject ? *backgroundObject : m_renderer; | ||
| bgImage->setContainerContextForRenderer(clientForBackgroundImage, geometry.tileSizeWithoutPixelSnapping, m_renderer.style().usedZoom()); | ||
|
|
||
| geometry.clip(LayoutRect(pixelSnappedRect)); | ||
| LayoutRect clipRect(pixelSnappedRect); |
There was a problem hiding this comment.
This is wrong; pixelSnappedRect is a FloatRect for painting, and should never be cast back to a LayoutRect (which in theory is lossy).
There was a problem hiding this comment.
OK, but as you can see in the diff, the old code clipped the geometry to pixelSnappedRect.
I can trace that code back to 2014 (a06542c).
| context.clip(clipRect); | ||
| clipRect.inflate(1_lu); | ||
| } | ||
| geometry.clip(clipRect); |
There was a problem hiding this comment.
BackgroundImageGeometry contains layout (unsnapped) geometry; it's wrong to clip to a paint-snapped rect here. Maybe this should clip to rect ?
There was a problem hiding this comment.
BackgroundImageGeometry actually contains snapped geometry. The pixelSnapBackgroundImageGeometryForPainting function snaps the geometry to pixels. Every constructed BackgroundImageGeometry object was first pixel-snapped by that function.
|
@smfr asked about clipping to a non-snapped rectangle— The code currently checked in, LayoutRect clipRect(pixelSnappedRect);
if (m_imageRect) {
backgroundClipStateSaver.ensureSave();
context.clip(clipRect);
clipRect.inflate(1_lu);
}
geometry.clip(clipRect);passes the tests in fast/table and imported/w3c/web-patform-tests/css/CSS2/tables on iphone-simulator. This code, geometry.clip(rect);fails The failures on the This code, LayoutRect clipRect(rect);
if (m_imageRect) {
backgroundClipStateSaver.ensureSave();
context.clip(clipRect);
clipRect.inflate(1_lu);
}
geometry.clip(clipRect);badly fails So the code currently in this patch passes these tests. Using a non-snapped rectangle reliably fails some tests with significant visual differences. It seems best to either use the code in the patch, or use |
|
This meta tag allows the problematic |
|
(Consider reaching out on webkit.slack.com , people should be more responsive there) |
302e2b2
🛠 playstation