fix: chromium area path render bug#1067
Conversation
|
neat bug, can you pls. link the chromium issue? If you ran into it and triaged yourself, and there are no obvious finds in https://bugs.chromium.org/p/chromium/issues/list maybe it's worth adding an issue for this, eg. with a super minimal codepen repro |
Codecov Report
@@ Coverage Diff @@
## master #1067 +/- ##
==========================================
+ Coverage 72.45% 72.90% +0.44%
==========================================
Files 366 382 +16
Lines 11365 11679 +314
Branches 2475 2524 +49
==========================================
+ Hits 8235 8515 +280
- Misses 3115 3141 +26
- Partials 15 23 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Yup will do @monfera thanks for the link. |
|
causes a bunch of very small pixel changes in vrt |
There was a problem hiding this comment.
Asking for a change of logic, so we check for the actual Chrome bug trigger condition, which is likely some pixel (or pixel in relation to shape/chart width) threshold, and is not safely inferrable from what the domain datum is, before we map it to pixel values (eg. a scale can diminish or magnify differences). Also, the error is not just triggered if the pixel difference is zero; a too small difference triggers it too.
The decimal truncation of the X pixels feels unrelated to the fix, worth separating, for the case when Google fixes the error, easier to make the reverse PR, maybe just a git revert.
There was a problem hiding this comment.
Thanks for the changes, all of them look good 👍
One more find: these mocks lost their zero lines due to the shift. Worth jittering in the other direction, it's less likely to happen on the top (would be good to check though, there are already test cases which have a straight line on the top, from the time Cartesians clipped the topmost points and lines in half, which got fixed ~2yrs ago - but they may not trigger the new offset logic)
and the next ~8 files. There's also impact somewhere else, a thicker line on the floor got thinned, same reason
|
Good catch @monfera. I thought the reference point was the other way. Flipped the add to subtract. (aka ➕ ➡️ ➖) |
monfera
left a comment
There was a problem hiding this comment.
Looks good to me, thanks! I sampled several dozen images via the slider, around 20% of all, and scan-compared the rest. I think it's OK due to the type of change, previous reviews and the fact that so many of them are very very similar (looking at you, percentage based area charts).
I didn't see any showstopper in this PR. The 0.5 values and the mentioned constants could perhaps be DRYed up in this PR or a follow-up one.
There are a couple of places when the appearance of multiple zero-prone lines changed, though one is not superior to the other, eg.: integration/tests/__image_snapshots__/line-stories-test-ts-line-series-stories-line-paths-for-ordered-values-should-render-correct-line-path-stacked-1-snap.png
🎉 Also, the small upward shift visually fixes the frequent case of 1px width zero-prone lines, which used to be chopped in half, eg. more seamless, uniform thickness line here:
| @@ -84,6 +84,7 @@ interface ReactiveChartDispatchProps { | |||
| interface ReactiveChartOwnProps { | |||
| forwardStageRef: RefObject<HTMLCanvasElement>; | |||
| } | |||
| const CLIPPING_MARGINS = 0.5; | |||
There was a problem hiding this comment.
Can/should it be shared with CHROME_PINCH_BUG_EPSILON or be one defined by the other?
There was a problem hiding this comment.
... on a 2nd thought, it feels like, unless I misconstrue the nature of clipping margins, two constants altogether would suffice for all these and the below naked 0.5s:
CHROME_PINCH_BUG_EPSILON(for testing, as you already do)CHROME_PINCH_BUG_WORKAROUND_OFFSET(for offsetting)
and maybe not worth making one depend on the other, though it'd be nice if they were right next to one another, with the comment about the chromium bug link and short explanation atop of it. Still in the nit category, just lets us centralize the workaround and its eventual hopeful reversal of this bad bug
There was a problem hiding this comment.
@monfera I think the idea here is to be generic to expand this later to prevent clipping of lines, points, areas, etc outside of the clipped range. In this case it should be the value of SMALL_PIXEL so maybe something like this would suffice.
| const CLIPPING_MARGINS = 0.5; | |
| const CLIPPING_MARGINS = CHROME_PINCH_BUG_EPSILON; |
But for the purposes of backporting this, I think this can wait for another time.
| */ | ||
| function chromeRenderBugBuffer(y1: number, y0: number): number { | ||
| const diff = Math.abs(y1 - y0); | ||
| return diff <= CHROME_PINCH_BUG_EPSILON ? 0.5 : 0; |
There was a problem hiding this comment.
Minor: wondering if the 0.5 here should be extracted into a constant
| }); | ||
| } else { | ||
| if (length > 0) { | ||
| context.rect(0, 0, clippedRanges[0][0], height); | ||
| context.rect(0, -0.5, clippedRanges[0][0], height); |
There was a problem hiding this comment.
Same here, maybe worth linking these 0.5s to a common constant
nickofthyme
left a comment
There was a problem hiding this comment.
LGTM after seemingly millions of screenshot reviews 😭
# Conflicts: # integration/tests/__image_snapshots__/all-test-ts-baseline-visual-tests-for-all-stories-area-chart-stacked-percentage-visually-looks-correct-1-snap.png # integration/tests/__image_snapshots__/all-test-ts-baseline-visual-tests-for-all-stories-scales-log-scale-options-visually-looks-correct-1-snap.png # integration/tests/__image_snapshots__/all-test-ts-baseline-visual-tests-for-all-stories-stylings-custom-series-styles-lines-visually-looks-correct-1-snap.png # integration/tests/__image_snapshots__/area-stories-test-ts-area-series-stories-negative-log-areas-shows-only-positive-domain-mixed-polarity-domain-with-limit-1-snap.png # integration/tests/__image_snapshots__/area-stories-test-ts-area-series-stories-stacked-as-not-percentage-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-hover-over-specific-bars-rotation-0-shows-tooltip-on-last-bar-group-top-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-chart-rotation-0-placement-bottom-shows-tooltip-in-top-left-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-chart-rotation-0-placement-bottom-shows-tooltip-in-top-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-chart-rotation-0-placement-top-shows-tooltip-in-bottom-left-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-chart-rotation-0-placement-top-shows-tooltip-in-bottom-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-chart-rotation-180-placement-bottom-shows-tooltip-in-top-left-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-chart-rotation-180-placement-bottom-shows-tooltip-in-top-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-chart-rotation-180-placement-top-shows-tooltip-in-bottom-left-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-chart-rotation-180-placement-top-shows-tooltip-in-bottom-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-0-placement-bottom-shows-tooltip-in-bottom-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-0-placement-bottom-shows-tooltip-in-top-left-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-0-placement-bottom-shows-tooltip-in-top-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-0-placement-left-shows-tooltip-in-bottom-left-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-0-placement-left-shows-tooltip-in-top-left-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-0-placement-right-shows-tooltip-in-bottom-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-0-placement-right-shows-tooltip-in-top-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-0-placement-top-shows-tooltip-in-bottom-left-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-0-placement-top-shows-tooltip-in-bottom-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-0-placement-top-shows-tooltip-in-top-left-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-0-placement-top-shows-tooltip-in-top-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-180-placement-bottom-shows-tooltip-in-bottom-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-180-placement-bottom-shows-tooltip-in-top-left-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-180-placement-bottom-shows-tooltip-in-top-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-180-placement-left-shows-tooltip-in-bottom-left-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-180-placement-left-shows-tooltip-in-top-left-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-180-placement-right-shows-tooltip-in-bottom-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-180-placement-right-shows-tooltip-in-top-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-180-placement-top-shows-tooltip-in-bottom-left-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-180-placement-top-shows-tooltip-in-bottom-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-180-placement-top-shows-tooltip-in-top-left-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-180-placement-top-shows-tooltip-in-top-right-corner-1-snap.png # integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-should-render-custom-tooltip-1-snap.png # integration/tests/__image_snapshots__/legend-stories-test-ts-legend-stories-should-render-legend-action-on-mouse-hover-1-snap.png # integration/tests/__image_snapshots__/scales-stories-test-ts-scales-stories-negative-values-should-render-proper-ticks-with-binary-base-1-snap.png # integration/tests/__image_snapshots__/scales-stories-test-ts-scales-stories-negative-values-should-render-proper-ticks-with-common-base-1-snap.png # integration/tests/__image_snapshots__/scales-stories-test-ts-scales-stories-negative-values-should-render-proper-ticks-with-natural-base-1-snap.png # integration/tests/__image_snapshots__/scales-stories-test-ts-scales-stories-negative-values-should-render-values-with-log-limit-1-snap.png # integration/tests/__image_snapshots__/scales-stories-test-ts-scales-stories-negative-values-should-render-with-baseline-set-to-1-if-fit-is-false-1-snap.png # integration/tests/__image_snapshots__/scales-stories-test-ts-scales-stories-negative-values-should-render-with-baseline-set-to-1-if-fit-is-false-and-limit-is-set-1-snap.png # integration/tests/__image_snapshots__/scales-stories-test-ts-scales-stories-positive-values-should-render-proper-ticks-with-binary-base-1-snap.png # integration/tests/__image_snapshots__/scales-stories-test-ts-scales-stories-positive-values-should-render-proper-ticks-with-common-base-1-snap.png # integration/tests/__image_snapshots__/scales-stories-test-ts-scales-stories-positive-values-should-render-proper-ticks-with-natural-base-1-snap.png # integration/tests/__image_snapshots__/scales-stories-test-ts-scales-stories-positive-values-should-render-values-with-log-limit-1-snap.png # integration/tests/__image_snapshots__/scales-stories-test-ts-scales-stories-positive-values-should-render-with-baseline-set-to-1-if-fit-is-false-1-snap.png # integration/tests/__image_snapshots__/scales-stories-test-ts-scales-stories-positive-values-should-render-with-baseline-set-to-1-if-fit-is-false-and-limit-is-set-1-snap.png
# [25.4.0](v25.3.0...v25.4.0) (2021-03-23) ### Bug Fixes * chromium area path render bug ([#1067](#1067)) ([e16d15d](e16d15d)) ### Features * **tooltip:** expose datum in the TooltipValue ([#1082](#1082)) ([0246784](0246784)), closes [#1042](#1042) * **wordcloud:** wordcloud ([#1038](#1038)) ([f08f4c9](f08f4c9))
|
🎉 This PR is included in version 25.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [25.4.0](elastic/elastic-charts@v25.3.0...v25.4.0) (2021-03-23) ### Bug Fixes * chromium area path render bug ([opensearch-project#1067](elastic/elastic-charts#1067)) ([37301bf](elastic/elastic-charts@37301bf)) ### Features * **tooltip:** expose datum in the TooltipValue ([opensearch-project#1082](elastic/elastic-charts#1082)) ([48dc9d5](elastic/elastic-charts@48dc9d5)), closes [opensearch-project#1042](elastic/elastic-charts#1042) * **wordcloud:** wordcloud ([opensearch-project#1038](elastic/elastic-charts#1038)) ([d724cad](elastic/elastic-charts@d724cad))
|
This bug appears to have been fixed and we can now revert this workaround. See https://bugs.chromium.org/p/chromium/issues/detail?id=1163912#c6 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1067 +/- ##
==========================================
+ Coverage 72.45% 73.20% +0.74%
==========================================
Files 366 383 +17
Lines 11365 11797 +432
Branches 2475 2555 +80
==========================================
+ Hits 8235 8636 +401
- Misses 3115 3138 +23
- Partials 15 23 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
fixes #1053
Related to https://bugs.chromium.org/p/chromium/issues/detail?id=1163912
There is an issue with chromium where the area line can be pinched causing the filled area to not be rendered.
Before
After
The temporary fix is to add a small imperceptible pixel value to the scaled
y1value when the unscaledy1andy0values are equal.Additionally, this PR adds rounding to the scaled values to avoid unnecessarily precise scaled values in the path strings.
Checklist