Skip to content

Conversation

@eoghanmurray
Copy link
Contributor

Fix issue where only indication I could see in any attribute that the document was scrolling was on window.pageYOffset, so we hadn't been able to replay scrolling

Website was https://www.malibal.com/boutique/pc/configurePrd.asp?idproduct=2061

I've no idea what conditions make this the case.

… document was scrolling was on `window.pageYOffset`, so we hadn't been able to replay scrolling
@eoghanmurray eoghanmurray force-pushed the pageYOffset-document-scrolling branch from bc39380 to c2700f7 Compare April 29, 2022 11:31
Copy link
Member

@Yuyz0112 Yuyz0112 left a comment

Choose a reason for hiding this comment

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

LGTM, we can merge this after the CI passed

Copy link
Member

@YunFeng0817 YunFeng0817 left a comment

Choose a reason for hiding this comment

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

The CI failed just because of some TS errors. I fixed these errors in the comments and wish these can help you.

document?.body.scrollTop ||
0,
},
initialOffset: getWindowScroll(window),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
initialOffset: getWindowScroll(window),
initialOffset: getWindowScroll(window.document),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Mark, thanks for this review! What is the idea behind switching to document rather than window? I'm happy with either, just curious.
We could probably change it to getDocumentScroll too; I'm just wondering about the semantics of it.

Copy link
Member

Choose a reason for hiding this comment

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

document.defaultView is nullable but window.document always exists.
If the parameter is a window and the window is null, the function can't retrieve a document object from it (win.document = null.document). The document and window can both be null. This is the reason those TS errors occurred.
I think renaming with getDocumentScroll is a good idea.

const id = mirror.getId(target as Node);
if (target === doc) {
const scrollEl = (doc.scrollingElement || doc.documentElement)!;
const scrollLeftTop = getWindowScroll(doc.defaultView);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const scrollLeftTop = getWindowScroll(doc.defaultView);
const scrollLeftTop = getWindowScroll(doc);

Comment on lines +172 to +193
export function getWindowScroll(win): number {
const doc = win.document;
return {
left:
doc.scrollingElement ? doc.scrollingElement.scrollLeft
: win.pageXOffset !== undefined
? win.pageXOffset
: doc?.documentElement.scrollLeft ||
doc?.body?.parentElement?.scrollLeft ||
doc?.body.scrollLeft ||
0,
top:
doc.scrollingElement ? doc.scrollingElement.scrollTop
: win.pageYOffset !== undefined
? win.pageYOffset
: doc?.documentElement.scrollTop ||
doc?.body?.parentElement?.scrollTop ||
doc?.body.scrollTop ||
0,
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export function getWindowScroll(win): number {
const doc = win.document;
return {
left:
doc.scrollingElement ? doc.scrollingElement.scrollLeft
: win.pageXOffset !== undefined
? win.pageXOffset
: doc?.documentElement.scrollLeft ||
doc?.body?.parentElement?.scrollLeft ||
doc?.body.scrollLeft ||
0,
top:
doc.scrollingElement ? doc.scrollingElement.scrollTop
: win.pageYOffset !== undefined
? win.pageYOffset
: doc?.documentElement.scrollTop ||
doc?.body?.parentElement?.scrollTop ||
doc?.body.scrollTop ||
0,
}
}
export function getWindowScroll(doc: Document) {
const win = doc.defaultView;
return {
left: doc.scrollingElement
? doc.scrollingElement.scrollLeft
: win?.pageXOffset !== undefined
? win.pageXOffset
: doc.documentElement.scrollLeft ||
doc.body?.parentElement?.scrollLeft ||
doc.body.scrollLeft ||
0,
top: doc.scrollingElement
? doc.scrollingElement.scrollTop
: win?.pageYOffset !== undefined
? win.pageYOffset
: doc.documentElement.scrollTop ||
doc.body?.parentElement?.scrollTop ||
doc.body.scrollTop ||
0,
};
}

@YunFeng0817 YunFeng0817 added the duplicate This issue or pull request already exists label Jan 9, 2023
@YunFeng0817
Copy link
Member

Close this because it is a duplicate of #1054

@YunFeng0817 YunFeng0817 closed this Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate This issue or pull request already exists

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants