Bug 235900 – REGRESSION(r166784): Accumulated pixel rounding errors cause gradient color stops to be misaligned

• Component: Layout and Rendering
• Status: NEW

[Simon Fraser (smfr)]

WebKit has misaligned gradient color stops in this test:
https://codepen.io/thebabydino/pen/KKydBxw

Reported by Ana Tudor: https://twitter.com/anatudor/status/1487853791147868162

[Myles C. Maxfield]

div {
  min-height: 8em;
  background: linear-gradient(90deg, #f00 1px, #0000 0) 0/8em, linear-gradient(90deg, #00f 1px, #0000 0) 0/1.6em 50% repeat-x;
}
div + div {
  --w: min(40em, 100vw - 2em);
  background-size: calc(var(--w)/5), calc(var(--w)/25) 50%;
}

[Myles C. Maxfield]

RenderBoxModelObject::calculateBackgroundImageGeometry() calls pixelSnapBackgroundImageGeometryForPainting().

[Myles C. Maxfield]

Commenting out these two lines in pixelSnapBackgroundImageGeometryForPainting() makes the bug go away:

tileSize = LayoutSize(snapRectToDevicePixels(LayoutRect(destinationRect.location(), tileSize), scaleFactor).size());
phase = LayoutSize(snapRectToDevicePixels(LayoutRect(destinationRect.location(), phase), scaleFactor).size());

but I don't know if that's a safe change.

Assigning to Alan.

[Myles C. Maxfield]

Thinking more about this, the correct answer probably isn't to not pixel snap, but instead would probably be to repeatedly place the images, and then pixel snap their left and right edges (as opposed to their widths).

[Myles C. Maxfield]

Also, if this is a paint-time operation, it probably shouldn't be using LayoutUnits...

Maybe?

[Radar WebKit Bug Importer]

<rdar://problem/88309058>

[alan]

Created attachment 453341 [details]
Test reduction

[alan]

>background: linear-gradient(90deg, black 1px, transparent 0) 0px/ 128px, linear-gradient(90deg, red 1px, transparent 0) 0px/ 25.6px 50px repeat-x;
I think this example should make every black line overlap a red line (1st, 6th, 11th...). However pixel-snapping translates the 25.6px value to 25.5px and while the second black line only misses the 6th red line by 0.5px (1 devices pixel on retina displays) these 0.1px pixel diffs add up on repeating backgrounds producing larger gaps over time.
I believe the solution is what Myles suggested above; not pixel-snap the tileSize, phase and space but only the destination rect.
Something like this:

-static void pixelSnapBackgroundImageGeometryForPainting(LayoutRect& destinationRect, LayoutSize& tileSize, LayoutSize& phase, LayoutSize& space, float scaleFactor)
-{
-    tileSize = LayoutSize(snapRectToDevicePixels(LayoutRect(destinationRect.location(), tileSize), scaleFactor).size());
-    phase = LayoutSize(snapRectToDevicePixels(LayoutRect(destinationRect.location(), phase), scaleFactor).size());
-    space = LayoutSize(snapRectToDevicePixels(LayoutRect(LayoutPoint(), space), scaleFactor).size());
-    destinationRect = LayoutRect(snapRectToDevicePixels(destinationRect, scaleFactor));
-}
-
 bool RenderBoxModelObject::fixedBackgroundPaintsInLocalCoordinates() const
 {
     if (!isDocumentElementRenderer())
@@ -1387,7 +1379,7 @@ BackgroundImageGeometry RenderBoxModelObject::calculateBackgroundImageGeometry(c
     }
 
     destinationRect.intersect(borderBoxRect);
-    pixelSnapBackgroundImageGeometryForPainting(destinationRect, tileSize, phase, spaceSize, deviceScaleFactor);
+    destinationRect = LayoutRect { snapRectToDevicePixels(destinationRect, deviceScaleFactor) };   <- Ignore that we still use LayoutRect type here instead of FloatRect.
     return BackgroundImageGeometry(destinationRect, tileSize, phase, spaceSize, fixedAttachment);
 }

I'd be more confident if someone with better understanding on background images could chime in though.

[Ahmad Saleem]

I am still able to reproduce this bug on WebKit ToT (265489@main) using attached test case from Alan where the 'red' does not cover 'blue' lines and have gaps while Chrome Canary 116 and Firefox Nightly 116 works properly and have no gaps.