fix(ios): prevent pixel-grid rounding from clipping text - #57698
Closed
fabriziocucci wants to merge 1 commit into
Closed
fix(ios): prevent pixel-grid rounding from clipping text#57698fabriziocucci wants to merge 1 commit into
fabriziocucci wants to merge 1 commit into
Conversation
Summary: Re-lands the iOS text measurement rounding fix from PR react#54260 / D85438723, which was reverted by D86791267. This version adds unit test coverage and moves the rounding into a helper that can be tested. On iOS, text measurement is rounded up to the device pixel grid. Converting the measured size from double to float and then rounding it to the pixel grid in Yoga can lose precision for a value that sits on a pixel boundary. That value can round down instead of up and clip the final line (height) or the trailing glyph (width). Fixes react#53450. The fix adds a small epsilon before ceil on both width and height. This matches the approach the legacy architecture has used since D7074168 (a534672). The rounding now lives in a helper, internal_roundTextMeasurementToPixelGrid, so it can be covered by unit tests. The helper keeps the internal_ prefix so it stays out of the public C++ API snapshot. On the earlier revert: D85438723 was backed out after wildeMarketplaceScreenshot-e2e.js failed. Running that test on an OnDemand shows what actually happens. It passes on trunk. With this change the ad renders one physical pixel taller (height 44px to 45px, width unchanged). The top 44 rows are byte-for-byte identical to the baseline and the extra row is blank, so the content renders correctly and the failure is only the pixel-exact size check rejecting a 1px-different image. This is expected for the epsilon and is resolved by re-capturing the baseline, which is what D7074168 did in its own commit. The test is also independently flaky (test infra has flagged it, and it renders autoplaying video and carousels), which is a separate reason its earlier auto-rebaseline looked wrong. Rendering verification (wildeMarketplaceScreenshot, config browse_dynamic_image_layer_frame_overlay_high_res_sr_video), captured on an OnDemand. This is the element-scoped capture the test compares (the mp_feed_ad_item ad cell), not a full screen. The two are identical except for one blank pixel row at the bottom. | Before (trunk, 274x44) | After (this change, 274x45) | | --- | --- | | {F1993025868} | {F1993025869} | Changelog: [iOS] [Fixed] - Prevent the final line or trailing glyph of text from being clipped due to pixel-grid rounding precision loss (react#53450) Reviewed By: cipolleschi Differential Revision: D113691958
|
@fabriziocucci has exported this pull request. If you are a Meta employee, you can view the originating Diff in D113691958. |
|
This pull request has been merged in 5e40a17. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Re-lands the iOS text measurement rounding fix from PR #54260 / D85438723, which was reverted by D86791267. This version adds unit test coverage and moves the rounding into a helper that can be tested.
On iOS, text measurement is rounded up to the device pixel grid. Converting the measured size from double to float and then rounding it to the pixel grid in Yoga can lose precision for a value that sits on a pixel boundary. That value can round down instead of up and clip the final line (height) or the trailing glyph (width). Fixes #53450.
The fix adds a small epsilon before ceil on both width and height. This matches the approach the legacy architecture has used since D7074168 (a534672). The rounding now lives in a helper, internal_roundTextMeasurementToPixelGrid, so it can be covered by unit tests. The helper keeps the internal_ prefix so it stays out of the public C++ API snapshot.
On the earlier revert: D85438723 was backed out after wildeMarketplaceScreenshot-e2e.js failed. Running that test on an OnDemand shows what actually happens. It passes on trunk. With this change the ad renders one physical pixel taller (height 44px to 45px, width unchanged). The top 44 rows are byte-for-byte identical to the baseline and the extra row is blank, so the content renders correctly and the failure is only the pixel-exact size check rejecting a 1px-different image. This is expected for the epsilon and is resolved by re-capturing the baseline, which is what D7074168 did in its own commit. The test is also independently flaky (test infra has flagged it, and it renders autoplaying video and carousels), which is a separate reason its earlier auto-rebaseline looked wrong.
Rendering verification (wildeMarketplaceScreenshot, config browse_dynamic_image_layer_frame_overlay_high_res_sr_video), captured on an OnDemand. This is the element-scoped capture the test compares (the mp_feed_ad_item ad cell), not a full screen. The two are identical except for one blank pixel row at the bottom.
Changelog: [iOS] [Fixed] - Prevent the final line or trailing glyph of text from being clipped due to pixel-grid rounding precision loss (#53450)
Reviewed By: cipolleschi
Differential Revision: D113691958