fix: chromium area path render bug #1067

nickofthyme · 2021-03-09T19:03:21Z

Summary

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 y1 value when the unscaled y1 and y0 values are equal.

Additionally, this PR adds rounding to the scaled values to avoid unnecessarily precise scaled values in the path strings.

Checklist

This was checked for cross-browser compatibility
Proper documentation or storybook story was added for features that require explanation or tutorials
Unit tests were updated or added to match the most common scenarios

monfera · 2021-03-09T19:10:51Z

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-io · 2021-03-09T19:10:53Z

Codecov Report

Merging #1067 (f493248) into master (25a247e) will increase coverage by 0.44%.
The diff coverage is 100.00%.

@@            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

Flag	Coverage Δ
unittests	`72.50% <100.00%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/chart_types/xy_chart/rendering/utils.ts	`97.93% <100.00%> (+3.49%)`	⬆️
src/mocks/scale/index.ts	`100.00% <0.00%> (ø)`
src/mocks/scale/scale.ts	`77.77% <0.00%> (ø)`
src/mocks/series/data.ts	`100.00% <0.00%> (ø)`
src/mocks/specs/index.ts	`100.00% <0.00%> (ø)`
src/mocks/store/index.ts	`100.00% <0.00%> (ø)`
src/mocks/specs/specs.ts	`81.81% <0.00%> (ø)`
src/mocks/series/series.ts	`91.22% <0.00%> (ø)`
src/mocks/index.ts	`100.00% <0.00%> (ø)`
src/mocks/theme.ts	`86.66% <0.00%> (ø)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25a247e...f493248. Read the comment docs.

nickofthyme · 2021-03-09T19:27:50Z

Yup will do @monfera thanks for the link.

nickofthyme · 2021-03-09T21:25:24Z

causes a bunch of very small pixel changes in vrt

monfera

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.

src/chart_types/xy_chart/rendering/utils.ts

monfera

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

nickofthyme · 2021-03-11T23:37:12Z

Good catch @monfera. I thought the reference point was the other way. Flipped the add to subtract. (aka ➕ ➡️ ➖)

monfera

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:

monfera · 2021-03-15T21:52:33Z

src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx

@@ -84,6 +84,7 @@ interface ReactiveChartDispatchProps {
 interface ReactiveChartOwnProps {
  forwardStageRef: RefObject<HTMLCanvasElement>;
 }
+const CLIPPING_MARGINS = 0.5;


Can/should it be shared with CHROME_PINCH_BUG_EPSILON or be one defined by the other?

... 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

@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.

Suggested change

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.

monfera · 2021-03-15T21:54:17Z

src/chart_types/xy_chart/rendering/utils.ts

+ */
+function chromeRenderBugBuffer(y1: number, y0: number): number {
+  const diff = Math.abs(y1 - y0);
+  return diff <= CHROME_PINCH_BUG_EPSILON ? 0.5 : 0;


Minor: wondering if the 0.5 here should be extracted into a constant

monfera · 2021-03-15T21:54:50Z

src/renderers/canvas/index.ts

      });
    } else {
      if (length > 0) {
-        context.rect(0, 0, clippedRanges[0][0], height);
+        context.rect(0, -0.5, clippedRanges[0][0], height);


Same here, maybe worth linking these 0.5s to a common constant

nickofthyme

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.0.5](v25.0.4...v25.0.5) (2021-03-16) ### Bug Fixes * chromium area path render bug ([#1067](#1067)) ([#1080](#1080)) ([ba4195b](ba4195b))

# [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))

nickofthyme · 2021-03-23T14:40:06Z

🎉 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))

nickofthyme · 2023-12-13T19:33:56Z

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

fix: chromium area path render bug

630d31b

nickofthyme added bug Something isn't working :xy Bar/Line/Area chart related labels Mar 9, 2021

nickofthyme requested review from markov00, monfera and rshen91 March 9, 2021 19:03

chore: add chromium bug link

5cff751

nickofthyme mentioned this pull request Mar 9, 2021

[Lens] Fix area percentage gaps with zero fitting function elastic/kibana#94086

Merged

9 tasks

nickofthyme added 2 commits March 9, 2021 14:52

Merge branch 'master' into chrome-path-bug

8161be5

chore: update vrt screenshots

15d67b1

monfera suggested changes Mar 10, 2021

View reviewed changes

nickofthyme added 6 commits March 10, 2021 14:24

chore: revert rounding changes

fa6b018

chore: revert all image snapshot changes

2bda1ac

refactor: shift px render based on px diff

e9364c5

test: fix unit tests for new extra value

65ad03a

chore: remove unneeded change

