Skip to content

[Image] | (DX) | Recheck height in Graphie to stop font position flakiness #3506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nishasy merged 10 commits into from
Apr 29, 2026
Merged

[Image] | (DX) | Recheck height in Graphie to stop font position flakiness #3506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d77f5c7
[image-flakey-test] [Image] | (DX) | Add delay to flakey Graphie imag…
nishasy Apr 16, 2026
48211c6
[image-flakey-test] docs(changeset):
nishasy Apr 16, 2026
896a3fc
[image-flakey-test] docs(changeset): [Image] | (DX) | Add delay to fl…
nishasy Apr 16, 2026
ae13265
[image-flakey-test] Trying something else
nishasy Apr 16, 2026
b76b3fa
[image-flakey-test] trying another thing
nishasy Apr 16, 2026
afee1c3
[image-flakey-test] Merge branch 'main' into image-flakey-test
nishasy Apr 28, 2026
71a8085
[image-flakey-test] reset to main
nishasy Apr 28, 2026
e6bf0c0
[image-flakey-test] Try syncing the original size
nishasy Apr 28, 2026
9ae6617
[image-flakey-test] replace the fragile currentHeightMatchesProps app…
nishasy Apr 28, 2026
99b3efb
[image-flakey-test] update comment to be more readable
nishasy Apr 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fifty-stingrays-provide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

[Image] | (DX) | Add delay to flakey Graphie image regression test
28 changes: 15 additions & 13 deletions packages/perseus/src/util/graphie.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1708,12 +1708,6 @@ const setLabelMargins = function (span: HTMLElement, size: Coord): void {
const direction = $span.data("labelDirection");
let [width, height] = size;

// Store the original (pre-scaling) label size on first call so that
// we can recalculate margins correctly when the container resizes.
if ($span.data("originalLabelSize") == null) {
$span.data("originalLabelSize", [width, height]);
}

// This can happen when a span
// is invisible but we still want to update the CSS. At worst, we will
// be off by a few pixels instead of in a different position entirely.
Expand All @@ -1736,9 +1730,12 @@ const setLabelMargins = function (span: HTMLElement, size: Coord): void {
marginLeft: -width / 2 + x * scale,
marginTop: -height / 2 - y * scale,
});
// Store the original (pre-scaling) label size on first call so that
// we can recalculate margins correctly when the container resizes.
if ($span.data("originalLabelSize") == null) {
$span.data("originalLabelSize", [width, height]);
}
} else {
const currentHeightMatchesProps = span.scrollHeight === height;

// We are using jQuery to collect information and calculate a scale
// since we don't have a way to pass it to this function.
// We need the width of the container in order to calculate the scale to apply to the label.
Expand All @@ -1760,12 +1757,17 @@ const setLabelMargins = function (span: HTMLElement, size: Coord): void {
// Inherited line-height values can really mess up placement.
$container.css("line-height", "normal");

// If the change in line-height affected the height of the element,
// then the height used for calculations should be updated.
// This can happen when the first label in the container calls this method,
// and the line-height was different when the height measurement was originally referenced.
if (currentHeightMatchesProps && span.scrollHeight !== height) {
Copy link
Copy Markdown
Contributor

@mark-fitzgerald mark-fitzgerald Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble seeing how this situation (line-height affecting the height) is handled by the new conditional involving just the "originnalLabelSize" data attribute, below. Can you verify the timing of this?

Copy link
Copy Markdown
Contributor Author

@nishasy nishasy Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like in the main code, the height gets set to the scroll height only if a change in the scroll height is detected. In the new code, the height gets set to the scroll height always on the first run. The timing on the two should be the same.

// The first time this runs for a label, we re-measure the label's height from
// the DOM instead of trusting the `height` value passed into this function.
// We do this because setting line-height above can shift the element's height,
// and the passed in `height` may have been captured before that shift happened
// (either because we just caused it, or because a sibling label caused it earlier).
// We store the corrected height so that on later calls (triggered by ResizeObserver
// when the container resizes), we can skip re-measuring and just scale from the
// original stored size.
if ($span.data("originalLabelSize") == null) {
height = span.scrollHeight;
$span.data("originalLabelSize", [width, height]);
}

// The expected width of the graphie is found in the "max-width" property on ".svg-image" containers,
Expand Down
Loading