fix: Broken effect in useCSSTextTruncation hook #22324
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
As discovered as part of #22317, the
useCSSTextTruncation
hook had a difficult to track down bug: the effect responsible for updating theisTruncated
value based on DOM measurements wouldn't run, even though console logs showed that values in its dependency array were changing.Upon investigation, it seems this is an intended behavior of React, as described by this comment: even if dependency arrays change, React will abort the entire render cycle (including effects) if neither props, state nor context have changed. Because the dependency array of the effect in question is currently based on values derived from refs, changes to these values don't indicate to React that the render cycle is "worth it", so if no other props, state or context changes in the component calling the hook, the render cycle is thrown out and the effect that would update the
isTruncated
value never runs.To fix this, this PR removes the use of refs to store intermediate values and instead adds a new effect that runs every render and sets the
offsetWidth
andscrollWidth
state variables to whatever the current DOM measurement is. No dependency array or checks for changes are necessary to avoid infinite render cycles because updating state in functional components to the same value does not trigger a re-render. This PR then updates the previously-existing effect to be much simpler, taking advantage of this same no-op behavior of setting the state to an existing value.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Example that's broken in #22317:
Screen.Recording.2022-12-02.at.8.24.32.PM.mov
#22317 after cherry-picking this commit:
Screen.Recording.2022-12-02.at.8.25.21.PM.mov
TESTING INSTRUCTIONS
Check that truncation-based tooltips still work as expected wherever
useCSSTextTruncation
is used:DateFilterLabel
in Dashboard native filters (vertical + horizontal) and Explore ad-hoc filtersADDITIONAL INFORMATION
HORIZONTAL_FILTER_BAR