Update anchor navigation with scroll margin top

[elroygohjy]

What is the purpose of this pull request?

• Documentation update
• Bug fix
• Feature addition or enhancement
• Code maintenance
• DevOps
• Improve developer experience
• Others, please explain:

Overview of changes:
Resolves #1775
Unintentional but also resolves #1915, #1638

Anything you'd like to highlight/discuss:

Testing instructions:

1. Go to any anchor navigation.
2. It still works as intended.

Proposed commit message: (wrap lines at 72 characters)

Use scroll-margin-top to improve anchor navigation

Current anchor navigation with fixed headers relies on dummy <span>
elements to prevent headings/panels from going under the fixed header,
resulting in a hacky solution.

Adopt scroll-margin-top, now fully supported by most modern browsers
(including iOS), to provide a cleaner and more reliable solution for
anchor navigation.

Let's make the following changes:
* Replace dummy span usages with scroll-margin-top
* Refactor scrollToUrlAnchorHeading to take advantage of
  scroll-margin-top

These changes resolve the following issues:
* #1775: Replace dummy <span>s with scroll-margin-top for anchor
  navigation with fixed header
* #1915: Fix id collision with markdown headings causing incorrect
  anchor scrolling effect
* #1638: Address scroll spying issue when heading has an inline include

Using scroll-margin-top eliminates the need for hacky fixes and
improves support for anchor navigation.

Checklist: ☑️

• No unrelated changes
• Updated the documentation for feature additions and enhancements
• Added tests for bug fixes or features
• Linked all related issues

[tlylt]

@elroygohjy Sorry just a quick point: I think the last portion of the PR description is meant to be included, perhaps don't remove it next time?
(this checklist part)

[elroygohjy]

Oopss, accidentally removed it. I have added it back in

[jonahtanjz]

Thanks @elroygohjy! Just some initial comments.

Will have to implement this for the panels as well :)

[tlylt]

Hi @elroygohjy, great work on this PR. Checking if you are still keen to implement the last few comments/fixes as requested?

[elroygohjy]

Hi @tlylt, @EltonGohJH would take a look into this.

[EltonGohJH]

@jonahtanjz
I updated panels with scroll margin top.

[jonahtanjz]

Thanks @EltonGohJH!

LGTM 👍

[tlylt]

@EltonGohJH there's a conflicting file, could you help to resolve it?

[tlylt]

@EltonGohJH pinging, in case you missed it.

[tlylt]

Hi @EltonGohJH, thank you for moving this PR further. I think everything works in general. However, I found a weird glitch:

Going to the preview site and clicking on one of the heading works as expected. However, if I refresh this page multiple times, the heading first goes under the header, and then goes below the expected position.

I tested it locally and the same thing occurs. The existing master does not have this behavior. Can you help to investigate?

[EltonGohJH]

Hi @EltonGohJH, thank you for moving this PR further. I think everything works in general. However, I found a weird glitch:

Going to the preview site and clicking on one of the heading works as expected. However, if I refresh this page multiple times, the heading first goes under the header, and then goes below the expected position.

I tested it locally and the same thing occurs. The existing master does not have this behavior. Can you help to investigate?

@tlylt
Do you know is anchor scrolling suppose to work on refresh? I think this may be linked to this. scroll-margin-top is too new I have no idea how to debug lol.

[tlylt]

@tlylt Do you know is anchor scrolling suppose to work on refresh? I think this may be linked to this. scroll-margin-top is too new I have no idea how to debug lol.

I don't think refresh is any different from landing on a page with the anchor specified as part of the URL (maybe there are browser differences). For MarkBind, this issue might be relevant: #1150 , we do have a function that deals with scrolling. Perhaps can start from there.

[EltonGohJH]

@tlylt Do you know is anchor scrolling suppose to work on refresh? I think this may be linked to this. scroll-margin-top is too new I have no idea how to debug lol.

I don't think refresh is any different from landing on a page with the anchor specified as part of the URL (maybe there are browser differences). For MarkBind, this issue might be relevant: #1150 , we do have a function that deals with scrolling. Perhaps can start from there.

A interesting point would be that currently our refresh with anchor is buggy on firefox. (currently not my branch)
Hmmmm. I guess I will try investigate this.

But my current theory is that refresh does not trigger rescrolling which leads to the bug in my branch.

MarkBind.-.User.Guide_.Formatting.Contents.Mozilla.Firefox.2023-03-20.17-01-30.mp4

[EltonGohJH]

@tlylt
Thank you so much for pointing me to the right direction.

scrollToUrlAnchorHeading is not doing its job on reload as the
window.scrollBy(0, -document.body.style.paddingTop.replace('px', ''));
is not retrieving the value before page is fully loaded. (So, this bug is not even related to the issue)

I have replaced it to run only window.onload and removed
window.scrollBy(0, -document.body.style.paddingTop.replace('px', '')) which is kinda hacky.

I used this instead which will account scroll-margin-top when scrolling
headingElement.scrollIntoView({ behavior: 'smooth', block: 'start', inline: 'nearest' });

Overall this proves that scroll-margin-top is indeed the most correct approach as with it we can now remove the hacky fix.

stale

[tlylt]

LGTM