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

ystarlongzi · 2019-07-13T11:17:52Z

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

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

步骤如下：

全局搜索 getScrollEventTarget 关键词，发现涉及到以下组件

<van-list />
<van-tabs />
<van-index-bar />
<van-pull-refresh />
mixins/popup 类。但在调用这个方法时，指定了最外层容器为当前的 this.$el，所以，下面这几个组件，都不会有问题
- Toast
- Dialog
- Notify
- <van-action-sheet />
- <van-image-preview />

全局搜索 scrollTo( 关键词，若有涉及到 window 相关的逻辑，那就替换为 setRootScrollTop 方法。涉及组件：

<van-index-bar />
<van-field />
<van-stepper />

搜索 setScrollTop( 关键词，若有涉及到 window 相关的逻辑，那就替换为 setRootScrollTop 方法。涉及组件：

<van-tabs />

最后全局搜索下 window 关键词，看是否有涉及到 scroll 相关的逻辑，无相关逻辑

update fork

…zan#3538

update fork

…r. see youzan#3823

codecov-io · 2019-07-13T11:21:08Z

Codecov Report

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

@@            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.

chenjiahan · 2019-07-13T12:50:11Z

src/list/demo/index.vue

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

  methods: {
-    onLoad(index) {
+    onLoad(index, isRefresh) {


这段改动的原因是啥

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

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

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

ystarlongzi · 2019-07-13T13:09:50Z

src/list/demo/index.vue

      const list = this.list[index];
+      list.loading = true;


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

ystarlongzi · 2019-07-13T14:33:17Z

src/index-bar/index.js

-      const scrollTop = this.scroller === window
-        ? getScrollTop(this.scroller)
-        : 0;
+      const scrollTop = getScrollTop(this.scroller);


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

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

ystarlongzi · 2019-07-13T15:16:45Z

src/utils/dom/scroll.ts

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


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

 <body> <div style="height: 200vh;" />  <div style="height: 200px; overflow-y: scroll"> <div style="height: 200px;"></div>  <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 相加

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

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

我的看法：

不为了 1% 的特殊情况而牺牲 99% 用户的体验 or 性能，要有所取舍

保持逻辑简洁可读

…ody. see youzan#3774

ystarlongzi · 2019-07-14T06:48:05Z

src/index-bar/index.js

+      } else {
+        // see: https://github.com/youzan/vant/issues/3774
+        scrollTop = 0;
+      }


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

chenjiahan · 2019-07-15T08:20:14Z

src/utils/dom/scroll.ts

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


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

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

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

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

👍👍 我调整下

chenjiahan · 2019-07-15T11:36:27Z

辛苦啦 👍

ystarlongzi added 6 commits June 28, 2019 21:53

Merge pull request #1 from youzan/dev

3ab3fe6

update fork

[bugfix] Picker: fix the error at low version of the Android. See you…

2a07d42

…zan#3538

Merge pull request #2 from youzan/dev

9f567ab

update fork

Merge pull request #3 from youzan/dev

b7d4243

update fork

[bugfix] compatible <body /> is the scene of the scrolling containe…

ea67220

…r. see youzan#3823

[improvement] List: update demo

b38bec5

chenjiahan reviewed Jul 13, 2019

View reviewed changes

ystarlongzi commented Jul 13, 2019

View reviewed changes

[bugfix]: Index-bar: fix the bug when the scroller is not window or b…

1361f6c

…ody. see youzan#3774

ystarlongzi commented Jul 14, 2019

View reviewed changes

chenjiahan reviewed Jul 15, 2019

View reviewed changes

[improvement] Optimize setRootScrollTop implementation

ad49c45

chenjiahan merged commit 511087b into youzan:dev Jul 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ystarlongzi commented Jul 13, 2019 •

edited

Loading

codecov-io commented Jul 13, 2019 •

edited

Loading

chenjiahan Jul 13, 2019

ystarlongzi Jul 13, 2019

chenjiahan Jul 14, 2019

ystarlongzi Jul 13, 2019

ystarlongzi Jul 13, 2019 •

edited

Loading

ystarlongzi Jul 13, 2019 •

edited

Loading

ystarlongzi Jul 13, 2019

chenjiahan Jul 14, 2019

ystarlongzi Jul 14, 2019

chenjiahan Jul 15, 2019

ystarlongzi Jul 15, 2019

chenjiahan Jul 15, 2019 •

edited

Loading

ystarlongzi Jul 15, 2019

chenjiahan commented Jul 15, 2019

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

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

Conversation

ystarlongzi commented Jul 13, 2019 • edited Loading

codecov-io commented Jul 13, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ystarlongzi Jul 13, 2019 • edited Loading

Choose a reason for hiding this comment

ystarlongzi Jul 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenjiahan Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenjiahan commented Jul 15, 2019

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

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

ystarlongzi commented Jul 13, 2019 •

edited

Loading

codecov-io commented Jul 13, 2019 •

edited

Loading

ystarlongzi Jul 13, 2019 •

edited

Loading

ystarlongzi Jul 13, 2019 •

edited

Loading

chenjiahan Jul 15, 2019 •

edited

Loading