-
Notifications
You must be signed in to change notification settings - Fork 15k
Fix anchor offset overlap blocking UI interactions #51954
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.
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
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Thanks for this improvement, @alikhere! Actually, I can't reproduce that existing CSS rules block interactions with elements above headings' scrollbars. E.g., I can easily scroll the The only thing that I want to clarify is: why do we need the |
@shurup
However, the ::before pseudo-elements still persist because they're defined in the compiled Docsy theme CSS (main.min.[hash].css), which we can't modify/ delete directly as it comes from the Docsy submodule. Since we can’t delete those rules at the source, we need to override them to:
Without display: none, the ::before elements still exist in the DOM and could interfere with layout or interactions. This override ensures a smooth transition to the new method. You can see the difference how h2 of "synopsis" of overlapping with h1 of "kubectl create secret docker-registry" before. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this detailed clarification! Now it totally makes sense to me, and I'm happy with the solution provided.
/lgtm
P.S. What do you think about suggesting this fix to Docsy itself? As far as I can see, here's the place in the upstream project we want to improve.
LGTM label has been added. Git tree hash: f88c7793f0a91c8a0681999d4e1f7b076eec78de
|
Thank you for the approval! I completely agree - fixing this at the source in the Docsy upstream project is the ideal long-term solution. This would benefit all projects using Docsy (not just Kubernetes), // Replace the old ::before hack with modern scroll-margin
- h2[id]:before,
- h3[id]:before,
- h4[id]:before,
- h5[id]:before {
- display: block;
- content: " ";
- margin-top: -5rem;
- height: 5rem;
- visibility: hidden;
+ h2[id],
+ h3[id],
+ h4[id],
+ h5[id] {
+ scroll-margin-top: 5rem; Once that upstream change is merged and released, later we could eventually remove our override in Kubernetes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
/cc @divya-mohan0209
Not adding an explicit hold in here, but we might want to wait a bit on this PR, as the linked issue is not reproducible by me. See #51923 (comment) @shurup are you able to reproduce this? |
While I wasn't able to reproduce it in terms of non-working scrolling (in Linux), I could see the actual overlap. After checking what |
The scrolling functionality also works on my side too- that's not the issue. The problem is the visual overlap and invisible blocking elements created by the ::before pseudo-elements. As you can see in the screenshots, the ::before elements create a 5rem invisible block above each heading that:
The Either way, I'm fine with whatever decision you all reviewer make. Just wanted to modernize the CSS approach. |
Yeah, that's what I was checking in macOS, and there doesn't seem to be an issue functionality-wise. The reason why I am a bit hesitant on this PR is cause right now we are working towards making the site as close to Docsyy as possible (see #41171), and while this PR makes the stylesheet better, it will be one more thing to keep in mind while working on the Docsy upgrade. |
@alikhere Thanks for the changes proposed through this PR. Sayak - Reg. your comment, when aligning it with the Docsy upgrade work, would it be prudent to track it all over an issue and perhaps add this as a note? Would that help with the reminder bit since this does introduce a style improvement? |
We do have a tracking issue #41171 for the upgrade work. My primary concern is that this PR isn't fixing anything that's broken right now. Don't get me wrong, the change in this PR is technically the correct thing to do, but this change might get reverted in the next bump to Docsy 0.7.0, as that version too won't have the current best practices. I haven't checked, but I believe that Docsy itself doesn't incorporate this change until the latest 2-3 releases or such. My suggestion would be to keep this PR open as things to track when we are almost up to date on Docsy. But I am flexible on that opinion and won't mind if it gets merged either, it will be just another item for me to keep in mind 😅. |
Description:
The Docsy theme uses invisible :before pseudo-elements on headings (h2[id], h3[id], etc.) to create anchor offsets that prevent headings from being hidden behind the fixed navigation bar. However, these invisible elements create "dead zones" that block interactions with elements above them, such as:
Solution:
Replace the legacy :before pseudo-element approach with modern CSS scroll-margin-top property, which achieves the same anchor offset functionality without blocking user interactions.
Changes Made:
Fixes: #51923
For more reference see #32695 #744
Screenshot
Before
After