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

ResizeObserver: Use contentBoxSize #2882

Merged
merged 10 commits into from
Apr 6, 2022
Merged

ResizeObserver: Use contentBoxSize #2882

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f96118d
Use contentBoxSize
Apr 6, 2022
97d925f
Remove logic to set initial dimensions
Apr 6, 2022
e460874
Fix tests
Apr 6, 2022
5ff7b04
Set width early
Apr 6, 2022
00f17e0
Correctly set initial value
Apr 6, 2022
ec3d886
Update src/hooks/useGridDimensions.ts
amanmahajan7 Apr 6, 2022
1b4bd77
Update src/hooks/useGridDimensions.ts
amanmahajan7 Apr 6, 2022
e7754ec
Import ceil
Apr 6, 2022
8b92d6d
Revert
Apr 6, 2022
046475c
Add back mock contentBoxSize
Apr 6, 2022
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
22 changes: 8 additions & 14 deletions src/hooks/useGridDimensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ export function useGridDimensions(): [
height: number
] {
const gridRef = useRef<HTMLDivElement>(null);
const [gridWidth, setGridWidth] = useState(1);
const [gridHeight, setGridHeight] = useState(1);
const [inlineSize, setInlineSize] = useState(1);
const [blockSize, setBlockSize] = useState(1);

useLayoutEffect(() => {
const { ResizeObserver } = window;
Expand All @@ -17,26 +17,20 @@ export function useGridDimensions(): [
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
if (ResizeObserver == null) return;

function saveDimensions() {
// Get dimensions without scrollbars.
// The dimensions given by the callback entries in Firefox do not substract the scrollbar sizes.
// https://bugzilla.mozilla.org/show_bug.cgi?id=1733042
const { clientWidth, clientHeight } = gridRef.current!;
const resizeObserver = new ResizeObserver((entries) => {
const size = entries[0].contentBoxSize[0];
// TODO: remove once fixed upstream
// we reduce width by 1px here to avoid layout issues in Chrome
// https://bugs.chromium.org/p/chromium/issues/detail?id=1206298
amanmahajan7 marked this conversation as resolved.
Show resolved Hide resolved
setGridWidth(clientWidth - (devicePixelRatio % 1 === 0 ? 0 : 1));
setGridHeight(clientHeight);
}

saveDimensions();
amanmahajan7 marked this conversation as resolved.
Show resolved Hide resolved
const resizeObserver = new ResizeObserver(saveDimensions);
setInlineSize(size.inlineSize - (devicePixelRatio % 1 === 0 ? 0 : 1));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we are using inlineSize/blockSize, RDG is one step closer to handling writing mode
https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserverEntry/contentBoxSize#value

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we check that these values are the same as clientWidth/clientHeight?
Something like

if (clientWidth !== inlineSize || clientHeight !== blockSize) {
  throw new Error('🤔');
}

Just to make sure, not to be commited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can give it a try, but I'm worried about decimal values leading to more shaky grids.

Copy link
Contributor Author

@amanmahajan7 amanmahajan7 Apr 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If we have a reproducible bug example then we can test it

setBlockSize(size.blockSize);
});
resizeObserver.observe(gridRef.current!);

return () => {
resizeObserver.disconnect();
};
}, []);

return [gridRef, gridWidth, gridHeight];
return [gridRef, inlineSize, blockSize];
}