(Regression r163262) Subpixel rendering: Text jumps up and down while adjacent <img>'s border size is transitioning.

[zalan]

Hover Divisions image on http://www.imgtec.com/about/

[zalan]

Created attachment 232616 [details]
Test reduction

[zalan]

It is actually caused by the integral rounding in RenderReplaced::computeReplacedLogicalHeight(). 

1. animation sets fractional values on the border. 0.25px
2. the calculated box height always snaps to integral value. 399.5 (400px box height - 0.5px border size)-> snaps to 400px
3. 400px + 0.5px border makes the box taller by 0.5px;
 
patch is coming up.

[zalan]

<rdar://problem/17317439>

[zalan]

This changeset fixes the problem, but unfortunately it also introduces inline positioning regression. (because baseline for inlines is int) tracked here: https://bugs.webkit.org/show_bug.cgi?id=133932

 
diff --git a/Source/WebCore/rendering/RenderReplaced.cpp b/Source/WebCore/rendering/RenderReplaced.cpp
index 82e9c14..3d8ffe4 100644
--- a/Source/WebCore/rendering/RenderReplaced.cpp
+++ b/Source/WebCore/rendering/RenderReplaced.cpp
@@ -375,7 +375,6 @@ LayoutUnit RenderReplaced::computeReplacedLogicalWidth(ShouldComputePreferred sh
         return computeReplacedLogicalWidthRespectingMinMaxWidth(computeReplacedLogicalWidthUsing(style().logicalWidth()), shouldComputePreferred);
 
     RenderBox* contentRenderer = embeddedContentBox();
-
     // 10.3.2 Inline, replaced elements: http://www.w3.org/TR/CSS21/visudet.html#inline-replaced-width
     double intrinsicRatio = 0;
     FloatSize constrainedSize;
@@ -394,10 +393,8 @@ LayoutUnit RenderReplaced::computeReplacedLogicalWidth(ShouldComputePreferred sh
             // If 'height' and 'width' both have computed values of 'auto' and the element has no intrinsic width, but does have an intrinsic height and intrinsic ratio;
             // or if 'width' has a computed value of 'auto', 'height' has some other computed value, and the element does have an intrinsic ratio; then the used value
             // of 'width' is: (used height) * (intrinsic ratio)
-            if (intrinsicRatio && ((computedHeightIsAuto && !hasIntrinsicWidth && hasIntrinsicHeight) || !computedHeightIsAuto)) {
-                LayoutUnit logicalHeight = computeReplacedLogicalHeight();
-                return computeReplacedLogicalWidthRespectingMinMaxWidth(roundToInt(round(logicalHeight * intrinsicRatio)), shouldComputePreferred);
-            }
+            if (intrinsicRatio && ((computedHeightIsAuto && !hasIntrinsicWidth && hasIntrinsicHeight) || !computedHeightIsAuto))
+                return computeReplacedLogicalWidthRespectingMinMaxWidth(computeReplacedLogicalHeight() * intrinsicRatio, shouldComputePreferred);
 
             // If 'height' and 'width' both have computed values of 'auto' and the element has an intrinsic ratio but no intrinsic height or width, then the used value of
             // 'width' is undefined in CSS 2.1. However, it is suggested that, if the containing block's width does not itself depend on the replaced element's width, then
@@ -440,7 +437,6 @@ LayoutUnit RenderReplaced::computeReplacedLogicalHeight() const
         return computeReplacedLogicalHeightRespectingMinMaxHeight(computeReplacedLogicalHeightUsing(style().logicalHeight()));
 
     RenderBox* contentRenderer = embeddedContentBox();
-
     // 10.6.2 Inline, replaced elements: http://www.w3.org/TR/CSS21/visudet.html#inline-replaced-height
     double intrinsicRatio = 0;
     FloatSize constrainedSize;
@@ -448,7 +444,6 @@ LayoutUnit RenderReplaced::computeReplacedLogicalHeight() const
 
     bool widthIsAuto = style().logicalWidth().isAuto();
     bool hasIntrinsicHeight = constrainedSize.height() > 0;
-
     // If 'height' and 'width' both have computed values of 'auto' and the element also has an intrinsic height, then that intrinsic height is the used value of 'height'.
     if (widthIsAuto && hasIntrinsicHeight)
         return computeReplacedLogicalHeightRespectingMinMaxHeight(constrainedSize.height());
@@ -456,7 +451,7 @@ LayoutUnit RenderReplaced::computeReplacedLogicalHeight() const
     // Otherwise, if 'height' has a computed value of 'auto', and the element has an intrinsic ratio then the used value of 'height' is:
     // (used width) / (intrinsic ratio)
     if (intrinsicRatio)
-        return computeReplacedLogicalHeightRespectingMinMaxHeight(roundToInt(round(availableLogicalWidth() / intrinsicRatio)));
+        return computeReplacedLogicalHeightRespectingMinMaxHeight(availableLogicalWidth() / intrinsicRatio);
 
     // Otherwise, if 'height' has a computed value of 'auto', and the element has an intrinsic height, then that intrinsic height is the used value of 'height'.
     if (hasIntrinsicHeight)

[Ahmad Saleem]

I am able to reproduce this bug in Safari 15.5 on macOS 12.4 using attached test case (Although picture is broken now).

** Safari behavior:

Image (broken) ease-in while slowly transitioning but below <div> is jumpy/shimmering and show fast flickering (can be observed based on border shimmering).

** Firefox behavior (IDEAL - IMO):

Image (broken) ease-in while slowly transitioning but below <div> does not show any strange behavior as Safari.

** Chrome behavior:

Image (broken) ease-in BUT PUSHES Image out while slowly transitioning but below <div> does not show any strange behavior as Safari.

___

I am not clear whether Safari want to achieve Firefox or Chrome behavior but I think Firefox behavior seems more reasonable. Thanks!

[Ahmad Saleem]

@Alan - it seems Safari Technology Preview 160 is not making <div> jump or stutter as much as before or as Safari 16.2, is something fixed or reduced this happening further?

Can you corroborate my above understanding?

[zalan]

Yeah, this seems to have been fixed (tested with WebKit nightly and code reading)