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

Jump to bottom

[material-ui][Drawer] Fix issue with main window being used instead of iframe's window #43818

Merged
merged 3 commits into from
Sep 20, 2024
Merged

[material-ui][Drawer] Fix issue with main window being used instead of iframe's window #43818

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f529128
Fix issue with main window being used instead of iframe's window
albarv340 Sep 19, 2024
0a7ad22
Switch to using doc.defaultView to access the document's window
albarv340 Sep 19, 2024
503c9ef
Make it error if document doesn't have a window
albarv340 Sep 19, 2024
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
2 changes: 1 addition & 1 deletion packages/mui-material/src/Modal/ModalManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ function handleContainer(containerInfo: Container, props: ManagedModalProps) {
if (!props.disableScrollLock) {
if (isOverflowing(container)) {
// Compute the size before applying overflow hidden to avoid any scroll jumps.
const scrollbarSize = getScrollbarSize(ownerDocument(container));
const scrollbarSize = getScrollbarSize(ownerDocument(container), ownerWindow(container));

restoreStyle.push({
value: container.style.paddingRight,
Expand Down
4 changes: 2 additions & 2 deletions packages/mui-utils/src/getScrollbarSize/getScrollbarSize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// A change of the browser zoom change the scrollbar size.
// Credit https://github.com/twbs/bootstrap/blob/488fd8afc535ca3a6ad4dc581f5e89217b6a36ac/js/src/util/scrollbar.js#L14-L18
export default function getScrollbarSize(doc: Document): number {
export default function getScrollbarSize(doc: Document, win: Window = window): number {
// https://developer.mozilla.org/en-US/docs/Web/API/Window/innerWidth#usage_notes
const documentWidth = doc.documentElement.clientWidth;
return Math.abs(window.innerWidth - documentWidth);
return Math.abs(win.innerWidth - documentWidth);
Copy link
Member

@Janpot Janpot Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Would this ever be any different than?

Suggested change
return Math.abs(win.innerWidth - documentWidth);
return Math.abs(doc.defaultView.innerWidth - documentWidth);

Under the hood ownerWindow(container) would be the same as doc.defaultView || window.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does seem like it would behave the same way. And it would by extension fix all other usages of getScrollbarSize. If using the window of the given document is the intended behavior, then your suggested change is definitely better. I will change to that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also added the || window since doc.defaultView might be null.

Copy link
Member

@Janpot Janpot Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I also added the || window since doc.defaultView might be null.

We'd be comparing document width to a window it is not associated with. I think it would be a bug if we're ever in that situation. I feel like it would make more sense to error in that case 🤔.

I also feel like it almost would make more sense to start from the window instead of the document

export default function getScrollbarSize(win: Window = window): number {
  // https://developer.mozilla.org/en-US/docs/Web/API/Window/innerWidth#usage_notes
  const documentWidth = win.document.documentElement.clientWidth;
  return Math.abs(win.innerWidth - documentWidth);
}

Copy link
Contributor Author

@albarv340 albarv340 Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We'd be comparing document width to a window it is not associated with. I think it would be a bug if we're ever in that situation. I feel like it would make more sense to error in that case 🤔.

That makes sense, I can change that.

I also feel like it almost would make more sense to start from the window instead of the document

If I were to change this, I would need to change a lot of files, and I feel like it is also unrelated to this specific issue.

Copy link
Member

@Janpot Janpot Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's only in a few places, but fair enough, I'll open a new issue

}
Loading