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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3ab3fe6
Merge pull request #1 from youzan/dev
ystarlongzi Jun 28, 2019
2a07d42
[bugfix] Picker: fix the error at low version of the Android. See #3538
ystarlongzi Jun 28, 2019
9f567ab
Merge pull request #2 from youzan/dev
ystarlongzi Jul 11, 2019
b7d4243
Merge pull request #3 from youzan/dev
ystarlongzi Jul 13, 2019
ea67220
[bugfix] compatible `<body />` is the scene of the scrolling containe…
ystarlongzi Jul 13, 2019
b38bec5
[improvement] List: update demo
ystarlongzi Jul 13, 2019
1361f6c
[bugfix]: Index-bar: fix the bug when the scroller is not window or b…
ystarlongzi Jul 14, 2019
ad49c45
[improvement] Optimize `setRootScrollTop` implementation
ystarlongzi Jul 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/field/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import Icon from '../icon';
import Cell from '../cell';
import { cellProps } from '../cell/shared';
import { preventDefault } from '../utils/dom/event';
import { getRootScrollTop } from '../utils/dom/scroll';
import { getRootScrollTop, setRootScrollTop } from '../utils/dom/scroll';
import { createNamespace, isObj, isDef, addUnit } from '../utils';
import { isIOS } from '../utils/validate/system';

Expand Down Expand Up @@ -139,7 +139,7 @@ export default createComponent({
// https://developers.weixin.qq.com/community/develop/doc/00044ae90742f8c82fb78fcae56800
/* istanbul ignore next */
if (isIOS()) {
window.scrollTo(0, getRootScrollTop());
setRootScrollTop(getRootScrollTop());
}
},

Expand Down
13 changes: 9 additions & 4 deletions src/index-bar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
getScrollTop,
getElementTop,
getRootScrollTop,
setRootScrollTop,
getScrollEventTarget
} from '../utils/dom/scroll';

Expand Down Expand Up @@ -77,9 +78,13 @@ export default createComponent({

methods: {
onScroll() {
const scrollTop = this.scroller === window
? getScrollTop(this.scroller)
: 0;
let scrollTop;
if (this.scroller === window || this.scroller === document.body) {
scrollTop = getScrollTop(this.scroller);
} 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场景，那这里加个判断改动是最小的

const rects = this.children.map(item => ({
height: item.height,
top: getElementTop(item.$el)
Expand Down Expand Up @@ -154,7 +159,7 @@ export default createComponent({
match[0].scrollIntoView();

if (this.stickyOffsetTop) {
window.scrollTo(0, getRootScrollTop() - this.stickyOffsetTop);
setRootScrollTop(getRootScrollTop() - this.stickyOffsetTop);
}

this.$emit('select', match[0].index);
Expand Down
10 changes: 7 additions & 3 deletions src/list/demo/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 里有很多人在加载数据时，也都没有这个逻辑

setTimeout(() => {
if (isRefresh) {
list.items = [];
}

for (let i = 0; i < 10; i++) {
const text = list.items.length + 1;
list.items.push(text < 10 ? '0' + text : text);
Expand All @@ -104,11 +109,10 @@ export default {
onRefresh(index) {
const list = this.list[index];
setTimeout(() => {
list.items = [];
list.error = false;
list.finished = false;
list.refreshing = false;
window.scrollTo(0, 10);
this.onLoad(index, true);
}, 1000);
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/stepper/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { createNamespace, isDef, addUnit } from '../utils';
import { getRootScrollTop } from '../utils/dom/scroll';
import { getRootScrollTop, setRootScrollTop } from '../utils/dom/scroll';
import { isIOS } from '../utils/validate/system';

const [createComponent, bem] = createNamespace('stepper');
Expand Down Expand Up @@ -160,7 +160,7 @@ export default createComponent({
// https://developers.weixin.qq.com/community/develop/doc/00044ae90742f8c82fb78fcae56800
/* istanbul ignore next */
if (isIOS()) {
window.scrollTo(0, getRootScrollTop());
setRootScrollTop(getRootScrollTop());
}
},

Expand Down
6 changes: 3 additions & 3 deletions src/tabs/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { on, off } from '../utils/dom/event';
import { ParentMixin } from '../mixins/relation';
import { BindEventMixin } from '../mixins/bind-event';
import {
setScrollTop,
setRootScrollTop,
getScrollTop,
getElementTop,
getScrollEventTarget
Expand Down Expand Up @@ -145,7 +145,7 @@ export default createComponent({

// scroll to correct position
if (this.position === 'top' || this.position === 'bottom') {
setScrollTop(window, getElementTop(this.$el) - this.offsetTop);
setRootScrollTop(getElementTop(this.$el) - this.offsetTop);
}
},

Expand Down Expand Up @@ -184,7 +184,7 @@ export default createComponent({

// adjust tab position
onScroll() {
const scrollTop = getScrollTop(window) + this.offsetTop;
const scrollTop = getScrollTop(this.scrollEl) + this.offsetTop;
const elTopToPageTop = getElementTop(this.$el);
const elBottomToPageTop =
elTopToPageTop + this.$el.offsetHeight - this.$refs.wrap.offsetHeight;
Expand Down
21 changes: 17 additions & 4 deletions src/utils/dom/scroll.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,26 @@ type ScrollElement = HTMLElement | Window;
// get nearest scroll element
// http://w3help.org/zh-cn/causes/SD9013
// http://stackoverflow.com/questions/17016740/onscroll-function-is-not-working-for-chrome
const overflowScrollReg = /scroll|auto/i;
export function getScrollEventTarget(element: HTMLElement, rootParent: ScrollElement = window) {
let node = element;
while (
node &&
node.tagName !== 'HTML' &&
node.tagName !== 'BODY' &&
node.nodeType === 1 &&
node !== rootParent
) {
const { overflowY } = window.getComputedStyle(node);
if (overflowY === 'scroll' || overflowY === 'auto') {
return node;
if (overflowScrollReg.test(<string>overflowY)) {
if (node.tagName !== 'BODY') {
return node;
}

// see: https://github.com/youzan/vant/issues/3823
const { overflowY: htmlOverflowY } = window.getComputedStyle(<Element>node.parentNode);
if (overflowScrollReg.test(<string>htmlOverflowY)) {
return node;
}
}
node = <HTMLElement>node.parentNode;
}
Expand All @@ -33,11 +41,16 @@ export function getRootScrollTop(): number {
return window.pageYOffset || document.documentElement.scrollTop || document.body.scrollTop || 0;
}

export function setRootScrollTop(value: number) {
setScrollTop(window, value);
setScrollTop(document.body, value);
}

// 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 •
edited
Loading

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. 保持逻辑简洁可读

);
}

Expand Down