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 scroll problem when scrollbar on html element #1751

Merged
merged 3 commits into from
May 8, 2021

Conversation

Manfred-on-github
Copy link
Contributor

@Manfred-on-github Manfred-on-github commented May 7, 2021

Description

fix for issue #1745

looks like browsers are quirky beyond believe.
The properties .clientHeight and .getBoundingClientRect() are inconsistent for regular elements compared to the outmost 'html' element.

Hope you don't mind the verbose comment, and sorry for being late to look at this problem.

Have also enhanced test file covering previous problem of #1727 to contain an additional scrollbar on html element to see any adverse effects, of course such scrollbar defeats the concept of automatically scrolling while user drags something, since you don't know what needs to be moved.

Checklist

  • Created tests which fail without the change (this is your test file website.html)
  • All tests passing (yarn test)

Copy link
Member

@adumesny adumesny left a comment

Choose a reason for hiding this comment

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

thanks for looking at that.

src/utils.ts Outdated
//So for 'html', we compare pointer position/height against viewport, for which top is 0,
//if some inner element is our scrollElement, we must compare it to the client position of that element.
let offsetTop = 0;
if (scrollEl != document.scrollingElement) {
Copy link
Member

@adumesny adumesny May 7, 2021

Choose a reason for hiding this comment

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

!== please

also maybe use this.getScrollElement() (which I JUST RENAMED but changed to use document.scrollingElement 2 weeks ago from document.documentElement for Safari and being correct). This way there is only 1 place to change (if ever)

src/utils.ts Outdated
@@ -351,7 +351,18 @@ export class Utils {
static updateScrollResize(event: MouseEvent, el: HTMLElement, distance: number): void {
const scrollEl = this.getScrollParent(el);
const height = scrollEl.clientHeight;
const offsetTop = scrollEl.getBoundingClientRect().top;
//when does pointer move out of visible area, which is then fixed by scrolling that element?
Copy link
Member

Choose a reason for hiding this comment

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

not sure that first line help - also wrong syntax.
Also please change comments to '// ' (add space)

can we maybe rephrase this comment ? It's not helping me. I would document why we need offsetTop at all, THEN explain that document HTML is different due to returning viewport and not actual needed height. or something like that...

I have not looked into this so taking you word for it. Also add #1745 #1727 to your comment.

@adumesny adumesny merged commit 81bcbd3 into gridstack:master May 8, 2021
@adumesny
Copy link
Member

adumesny commented May 8, 2021

thank you!

@Manfred-on-github Manfred-on-github deleted the issue-1745 branch May 10, 2021 07:00
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.

2 participants