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

fix(Ellipsis): Handle some post-update issues #6797

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hsjzhcq
Copy link

@hsjzhcq hsjzhcq commented Dec 13, 2024

关于第一个文本省略字数错误的问题
我分析了一下,其实是setWalkingIndexes设置的值最后被赋值导致

第一次调用正常计算,然后第二次由useResizeEffect触发,先setStatus然后setWalkingIndexes,进入useLayoutEffect计算PREPARE时,此时walkingIndexes的值还是上一次的值,然后进入MEASURE_WALKING,执行了一次diff就等于0,然后renderContent,这时setWalkingIndexes初始化设置的值生效了,因为state是STABLE_ELLIPSIS,然后初始化的值变成渲染了一半的文本,所以文本省略看起来算的有问题,实则被覆盖了。
image

不过这个问题也很奇怪,为什么setWalkingIndexes没有与setStatus一同生效? 而且只有在codesandbox链接里这个简单页面渲染下100%复现这个问题,其他正常业务场景和官网文档实例中写一个一样的代码都没这个问题,难绷。

现在做法是把setWalkingIndexes放在setStatus(MEASURE_WALKING)前面,这样就不会有这个问题了,而且其实只有MEASURE_WALKING时才需要walkingIndexes的值。

引申优化问题
这里其实还有一个重复计算的问题,当fullMeasureRef大于rowMeasureHeight时才会计算,但是这个fullMeasureRef永远都是渲染content所有内容的高度,导致issue中debug截屏看到的一直是59>40。所以即使文本超出被截断了,下一次进入判断的时候也会重新命中,然后计算截断。

之前组件的实现就没有这个问题,因为下一次判断时container的内容就是已经处理过的,然后在取值container.offsetHeight,就会<=maxHeight,从而跳过判断。

我根据之前的思路再结合现在的实现,在计算前先获取containerRef的高度,然后取最小值,这样就解决了截断情况下fullMeasureRef一直大于rowMeasureHeight的情况,正常的跳过了。
image

但是引发了一个新的问题,跳过后state变成了STABLE_NO_ELLIPSIS,然后又渲染了content,导致元素触发ResizeObserver重新计算,结果就是state一直在STABLE_ELLIPSIS与STABLE_NO_ELLIPSIS反复横跳死循环了。

目前能想到两个解法

  1. 在ellipsis中存储一个initResizeRef,让第一次useResizeEffect时不调用forceResize。避免组件初次渲染100%重复计算一次
  2. 这个组件实现调整一下,参考之前的实现存储计算结果,让fullMeasureRef<=rowMeasureHeight时,不是单纯的改成STABLE_NO_ELLIPSIS渲染content

Copy link
Contributor

github-actions bot commented Dec 13, 2024

Preview is ready

@hsjzhcq hsjzhcq changed the title fix(Ellipsis): Handle some post-update issues (#6792) fix(Ellipsis): Handle some post-update issues Dec 13, 2024
@@ -66,12 +60,18 @@ export default function useMeasure(
const fullMeasureHeight = fullMeasureRef.current?.offsetHeight || 0
const singleRowMeasureHeight =
singleRowMeasureRef.current?.offsetHeight || 0
const rowMeasureHeight = singleRowMeasureHeight * rows
const rowMeasureHeight = singleRowMeasureHeight * (rows + 0.5)
Copy link
Member

Choose a reason for hiding this comment

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

能直接 +0.5 而不改 setWalkingIndexes 不,理论上启动阶段就是应该和 startMeasure 同步的

Copy link
Author

Choose a reason for hiding this comment

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

理论上是这样同步更新值,但是实际运行时,在线demo运行的结果却是不符合预期的。

从代码执行层面来说,其实只有状态为MEASURE_WALKING时才需要walkingIndexes的值,所以在设置MEASURE_WALKING状态前去更新walkingIndexes应该也是合理的。且这样做也能解决了结果不符合预期的问题。

Copy link
Author

Choose a reason for hiding this comment

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

对于重复计算的问题,您觉得这个点目前是否需要优化?
例如参考旧组件实现中缓存计算结果的做法,这样重新进入溢出判断时,不会一直命中计算条件。如果需要优化,我可以在这个迭代中一起处理。

Copy link
Member

Choose a reason for hiding this comment

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

理论上是这样同步更新值,但是实际运行时,在线demo运行的结果却是不符合预期的。

那直接包一个 unstable_batchedUpdates 告知其同时启动就行了

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.70%. Comparing base (495be63) to head (b949787).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6797      +/-   ##
==========================================
- Coverage   92.72%   92.70%   -0.02%     
==========================================
  Files         335      335              
  Lines        7198     7198              
  Branches     1804     1803       -1     
==========================================
- Hits         6674     6673       -1     
- Misses        489      517      +28     
+ Partials       35        8      -27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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