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

useInfiniteScroll 的getScrollTop方法问题 #2283

Closed
LJJCherry opened this issue Aug 4, 2023 · 26 comments · Fixed by #2285
Closed

useInfiniteScroll 的getScrollTop方法问题 #2283

LJJCherry opened this issue Aug 4, 2023 · 26 comments · Fixed by #2285

Comments

@LJJCherry
Copy link
Contributor

LJJCherry commented Aug 4, 2023

const scrollMethod = () => {
    let el = getTargetElement(target);
    if (!el) {
      return;
    }

    el = el === document ? document.documentElement : el;

    const scrollTop = getScrollTop(el);
    const scrollHeight = getScrollHeight(el);
    const clientHeight = getClientHeight(el);

    if (scrollHeight - scrollTop <= clientHeight + threshold) {
      loadMore();
    }
  };
const getScrollTop = (el: Document | Element) => {
  if (el === document || el === document.body) {
    return Math.max(
      window.pageYOffset,
      document.documentElement.scrollTop,
      document.body.scrollTop,
    );
  }
  return (el as Element).scrollTop;
}

相关代码链接:

const scrollMethod = () => {

导致el = document的时候,在某些安卓浏览器里面没有获取到scrollTop,getScrollTop应该改成

const getScrollTop = (el: Document | Element) => {
  if (el === document.documentElement || el === document.body) {
    return Math.max(
      window.pageYOffset,
      document.documentElement.scrollTop,
      document.body.scrollTop,
    );
  }
  return (el as Element).scrollTop;
}
@hchlq
Copy link
Collaborator

hchlq commented Aug 7, 2023

不能直接这么改,会有 break change,应该是安卓浏览器不兼容的问题吗?

@hchlq
Copy link
Collaborator

hchlq commented Aug 7, 2023

安卓浏览器命中了 el === document 的时候,这三个没有一个是有值的吗

window.pageYOffset
document.documentElement.scrollTop
document.body.scrollTop,

@LJJCherry
Copy link
Contributor Author

安卓浏览器命中了 el === document 的时候,这三个没有一个是有值的吗

window.pageYOffset
document.documentElement.scrollTop
document.body.scrollTop,

现在的代码当el等于document的时候走不到这个逻辑啊,在外面 el = el === document ? document.documentElement : el;
el被赋值为document.documentElement了,现在安卓的现象是
7316bd65-eaf9-4441-a285-4c346e5bd001

@hchlq
Copy link
Collaborator

hchlq commented Aug 7, 2023

应该是安卓下面的 document.documentElement.scrollTop 不兼容,是否可以换 el,之前修复的参考 #2119

@LJJCherry
Copy link
Contributor Author

应该是安卓下面的 document.documentElement.scrollTop 不兼容,是否可以换 el,之前修复的参考 #2119

我的el用的就是document的,是因为ahooks里面这个逻辑导致的安卓有问题
image

@hchlq
Copy link
Collaborator

hchlq commented Aug 7, 2023 via email

@hchlq
Copy link
Collaborator

hchlq commented Aug 7, 2023

问题上下文大概是,下面这段逻辑有问题,如果是 document,让其走 document.documentElement,不让进入这个分支

  if (el === document || el === document.body) {
    return Math.max(
      window.pageYOffset,
      document.documentElement.scrollTop,
      document.body.scrollTop,
    );
  }

@LJJCherry
Copy link
Contributor Author

LJJCherry commented Aug 7, 2023

正常情况下 document.documentElement.scrollTop 是有值的,是不是你所在的浏览器不兼容。可以看看上面提到之前修复一个问题的上下文

目前的我们这边传的el是document的,3.7.8版本的ahooks,所有的安卓手机都无法自动实现滚动加载更多,我去看下你说的那个问题,我们目前只能锁死在3.7.5的版本,
企业微信截图_deedcdfe-1d50-45c2-8335-7c84cf7ceb65
就是这次改动

@LJJCherry
Copy link
Contributor Author

问题上下文大概是,下面这段逻辑有问题,如果是 document,让其走 document.documentElement,不让进入这个分支

  if (el === document || el === document.body) {
    return Math.max(
      window.pageYOffset,
      document.documentElement.scrollTop,
      document.body.scrollTop,
    );
  }

我看你提到的那个修复,是修复的getClientHeight的问题,但那个改动影响了document 的 getScrollTop的获取,

@LJJCherry
Copy link
Contributor Author

应该是安卓下面的 document.documentElement.scrollTop 不兼容,是否可以换 el,之前修复的参考 #2119

el 应该换啥呢?本身 el 就是document,整个页面滚动到底部触发自动加载更多啊,呜呜呜~我其实尝试过用document.body,但 getClientheight又不对了

@hchlq
Copy link
Collaborator

hchlq commented Aug 7, 2023

不兼容这种情况比较少见,可能是安卓不兼容,你可以按照下面的方案来,应该是没问题的

  const el = useMemo(() => {
    const doc = document.documentElement;
    Reflect.defineProperty(doc, 'scrollTop', {
      get() {
        return Math.max(
          window.pageYOffset,
          document.documentElement.scrollTop,
          document.body.scrollTop,
        );
      },
    });
    Reflect.defineProperty(doc, 'scrollHeight', {
      get() {
        return document.documentElement.scrollHeight;
      },
    });
    Reflect.defineProperty(doc, 'clientHeight', {
      get() {
        return document.documentElement.clientHeight;
      },
    });

    return doc;
  }, []);

@LJJCherry
Copy link
Contributor Author

我其实没有理解getClientHeight获取的有问题,为什么要更改getScrollTop兼容方法的获取逻辑?

@hchlq
Copy link
Collaborator

hchlq commented Aug 7, 2023

两者不可兼得吧,绝大部分都是兼容的,document.documentElement.scrollTop 不兼容的情况很少看见

@hchlq hchlq closed this as completed Aug 7, 2023
@LJJCherry
Copy link
Contributor Author

两者不可兼得吧,绝大部分都是兼容的,document.documentElement.scrollTop 不兼容的情况很少看见

image 不能把这个逻辑移到getClientHeight里面吗?

@hchlq
Copy link
Collaborator

hchlq commented Aug 7, 2023

可以是可以,不过会出现获取一些信息元素不一致的情况,可能会存在其他场景的 badcase

@LJJCherry
Copy link
Contributor Author

getScrollTop 这个方法我搜了一下只有 useInfiniteScroll 这个hook在用,目前 badcase 就在眼前啊,如果有demo的话可以试试安卓机

@hchlq
Copy link
Collaborator

hchlq commented Aug 7, 2023

其他场景指的是 document 获取 scrollTop 和 clientHeight 不一致的情况,不是指被引用情况

@hchlq
Copy link
Collaborator

hchlq commented Aug 7, 2023

想了一下,应该需要处理一下,可否来个 PR

@hchlq hchlq reopened this Aug 7, 2023
@hchlq
Copy link
Collaborator

hchlq commented Aug 7, 2023

如果没时间提 PR 的话说下哈,后续我处理一下

@LJJCherry
Copy link
Contributor Author

LJJCherry commented Aug 7, 2023

我提一个 PR 吧,#2285

@hchlq
Copy link
Collaborator

hchlq commented Aug 7, 2023

这个方法可以解决你的问题吗

  const el = useMemo(() => {
    const doc = document.documentElement;
    Reflect.defineProperty(doc, 'scrollTop', {
      get() {
        return Math.max(
          window.pageYOffset,
          document.documentElement.scrollTop,
          document.body.scrollTop,
        );
      },
    });
    Reflect.defineProperty(doc, 'scrollHeight', {
      get() {
        return document.documentElement.scrollHeight;
      },
    });
    Reflect.defineProperty(doc, 'clientHeight', {
      get() {
        return document.documentElement.clientHeight;
      },
    });

    return doc;
  }, []);

@LJJCherry
Copy link
Contributor Author

这个方法可以解决你的问题吗

  const el = useMemo(() => {
    const doc = document.documentElement;
    Reflect.defineProperty(doc, 'scrollTop', {
      get() {
        return Math.max(
          window.pageYOffset,
          document.documentElement.scrollTop,
          document.body.scrollTop,
        );
      },
    });
    Reflect.defineProperty(doc, 'scrollHeight', {
      get() {
        return document.documentElement.scrollHeight;
      },
    });
    Reflect.defineProperty(doc, 'clientHeight', {
      get() {
        return document.documentElement.clientHeight;
      },
    });

    return doc;
  }, []);
image 不太行?

@hchlq
Copy link
Collaborator

hchlq commented Aug 7, 2023

@LJJCherry 改成这样即可

  const el = useMemo(() => {
    const doc = document.documentElement;
    Reflect.defineProperty(doc, 'scrollTop', {
      get() {
        return Math.max(
          window.pageYOffset,
          document.body.scrollTop,
        );
      },
    });
    Reflect.defineProperty(doc, 'scrollHeight', {
      get() {
        return document.documentElement.scrollHeight;
      },
    });
    Reflect.defineProperty(doc, 'clientHeight', {
      get() {
        return document.documentElement.clientHeight;
      },
    });

    return doc;
  }, []);

@LJJCherry
Copy link
Contributor Author

也还是不行,下面这行代码会报Maximum call stack size exceeded,屏蔽掉这个, 下面clineHeight也会报这个错,把这俩都屏蔽掉,不报错了,页面也无法滚动加载更多了

Reflect.defineProperty(doc, 'scrollHeight', {
      get() {
        return document.documentElement.scrollHeight;
      },
    });

@hchlq
Copy link
Collaborator

hchlq commented Aug 7, 2023

document.documentElement.scrollHeightdocument.documentElement.clientHeight 处理一下呢,提升一下,因为递归读取死循环了

@LJJCherry
Copy link
Contributor Author

document.documentElement.scrollHeightdocument.documentElement.clientHeight 处理一下呢,提升一下,因为递归读取死循环了

放弃,我直接先写死了3.7.5的版本号

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 a pull request may close this issue.

2 participants