Skip to content

Commit

Permalink
Fix inaccurate text size calculations (#2505)
Browse files Browse the repository at this point in the history 
* Remedy inaccurate tooltip size approximation

* Fix tests

* Fix multiline measurements

* Add changeset

* Update .changeset/pretty-moles-laugh.md

Co-authored-by: Ryan Roemer <ryan.roemer@formidable.com>

* Fix storybook webpack config on windows

* Memoize text measurement results

* Fix formatting

Co-authored-by: Ryan Roemer <ryan.roemer@formidable.com>
  • Loading branch information
@EthanRutherford @ryan-roemer
EthanRutherford and ryan-roemer committed Oct 28, 2022
1 parent 272649b commit 11906c4
Show file tree
Hide file tree
Showing 6 changed files with 164 additions and 44 deletions.
5 changes: 5 additions & 0 deletions .changeset/pretty-moles-laugh.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"victory-core": patch
---

Improve accuracy of text size measurements (fixes #2475)
2 changes: 1 addition & 1 deletion .storybook/main.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const STORIES = path.resolve(ROOT, "stories");
module.exports = {
webpackFinal: async (config) => {
// Read all the victory packages and alias.
glob.sync(path.join(PKGS, "victory*/package.json")).forEach((pkgPath) => {
glob.sync(path.join(PKGS, "victory*/package.json").replaceAll("\\", "/")).forEach((pkgPath) => {
const key = path.dirname(path.relative(PKGS, pkgPath));
config.resolve.alias[key] = path.resolve(path.dirname(pkgPath));
});
Expand Down
45 changes: 24 additions & 21 deletions packages/victory-core/src/victory-util/textsize.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { TextSize } from "victory-core";

const approximate = (text, style?) =>
TextSize._approximateTextSizeInternal.impl(text, style, true);

const testString = "ABC";

describe("victory-util/textsize", () => {
Expand All @@ -17,64 +20,64 @@ describe("victory-util/textsize", () => {

describe("approximateWidth", () => {
it("return zero width when no style", () => {
expect(TextSize.approximateTextSize(testString).width).toEqual(0);
expect(approximate(testString).width).toEqual(0);
});
it("return correct width with signed angle", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
angle: -45,
fontSize: 14,
}).width.toFixed(2),
).toEqual("31.71");
});
it("return correct width with pixel fontsize", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
fontSize: "14px",
}).width.toFixed(2),
).toEqual("28.74");
});
it("return appropriate width with defined fontSize", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
fontSize: 12,
}).width.toFixed(2),
).toEqual("24.64");
});
it("consider font", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
fontSize: 16,
}).width.toFixed(2),
).toEqual("32.85");
});
it("consider letterSpacing", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
fontSize: 12,
letterSpacing: "1px",
}).width.toFixed(2),
).toEqual("26.64");
});
it("consider angle", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
fontSize: 12,
angle: 30,
}).width.toFixed(2),
).toEqual("28.24");
});
it("not consider lineHeight without angle", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
fontSize: 12,
lineHeight: 2,
}).width.toFixed(2),
).toEqual("24.64");
});
it("consider lineHeight with angle", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
fontSize: 12,
lineHeight: 2,
angle: 30,
Expand All @@ -83,15 +86,15 @@ describe("victory-util/textsize", () => {
});
it("return width of widest string in text", () => {
expect(
TextSize.approximateTextSize("ABC\nDEFGH\nIJK", {
approximate("ABC\nDEFGH\nIJK", {
fontSize: 12,
}).width.toFixed(2),
).toEqual("41.94");
});

it("returns width of widest string in array if array has an empty string", () => {
expect(
TextSize.approximateTextSize(["06-14-20", ""], {
approximate(["06-14-20", ""], {
fontSize: 12,
}).width.toFixed(2),
).toEqual("47.93");
Expand All @@ -100,56 +103,56 @@ describe("victory-util/textsize", () => {

describe("approximateHeight", () => {
it("return zero width when no style", () => {
expect(TextSize.approximateTextSize(testString).height).toEqual(0);
expect(approximate(testString).height).toEqual(0);
});
it("return correct height with signed angle", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
angle: -45,
fontSize: 14,
}).height.toFixed(2),
).toEqual("33.29");
});
it("return correct height with pixel fontsize", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
fontSize: "14px",
}).height.toFixed(2),
).toEqual("16.90");
});
it("return appropriate height with expected precision", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
fontSize: 12,
}).height.toFixed(2),
).toEqual("14.49");
});
it("consider font", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
fontSize: 16,
}).height.toFixed(2),
).toEqual("19.32");
});
it("consider angle", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
fontSize: 12,
angle: 30,
}).height.toFixed(2),
).toEqual("25.48");
});
it("not consider letterSpacing without angle", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
fontSize: 12,
letterSpacing: "1px",
}).height.toFixed(2),
).toEqual("14.49");
});
it("consider letterSpacing with angle", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
fontSize: 12,
angle: 30,
letterSpacing: "1px",
Expand All @@ -158,15 +161,15 @@ describe("victory-util/textsize", () => {
});
it("consider lineHeight", () => {
expect(
TextSize.approximateTextSize(testString, {
approximate(testString, {
fontSize: 12,
lineHeight: 2,
}).height.toFixed(2),
).toEqual("28.98");
});
it("consider multiLines text", () => {
expect(
TextSize.approximateTextSize(`ABC\n${"DBCDEFG"}\n123`, {
approximate(`ABC\n${"DBCDEFG"}\n123`, {
fontSize: 12,
}).height.toFixed(2),
).toEqual("43.47");
Expand Down
125 changes: 107 additions & 18 deletions packages/victory-core/src/victory-util/textsize.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// http://www.pearsonified.com/2012/01/characters-per-line.php
/* eslint-disable no-magic-numbers */
import { assign, defaults } from "lodash";
import { assign, defaults, memoize } from "lodash";

// Based on measuring specific character widths
// as in the following example https://bl.ocks.org/tophtucker/62f93a4658387bb61e4510c37e2e97cf
Expand Down Expand Up @@ -243,9 +243,97 @@ const _approximateTextHeightInternal = (text: string | string[], style) => {
}, 0);
};

const _approximateDimensionsInternal = (
text: string | string[],
style?: TextSizeStyleInterface,
) => {
const angle = Array.isArray(style)
? style[0] && style[0].angle
: style && style.angle;
const height = _approximateTextHeightInternal(text, style);
const width = _approximateTextWidthInternal(text, style);
const widthWithRotate = angle
? _getSizeWithRotate(width, height, angle)
: width;
const heightWithRotate = angle
? _getSizeWithRotate(height, width, angle)
: height;
return {
width: widthWithRotate,
height: heightWithRotate * coefficients.heightOverlapCoef,
};
};

const _getMeasurementContainer = memoize(() => {
const element = document.createElementNS("http://www.w3.org/2000/svg", "svg");
element.setAttribute("xlink", "http://www.w3.org/1999/xlink");

const containerElement = document.createElementNS(
"http://www.w3.org/2000/svg",
"text",
);
element.appendChild(containerElement);

element.style.position = "fixed";
element.style.top = "-9999px";
element.style.left = "-9999px";

document.body.appendChild(element);

return containerElement;
});

const styleToKeyComponent = (style) => {
if (!style) {
return "null";
}

return `${style.angle}:${style.fontFamily}:${style.fontSize}:${style.letterSpacing}:${style.lineHeight}`;
};

const _measureDimensionsInternal = memoize(
(text: string | string[], style?: TextSizeStyleInterface) => {
const containerElement = _getMeasurementContainer();

const lines = _splitToLines(text);
let heightAcc = 0;
for (const [i, line] of lines.entries()) {
const textElement = document.createElementNS(
"http://www.w3.org/2000/svg",
"tspan",
);
const params = _prepareParams(style, i);
textElement.style.fontFamily = params.fontFamily;
textElement.style.transform = `rotate(${params.angle})`;
textElement.style.fontSize = `${params.fontSize}px`;
textElement.style.lineHeight = params.lineHeight;
textElement.style.fontFamily = params.fontFamily;
textElement.style.letterSpacing = params.letterSpacing;
textElement.textContent = line;
textElement.setAttribute("x", "0");
textElement.setAttribute("y", `${heightAcc}`);
heightAcc += params.lineHeight * params.fontSize;

containerElement.appendChild(textElement);
}

const { width, height } = containerElement.getBoundingClientRect();

containerElement.innerHTML = "";

return { width, height };
},
(text, style) => {
const totalText = Array.isArray(text) ? text.join() : text;
const totalStyle = Array.isArray(style)
? style.map(styleToKeyComponent).join()
: styleToKeyComponent(style);
return `${totalText}::${totalStyle}`;
},
);

export interface TextSizeStyleInterface {
angle?: number;
characterConstant?: string;
fontFamily?: string;
fontSize?: number | string;
letterSpacing?: string;
Expand All @@ -254,22 +342,23 @@ export interface TextSizeStyleInterface {

// Stubbable implementation.
export const _approximateTextSizeInternal = {
impl: (text: string | string[], style?: TextSizeStyleInterface) => {
const angle = Array.isArray(style)
? style[0] && style[0].angle
: style && style.angle;
const height = _approximateTextHeightInternal(text, style);
const width = _approximateTextWidthInternal(text, style);
const widthWithRotate = angle
? _getSizeWithRotate(width, height, angle)
: width;
const heightWithRotate = angle
? _getSizeWithRotate(height, width, angle)
: height;
return {
width: widthWithRotate,
height: heightWithRotate * coefficients.heightOverlapCoef,
};
impl: (
text: string | string[],
style?: TextSizeStyleInterface,
__debugForceApproximate = false,
) => {
// Attempt to first measure the element in DOM. If there is no DOM, fallback
// to the less accurate approximation algorithm.
const isClient =
typeof window !== "undefined" &&
typeof window.document !== "undefined" &&
typeof window.document.createElement !== "undefined";

if (!isClient || __debugForceApproximate) {
return _approximateDimensionsInternal(text, style);
}

return _measureDimensionsInternal(text, style);
},
};

Expand Down
7 changes: 3 additions & 4 deletions packages/victory-vendor/.babelrc.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,9 @@ module.exports = {
/^node_modules/,
"lib-vendor",
);
const relPathToPkg = path.relative(
path.dirname(currentFileVendor),
vendorPkg,
).replaceAll("\\", "/");
const relPathToPkg = path
.relative(path.dirname(currentFileVendor), vendorPkg)
.replaceAll("\\", "/");

return relPathToPkg;
}
Expand Down
24 changes: 24 additions & 0 deletions stories/victory-label.stories.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,30 @@ export const LineHeight = () => {
/>
}
/>
<VictoryScatter
{...defaultScatterProps}
labelComponent={
<VictoryLabel
lineHeight={[2, 1, 3]}
text={["测试汉字", "不在正常的 ASCII 范围内", "最后一行"]}
backgroundStyle={[{ stroke: "blue", fill: "none" }]}
/>
}
/>
<VictoryScatter
{...defaultScatterProps}
labelComponent={
<VictoryLabel
lineHeight={[2, 1, 3]}
text={[
"اختبار اللغات التي تُقرأ من اليمين إلى اليسار",
"مثل العربية",
"هناك أكثر من ذلك بكثير",
]}
backgroundStyle={[{ stroke: "blue", fill: "none" }]}
/>
}
/>
</div>
);
};
Expand Down

0 comments on commit 11906c4

Please sign in to comment.