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

[bugfix] compatible <body /> is the scene of the scrolling container #3844

Merged
merged 8 commits into from
Jul 15, 2019

Conversation

ystarlongzi
Copy link
Contributor

@ystarlongzi ystarlongzi commented Jul 13, 2019

see: #3823

html, body {
  height: 100%;
  overflow-y: scroll;
  -webkit-overflow-scrolling: touch;
}

在设置以上 css 后,通过查找一些关键词,验证可能涉及到相关的一些组件,没发现啥问题

步骤如下:

  1. 全局搜索 getScrollEventTarget 关键词,发现涉及到以下组件
  • <van-list />
  • <van-tabs />
  • <van-index-bar />
  • <van-pull-refresh />
  • mixins/popup 类。但在调用这个方法时,指定了最外层容器为当前的 this.$el,所以,下面这几个组件,都不会有问题
    • Toast
    • Dialog
    • Notify
    • <van-action-sheet />
    • <van-image-preview />
  1. 全局搜索 scrollTo( 关键词,若有涉及到 window 相关的逻辑,那就替换为 setRootScrollTop 方法。涉及组件:
  • <van-index-bar />
  • <van-field />
  • <van-stepper />
  1. 搜索 setScrollTop( 关键词,若有涉及到 window 相关的逻辑,那就替换为 setRootScrollTop 方法。涉及组件:
  • <van-tabs />
  1. 最后全局搜索下 window 关键词,看是否有涉及到 scroll 相关的逻辑,无相关逻辑

@codecov-io
Copy link

codecov-io commented Jul 13, 2019

Codecov Report

Merging #3844 into dev will decrease coverage by 0.16%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #3844      +/-   ##
==========================================
- Coverage   93.36%   93.19%   -0.17%     
==========================================
  Files         127      127              
  Lines        2788     2807      +19     
  Branches      253      255       +2     
==========================================
+ Hits         2603     2616      +13     
- Misses        155      160       +5     
- Partials       30       31       +1
Impacted Files Coverage Δ
src/field/index.js 98.7% <ø> (ø) ⬆️
src/stepper/index.js 100% <ø> (ø) ⬆️
src/utils/dom/scroll.ts 59.09% <33.33%> (-14.25%) ⬇️
src/tabs/index.js 87.75% <50%> (ø) ⬆️
src/index-bar/index.js 95% <50%> (-1.56%) ⬇️
src/dropdown-item/index.js 100% <0%> (ø) ⬆️
src/image-preview/ImagePreview.js 96.77% <0%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d2f010...ad49c45. Read the comment docs.

@@ -79,9 +79,14 @@ export default {
},

methods: {
onLoad(index) {
onLoad(index, isRefresh) {
Copy link
Member

Choose a reason for hiding this comment

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

这段改动的原因是啥

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个和本次调整 scroll 逻辑无关,只是优化模拟 onRefresh 的场景

之前 demo 里的 onRefresh 逻辑是:在触发刷新时,立即清空数据列表,这样会导致列表「闪」一下

在实际应用中,应该是在接口请求后,旧的数据列表才被「第一页」替换掉,所以,在这里加了个 isRefresh 字段,用来模拟刷新场景中,清空下之前的旧的数据列表

Copy link
Member

Choose a reason for hiding this comment

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

Get 👌

const list = this.list[index];
list.loading = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里丢了个主动将 loading 赋值为 true 的逻辑,估计很多人都会参照 demo 去写,才导致之前 issue 里有很多人在加载数据时,也都没有这个逻辑

const scrollTop = this.scroller === window
? getScrollTop(this.scroller)
: 0;
const scrollTop = getScrollTop(this.scroller);
Copy link
Contributor Author

@ystarlongzi ystarlongzi Jul 13, 2019

Choose a reason for hiding this comment

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

之前的逻辑是为了兼容 <van-popup><van-index-bar /></van-popup>,见 #3832

这次改动有点问题,我在看下

// get distance from element top to page top
export function getElementTop(element: ScrollElement) {
return (
(element === window ? 0 : (<HTMLElement>element).getBoundingClientRect().top) +
getScrollTop(window)
getRootScrollTop()
Copy link
Contributor Author

@ystarlongzi ystarlongzi Jul 13, 2019

Choose a reason for hiding this comment

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

这个函数实现逻辑感觉也有点问题,可能需要递归获取下 😹😹。考虑下面这样的结构:

<!-- 1. 第一层,window 可以滚动 -->
<body>
<div style="height: 200vh;" />

<!-- 2. 第二层,祖先元素可以滚动 -->
<div style="height: 200px; overflow-y: scroll">
  <div style="height: 200px;"></div>

   <!-- 3. 第三层,父元素也能滚动 -->
   <div style="height: 200px; overflow-y: scroll">
     <div style="height: 200px;"></div>
     <div style="height: 200px;"></div>
     <div style="height: 200px;"></div>
   </div>

  <div style="height: 200px;"></div>
</div>

如果要获取第三层元素距离页面顶部的距离,就需要一级级遍历下其祖先元素,如果拥有 overflow-y 属性,就需要取他们的 scrollTop 值后,最后和元素自身的 rect.top 相加

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getElementTop 方法目前用在 <van-tab /><van-index-bar /> 两个组件中,然后都是在 scroll 事件中调用,用来判断是否达到容器的边界,如果要改成上面的逻辑,性能必然不高

同时,组件内一些容器边界逻辑的判断,建议可以像之前 <vant-list /> 边界判断优化 那样做

Copy link
Member

Choose a reason for hiding this comment

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

我的看法:

  1. 不为了 1% 的特殊情况而牺牲 99% 用户的体验 or 性能,要有所取舍
  2. 保持逻辑简洁可读

} else {
// see: https://github.com/youzan/vant/issues/3774
scrollTop = 0;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

如果不考虑完善 getElementTop场景,那这里加个判断改动是最小的

export function setRootScrollTop(value: number) {
const { body } = document;
window.scrollTo(0, value);
'scrollTop' in body ? (body.scrollTop = value) : (body as HTMLElement).scrollTo(0, value);
Copy link
Member

Choose a reason for hiding this comment

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

对这个地方还有一点疑惑,为什么需要同时设置 window 和 body 的滚动条位置

Copy link
Contributor Author

Choose a reason for hiding this comment

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

哦哦,忘记评论了。主要是认为 body 能滚动时,它就是根容器,然后在设置根容器滚动时,不关心它是 window 还是 body,因为它们是互斥的,同时只可能有一个会生效,于是在写法上就「偷懒」了下,无脑的都尝试去滚动下

Copy link
Member

@chenjiahan chenjiahan Jul 15, 2019

Choose a reason for hiding this comment

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

这里是不是直接调用 setScrollTop(document.body, value) 比较合适

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里是不是直接调用 setScrollTop(document.body, value) 比较合适

👍👍 我调整下

@chenjiahan chenjiahan merged commit 511087b into youzan:dev Jul 15, 2019
@chenjiahan
Copy link
Member

辛苦啦 👍

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.

3 participants