[material-ui][Drawer] Fix issue with main window being used instead of iframe's window #43818
Conversation
Netlify deploy previewhttps://deploy-preview-43818--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
albarv340
changed the title
|
Sorry if I am being dumb, but how do I add a label to a pull request? |
Janpot
added
scope: drawer
| // 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); |
There was a problem hiding this comment.
Would this ever be any different than?
| 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I also added the || window since doc.defaultView might be null.
There was a problem hiding this comment.
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);
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It's only in a few places, but fair enough, I'll open a new issue
|
Thank you! |
4 participants
Closes #43244
Before
After. Preview: https://deploy-preview-43818--material-ui.netlify.app/material-ui/react-drawer/#responsive-drawer