f493248

chore: update vrt screenshots

12c4b87

monfera self-requested a review March 11, 2021 11:47

monfera suggested changes Mar 11, 2021

View reviewed changes

fix: flip buffer to top

985c7ec

nickofthyme and others added 4 commits March 11, 2021 17:38

fix: unit tests

ed75f33

fix: fix rendering offset

12c347a

fix: add margin around clipped areas

78d0918

test: update vrts

7430337

monfera self-requested a review March 15, 2021 21:44

monfera approved these changes Mar 15, 2021

View reviewed changes

nickofthyme commented Mar 15, 2021

View reviewed changes

nickofthyme merged commit e16d15d into elastic:master Mar 16, 2021

nickofthyme deleted the chrome-path-bug branch March 16, 2021 00:30

nickofthyme mentioned this pull request Mar 16, 2021

fix: chromium area path render bug (#1067) [25.0.x] #1080

Merged

markov00 pushed a commit that referenced this pull request Mar 16, 2021

fix: chromium area path render bug (#1067) (#1080)

ba4195b

github-actions bot pushed a commit that referenced this pull request Mar 16, 2021

chore(release): 25.0.5 [skip ci]

b2a69f2

## [25.0.5](v25.0.4...v25.0.5) (2021-03-16) ### Bug Fixes * chromium area path render bug ([#1067](#1067)) ([#1080](#1080)) ([ba4195b](ba4195b))

markov00 mentioned this pull request Mar 16, 2021

[7.12] Fixes chromium bug on rendering charts elastic/kibana#94698

Merged

nickofthyme added the released Issue released publicly label Mar 23, 2021

nickofthyme mentioned this pull request Dec 13, 2023

Revert Chrome bug workaround #2279

Closed

nickofthyme added a commit to nickofthyme/elastic-charts that referenced this pull request Apr 26, 2024

chore: remove chrome bug logic from elastic#1067

66d29b4

nickofthyme mentioned this pull request Apr 26, 2024

chore: remove chrome bug logic from #1067 #2417

Merged

3 tasks

nickofthyme added a commit that referenced this pull request Apr 26, 2024

chore: remove chrome bug logic from #1067 (#2417)

8acaf26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: chromium area path render bug #1067

fix: chromium area path render bug #1067

nickofthyme commented Mar 9, 2021 •

edited

Loading

monfera commented Mar 9, 2021

codecov-io commented Mar 9, 2021 •

edited

Loading

nickofthyme commented Mar 9, 2021

nickofthyme commented Mar 9, 2021

monfera left a comment •

edited

Loading

monfera left a comment •

edited

Loading

nickofthyme commented Mar 11, 2021

monfera left a comment

monfera Mar 15, 2021

monfera Mar 15, 2021 •

edited

Loading

nickofthyme Mar 15, 2021 •

edited

Loading

monfera Mar 15, 2021

monfera Mar 15, 2021

nickofthyme left a comment

nickofthyme commented Mar 23, 2021

nickofthyme commented Dec 13, 2023 •

edited

Loading

	const CLIPPING_MARGINS = 0.5;
	const CLIPPING_MARGINS = CHROME_PINCH_BUG_EPSILON;

fix: chromium area path render bug #1067

fix: chromium area path render bug #1067

Conversation

nickofthyme commented Mar 9, 2021 • edited Loading

Summary

Before

After

Checklist

monfera commented Mar 9, 2021

codecov-io commented Mar 9, 2021 • edited Loading

Codecov Report

nickofthyme commented Mar 9, 2021

nickofthyme commented Mar 9, 2021

monfera left a comment • edited Loading

Choose a reason for hiding this comment

monfera left a comment • edited Loading

Choose a reason for hiding this comment

nickofthyme commented Mar 11, 2021

monfera left a comment

Choose a reason for hiding this comment

monfera Mar 15, 2021

Choose a reason for hiding this comment

monfera Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

nickofthyme Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

monfera Mar 15, 2021

Choose a reason for hiding this comment

monfera Mar 15, 2021

Choose a reason for hiding this comment

nickofthyme left a comment

Choose a reason for hiding this comment

nickofthyme commented Mar 23, 2021

nickofthyme commented Dec 13, 2023 • edited Loading

nickofthyme commented Mar 9, 2021 •

edited

Loading

codecov-io commented Mar 9, 2021 •

edited

Loading

monfera left a comment •

edited

Loading

monfera left a comment •

edited

Loading

monfera Mar 15, 2021 •

edited

Loading

nickofthyme Mar 15, 2021 •

edited

Loading

nickofthyme commented Dec 13, 2023 •

edited

Loading