Skip to content
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.

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

Infinite loop in Truncate #1797

Closed
arbrandes opened this issue Nov 25, 2022 · 2 comments · Fixed by #1702
Closed

Infinite loop in Truncate #1797

arbrandes opened this issue Nov 25, 2022 · 2 comments · Fixed by #1702

Comments

@arbrandes
Copy link
Contributor

As of this revision, Truncate assumes that the scrollHeight of an unpadded element containing only text will be an exact multiple of its line-height property:

const visibilityArea = LINE_HEIGHT_VALUE * Number(lines);
let newElementTextHeight = newElement.scrollHeight;
while (newElementTextHeight > visibilityArea) {
   //... code to truncate text
  newElementTextHeight = newElement.scrollHeight;
}

This assumption is incorrect. Independently of a parent element's line-height, if a child inline element is vertically aligned to baseline (the default), it is possible for the parent's scrollHeight to be greater than the sum of the heights of all lines it contains. This is because the browser will pad each line's box vertically with enough space for a font's descenders with a value that varies according to font, browser zoom level, etc.

In particular, consider this usage of Truncate, where a single line is desired across three inner <span />s. No matter what value of line-height is set for the parent, it was observed in at least two browser implementations (Firefox 107.0 (64-bit) and Chromium 107.0.5304.121 (64-bit), both on Ubuntu Linux 22.04.1) that when finally truncated to one line, visibilityArea would always be one pixel larger than newElement.scrollHeight, leading to an infinite loop.

A workaround, as suggested here, is to vertical-align: bottom all child <span />s so they conform to the parent's line-height. In practice, this successfully avoids the infinite loop because when its down to a single line, newElementTextHeight will finally be equal to the expected visibilityArea.

Truncate should be smart enough to avoid this without any expectations regarding the vertical alignment of child elements.

(Nowadays browsers actually support CSS-only line clamping, so Truncate might optionally use that. Clamp.js, for example, checks for support and uses it, if enabled.)

@arbrandes
Copy link
Contributor Author

@viktorrusakov, I see you'll soon start working on Truncate v3. Can we take this issue in consideration during its development?

arbrandes pushed a commit to arbrandes/frontend-app-discussions that referenced this issue Nov 25, 2022
This works around [a known issue](openedx/paragon#1797)
with `Truncate` that in some situations can lead to an infinite loop and
subsequent page hang.
arbrandes pushed a commit to arbrandes/frontend-app-discussions that referenced this issue Nov 28, 2022
This works around [a known issue](openedx/paragon#1797)
with `Truncate` that in some situations can lead to an infinite loop and
subsequent page hang.
arbrandes pushed a commit to arbrandes/frontend-app-discussions that referenced this issue Nov 28, 2022
This works around [a known issue](openedx/paragon#1797)
with `Truncate` that in some situations can lead to an infinite loop and
subsequent page hang.
arbrandes pushed a commit to openedx/frontend-app-discussions that referenced this issue Nov 28, 2022
This works around [a known issue](openedx/paragon#1797)
with `Truncate` that in some situations can lead to an infinite loop and
subsequent page hang.
@viktorrusakov
Copy link
Contributor

@arbrandes working on it, thanks for reporting the issue!

@viktorrusakov viktorrusakov linked a pull request Dec 9, 2022 that will close this issue
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants