Fix(web-react): Get initial scroll from offsetY #1203

literat · 2024-01-04T17:15:29Z

Description

useEffect runs enableScroll function by default
at that time the document.body.style.top is not set
so the function falls back to 0 even if the window is scrolled
do not work particularly with #anchors

Additional context

Fix(web-react): Add useScrollControl hook and block scrolling when Dialog open #DS-1052 #1140

Issue reference

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines.
Follow the PR Title/Commit Message Convention.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Ideally, include relevant tests that fail without this PR but pass with it.

* useEffect runs enableScroll function by default * in that time the document.body.style.top is not set * so the function fallsback to 0 even if the window is scrolled * do not work particulary with #anchors

netlify · 2024-01-04T17:15:34Z

✅ Deploy Preview for spirit-design-system-storybook ready!

Name	Link
🔨 Latest commit	`ef4d8e3`
🔍 Latest deploy log	https://app.netlify.com/sites/spirit-design-system-storybook/deploys/659812396d18c300071ef300
😎 Deploy Preview	https://deploy-preview-1203--spirit-design-system-storybook.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2024-01-04T17:16:25Z

✅ Deploy Preview for spirit-design-system-demo canceled.

Name	Link
🔨 Latest commit	`ef4d8e3`
🔍 Latest deploy log	https://app.netlify.com/sites/spirit-design-system-demo/deploys/659812391f60840008af6211

netlify · 2024-01-04T17:17:36Z

✅ Deploy Preview for spirit-design-system-validations canceled.

Name	Link
🔨 Latest commit	`ef4d8e3`
🔍 Latest deploy log	https://app.netlify.com/sites/spirit-design-system-validations/deploys/65981239608d6600083bca1a

netlify · 2024-01-04T17:17:45Z

✅ Deploy Preview for spirit-design-system-react ready!

Name	Link
🔨 Latest commit	`ef4d8e3`
🔍 Latest deploy log	https://app.netlify.com/sites/spirit-design-system-react/deploys/659812391787be0008552f48
😎 Deploy Preview	https://deploy-preview-1203--spirit-design-system-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

literat · 2024-01-04T17:21:03Z

packages/web-react/src/hooks/useScrollControl.ts

@@ -17,15 +17,15 @@ const enableScroll = (offsetY: number) => {
  body.style.paddingRight = '';
  body.style.top = '';
  body.classList.remove(CLASSNAME_SCROLLING_DISABLED);
-  window.scrollTo(0, -offsetY);
+  window.scrollTo(0, offsetY);


@crishpeen Let's discuss why there is here a negative value and what it caused. When used on Jobs.cz the Modal is rendered but not open. The useEffect hook is called causing the scroll to zero even if there is an # anchor in the URL and window.offsetY set to 5878. This scroll must be saved and used.

Same if the Modal is open -> closed -> open: the page must stay on the same offset.

I am not saying my code is flawless, probably the opposite, but with your updates our demo scrolls to the top on modal close. So I think it is still broken.

I am not saying that this is the fix. This modification works on Jobs.cz side but I did not test it on our demo. Just saved the work to consult with you.

Can you please explain the negative value for offset?

Now when I look into it, it looks like it shouldn't be negative.

You want to reset the position fixed (removing the class) and the body.top and then you want to scroll to the spot where you were before. So I think it shouldn't be negative.

I have created and example with an anchor. Now I think I know why the value is negative.

github-actions · 2024-01-04T17:21:45Z

coverage: 70.462% (-9.2%) from 79.697%
when pulling 4f2e9b1 on fix/blocking-natural-scroll
into cbe878b on main.

coveralls · 2024-01-04T17:24:35Z

coverage: 79.705% (+0.008%) from 79.697%
when pulling ef4d8e3 on fix/blocking-natural-scroll
into cbe878b on main.

literat · 2024-01-05T14:09:14Z

packages/web-react/docs/DocsSections.tsx

Revert this after PR is ready.

literat · 2024-01-05T14:10:02Z

packages/web-react/src/components/Modal/demo/index.tsx

Revert this after PR is ready

dlouhak · 2024-01-08T22:11:27Z

packages/web-react/src/hooks/useScrollControl.ts

@@ -25,7 +25,7 @@ export const useScrollControl = (ref: MutableRefObject<HTMLDialogElement | null>
    if (isOpen) {
      disableScroll();
    } else if (ref.current && !ref.current.open) {
-      const offsetY = parseFloat(document.body.style.top || '0');
+      const offsetY = parseFloat(document.body.style.top || '0') || window.scrollY;


I am afraid that this solution is still not what we need in Jobs. 😞

We need to prevent the page from bouncing once the modal is closed. But we have to consider a scenario where the modal has not been opened yet – that case falls in to this branch of the condition. That's why when it comes to the native scroll to the anchor in Jobs, we face the problem that only on Firefox we scroll to the top of the page, because Chrome scrolls to the anchor after the execution of this script.

A suggestion for modification is as follows:

Suggested change

const offsetY = parseFloat(document.body.style.top || '0') || window.scrollY;

const enableScroll = (offsetY: number) => {

…

window.scrollTo(0, offsetY);

};

const offsetY = window.scrollY ? window.scrollY : Math.max(-1 * parseFloat(document.body.style.top) || 0, 0);

When we open the modal, setting position: fixed; to body resets window.scrollY to 0. Hence, we need to ask for the value stored in top, which has saved the size of the page's offset scroll. This value corresponds to a downward shift of the page, so it is negative and we need to convert it to positive by multiplying by -1 to get the y page coordinate where we want to scroll.

If the modal is not open, the top property is not defined, so we need to look at window.scrollY if there was a scroll on the page to prevent an unwanted scrolling to y: 0.

Math.max is used to avoid the value of the variable -0.

FYI: The code suggestion has been tested on Jobs and it looks like the scrolling and open/close modal works as it should. 🎉

@crishpeen @literat I know that this solution is not ideal, but I have no idea how else to fix different native implementation of the anchor scroll. What do you think? Can we apply this suggestion or do you see any side effects or improvement ideas?

literat · 2024-01-11T10:06:46Z

Closing in favor of #1230

Fix(web-react): Get initial scroll from offsetY

4f2e9b1

* useEffect runs enableScroll function by default * in that time the document.body.style.top is not set * so the function fallsback to 0 even if the window is scrolled * do not work particulary with #anchors

literat requested review from crishpeen and dlouhak January 4, 2024 17:15

github-actions bot assigned literat Jan 4, 2024

github-actions bot added the bug Something isn't working label Jan 4, 2024

literat commented Jan 4, 2024

View reviewed changes

fixup! Fix(web-react): Get initial scroll from offsetY

11a08e3

literat commented Jan 5, 2024

View reviewed changes

packages/web-react/docs/DocsSections.tsx Outdated

Copy link

Collaborator Author

literat Jan 5, 2024 •

edited

Loading

Choose a reason for hiding this comment

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

Revert this after PR is ready.

literat commented Jan 5, 2024

View reviewed changes

packages/web-react/src/components/Modal/demo/index.tsx Outdated

Copy link

Collaborator Author

literat Jan 5, 2024

Choose a reason for hiding this comment

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

Revert this after PR is ready

literat marked this pull request as ready for review January 5, 2024 14:11

literat requested review from pavelklibani and a team as code owners January 5, 2024 14:11

literat requested a review from adamkudrna January 5, 2024 14:12

fixup! Fix(web-react): Get initial scroll from offsetY

ef4d8e3

crishpeen approved these changes Jan 5, 2024

View reviewed changes

dlouhak reviewed Jan 8, 2024

View reviewed changes

literat closed this Jan 11, 2024

literat deleted the fix/blocking-natural-scroll branch March 4, 2024 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix(web-react): Get initial scroll from offsetY #1203

Fix(web-react): Get initial scroll from offsetY #1203

literat commented Jan 4, 2024 •

edited

Loading

netlify bot commented Jan 4, 2024 •

edited

Loading

netlify bot commented Jan 4, 2024 •

edited

Loading

netlify bot commented Jan 4, 2024 •

edited

Loading

netlify bot commented Jan 4, 2024 •

edited

Loading

literat Jan 4, 2024

crishpeen Jan 5, 2024

literat Jan 5, 2024

literat Jan 5, 2024

crishpeen Jan 5, 2024

literat Jan 5, 2024

github-actions bot commented Jan 4, 2024

coveralls commented Jan 4, 2024 •

edited

Loading

literat Jan 5, 2024 •

edited

Loading

literat Jan 5, 2024

dlouhak Jan 8, 2024

dlouhak Jan 8, 2024 •

edited

Loading

literat commented Jan 11, 2024

Fix(web-react): Get initial scroll from offsetY #1203

Fix(web-react): Get initial scroll from offsetY #1203

Conversation

literat commented Jan 4, 2024 • edited Loading

Description

Additional context

Issue reference

Before submitting the PR, please make sure you do the following

netlify bot commented Jan 4, 2024 • edited Loading

✅ Deploy Preview for spirit-design-system-storybook ready!

netlify bot commented Jan 4, 2024 • edited Loading

✅ Deploy Preview for spirit-design-system-demo canceled.

netlify bot commented Jan 4, 2024 • edited Loading

✅ Deploy Preview for spirit-design-system-validations canceled.

netlify bot commented Jan 4, 2024 • edited Loading

✅ Deploy Preview for spirit-design-system-react ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 4, 2024

coveralls commented Jan 4, 2024 • edited Loading

literat Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlouhak Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

literat commented Jan 11, 2024

literat commented Jan 4, 2024 •

edited

Loading

netlify bot commented Jan 4, 2024 •

edited

Loading

netlify bot commented Jan 4, 2024 •

edited

Loading

netlify bot commented Jan 4, 2024 •

edited

Loading

netlify bot commented Jan 4, 2024 •

edited

Loading

coveralls commented Jan 4, 2024 •

edited

Loading

literat Jan 5, 2024 •

edited

Loading

dlouhak Jan 8, 2024 •

edited

Loading