Fix anchor links being obscured by header#26103
Fix anchor links being obscured by header#26103Gautam-aman wants to merge 4 commits intojenkinsci:masterfrom
Conversation
|
/label skip-changelog |
|
Instead of applying a global scroll-padding based on an assumed header height, this fix uses scroll-margin-top on anchor targets. This avoids coupling to header sizing and continues to work correctly even when the header height is customized by plugins or themes. Verified manually by navigating to configuration pages using anchor URLs (/configure#jenkins-location) and confirming the target section is fully visible. |
There was a problem hiding this comment.
Pull request overview
This PR fixes anchor links being obscured by Jenkins' fixed header by adding a scroll margin to elements with IDs.
Changes:
- Added CSS rule to apply 4rem scroll-margin-top to all elements with id attributes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for the feedback. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
09c43e5 to
b559888
Compare
|
CI failures appear unrelated to this change (CSS-only). The branch is currently out of date with master; I’ve updated the branch and re-ran CI. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * See: https://github.com/jenkinsci/jenkins/issues/16803 | ||
| */ | ||
| :target { | ||
| scroll-margin-top: 4rem; |
There was a problem hiding this comment.
scroll-margin-top: 4rem is smaller than the current header height (--header-height: 4.125rem in src/main/scss/abstracts/_theme.scss), so anchor targets can still end up partially obscured (e.g., due to the extra 0.125rem plus border/rounding/zoom). Use the existing CSS variable (e.g., var(--header-height) or an appropriate calc(...)) instead of a hard-coded value so this stays correct if the header height changes.
| scroll-margin-top: 4rem; | |
| scroll-margin-top: var(--header-height); |
| <style> | ||
| /* | ||
| * Prevent anchor targets from being hidden behind the fixed Jenkins header | ||
| * See: https://github.com/jenkinsci/jenkins/issues/16803 | ||
| */ | ||
| :target { | ||
| scroll-margin-top: 4rem; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
This introduces a global CSS rule via an inline <style> in layout.jelly. Since this affects every page and Jenkins already centralizes global layout styles in the compiled styles.css bundle (from src/main/scss/...), it would be more maintainable (and better for caching) to move this rule into the appropriate SCSS file (e.g. alongside the existing scroll/header rules in src/main/scss/base/_core.scss).
| <style> | |
| /* | |
| * Prevent anchor targets from being hidden behind the fixed Jenkins header | |
| * See: https://github.com/jenkinsci/jenkins/issues/16803 | |
| */ | |
| :target { | |
| scroll-margin-top: 4rem; | |
| } | |
| </style> |
Fixes #16803
Testing done
-Started Jenkins locally from the built WAR
-Navigated to configuration pages using anchor URLs
-Verified that anchored sections are fully visible and not obscured by the fixed header
Screenshots (UI changes only)
Proposed changelog entries
N/A – UI-only CSS fix with no user-facing functional change.
Proposed changelog category
(skip)
Proposed upgrade guidelines
N/A – no upgrade or migration impact.
Submitter checklist
This is a CSS-only layout fix; Jenkins does not have automated coverage for anchor scroll positioning.
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Before the changes are marked as
ready-for-merge:Maintainer checklist
upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidateto be considered.