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

perf: remove redundant overflow style manipulation #2678

Merged
merged 1 commit into from
Dec 29, 2024

Conversation

talkor
Copy link
Member

@talkor talkor commented Dec 26, 2024

@talkor talkor requested a review from a team as a code owner December 26, 2024 07:09
Comment on lines -8 to -9
const curOverflow = element.style.overflow;
if (!curOverflow || curOverflow === "visible") element.style.overflow = "hidden";
Copy link
Member Author

Choose a reason for hiding this comment

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

This probably intended for more accurate calculation but it triggers a redraw which makes this function heavy

Copy link
Contributor

Choose a reason for hiding this comment

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

when you say more accurate, what are we "losing" here? only legacy browsers?

Copy link
Member Author

@talkor talkor Dec 29, 2024

Choose a reason for hiding this comment

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

In modern browsers, elements can have overflow styles like visible, hidden, auto, or scroll, which control how content is displayed when it exceeds the container's dimensions. Historically (2017<), and in some edge cases even today, browsers have had quirks in how they calculate scrollWidth and scrollHeight when overflow is set to visible:

  1. overflow: visible Behavior:
    • When overflow is visible, the browser does not clip or restrict content, so scrollWidth and scrollHeight might not accurately represent the "overflowing" content dimensions.
    • Some older browsers would inconsistently calculate the scrollHeight or scrollWidth, leading to unreliable results. For example:
      • If the content extended outside the container, scrollHeight might only reflect the visible portion of the element.
  2. Dynamic Overflow Calculations:
    • Some browsers historically recalculated layout properties like scrollHeight and scrollWidth lazily or only when overflow was set to hidden, auto, or scroll. As a result, developers often had to temporarily set overflow to force the browser to update these values.
  3. Rendering Engine Differences:
    • Older rendering engines (e.g., Trident in Internet Explorer or older versions of WebKit) had subtle bugs in handling overflow. Developers would use workarounds like temporarily changing the overflow property to get consistent values.

The approach of temporarily setting overflow: hidden is a deliberate attempt to work around these historical inconsistencies. Now, it assumes that these historical quirks do not apply or that the overflow property is already managed correctly. In modern browsers, the need for such workarounds has significantly decreased. The major benefit is the dramatic performance improvement

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough

@talkor talkor changed the title perf: remove redundant overflow style manipulation [prerelease] perf: remove redundant overflow style manipulation Dec 26, 2024
Copy link
Contributor

A new prerelease version of this PR has been published! 🎉
To install this prerelease version, run the following command in your terminal with any one of the packages changed in this PR:

To update @vibe/core:

yarn add @vibe/[email protected]

Or with npm:

npm i @vibe/[email protected]

@talkor talkor merged commit 5b57172 into master Dec 29, 2024
14 checks passed
@talkor talkor deleted the perf/text-overflowing-improvements branch December 29, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants