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(useVirtualList): mutate style in react context #2034

Merged
merged 6 commits into from
Feb 27, 2023

Conversation

luo3house
Copy link
Contributor

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / documentation update
  • Demo update
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Enhancement feature
  • Internationalization
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

🔗 Related issue link

fix #2033

💡 Background and solution

农历兔年快乐。

当滚动需要视野内数据变更时,钩子直接修改了 dom 中 wrapper 样式,因为这个回调不属于 React 的上下文,这可能破坏 React 一致性,出现频闪。

这个 PR 让钩子把计算好的 wrapper 样式存起来,在 Update Effect 中设置 wrapper 样式。

📝 Changelog

Language Changelog
🇺🇸 English mutate wrapper style in react context
🇨🇳 Chinese 在 react 上下文设置 wrapper 样式

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

@CLAassistant
Copy link

CLAassistant commented Jan 24, 2023

CLA assistant check)
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement) before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ luo3house
❌ crazylxr
You have signed the CLA already but the status is still pending? Let us recheck) it.

@luo3house
Copy link
Contributor Author

related:
#1940
#2040

也可以像 #1940 那样把计算后的样式交给用户,只是这样会有 breaking change。react virtualized 就是这样做的。这个 PR 避免向用户代码暴露内部处理。
cc @crazylxr ready for review

@crazylxr
Copy link
Collaborator

确实能解决这个问题。我先合掉。

不过你说的 :

因为这个回调不属于 React 的上下文,这可能破坏 React 一致性,出现频闪。

有什么资料可以参考么?

@crazylxr crazylxr merged commit 29c9203 into alibaba:master Feb 27, 2023
@luo3house
Copy link
Contributor Author

抱歉我没有太了解 react dom 的工作机制,但是我发现在直接设置样式的 demo1 下,会有如下现象:

useEffect:
  wrapper marginTop = 0

手动滚动到临界点,接收预期 scroll 事件(1):
  此时 container scrollTop = 601
  钩子计算后设置 wrapper marginTop = 60
  
  宏任务结束后,接收非预期的 scroll 事件(2):
      此时 container scrollTop = 541
      此时 wrapper marginTop = 60
      钩子计算后设置 wrapper marginTop = 0
  
  宏任务结束后,接收非预期的 scroll 事件(3):
      此时 container scrollTop = 601
      此时 wrapper marginTop = 0
      钩子计算后设置 wrapper marginTop = 60

  重复接收非预期事件(2),(3)

对于这个现象,我无法解释这个非预期的事件的原因;

但是设置样式时一般场景是在渲染周期中把样式写在 ReactEL 让 React 调度,而这个钩子设置样式是脱离了 React 周期走了 EventListener 回调,我觉得这样可能会导致和 react dom 的操作撞车,所以修改了此处,推迟到 Effect 再设置。

e.g.

  1. 钩子设置了 wrapper style,结束
  2. demo1 渲染了 ReactEL,其中 wrapper (<div ref={wrapperRef} />) 没有 style 属性
  3. 上屏后的 wrapper 是否应该有 style?

@luo3house luo3house deleted the draft-2033-ii branch February 28, 2023 11:27
@kakachake
Copy link
Contributor

@luo3house 你好,我最近在学习这个hooks的源码,也遇到这个问题了, 对于直接在calculateRange中设置样式导致闪动的问题您现在知道原因了吗,我也有疑问,为什么会有非预期的scroll事件产生呢?

@luo3house
Copy link
Contributor Author

@luo3house 你好,我最近在学习这个hooks的源码,也遇到这个问题了, 对于直接在calculateRange中设置样式导致闪动的问题您现在知道原因了吗,我也有疑问,为什么会有非预期的scroll事件产生呢?

抱歉非常晚才回复,通过debugger我发现出现闪动的位置可能是0也可能是前一个虚拟视口的top,所以我猜想 reactdom 内部可能对 dom 做了缓存用作diff,此时脱离 react 生命周期直接修改 dom 及其属性可能会有非预期的问题产生,但是具体还是要看 reactdom 的工作流程。而类似虚拟滚动解决方案 react-vxxxxlist或者react-scxxxn 把计算后的属性交给用户渲染应更加对用户透明。钩子这样改有breaking change,所以先让操作跟生命周期看看情况。

@adolfheir
Copy link

adolfheir commented Jul 18, 2023

原因应该是这个 但是感觉这样解没解完全 我这边还是回出现。
还是改源码把wrapper样式 放出来了

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.

useVirtualList 这个 hooks 官网示例会无限重新加载 dom
5 participants