Skip to content

Conversation

@drale1
Copy link
Collaborator

@drale1 drale1 commented Jan 26, 2026

Partially fixes: #762

Linked issues

Solution

Fixed the root cause of mobile header fixed position issues: Drupal sets --drupal-displace-offset-top to an empty string for anonymous users, which breaks CSS calc() fallbacks.

Changes made:

  • Split body padding rules for toolbar vs non-toolbar users
  • For anonymous/regular users (no toolbar): use only header height variable
  • For administrators with toolbar: use calc() with displacement offset (where it works correctly)

Note: This PR only fixes the mobile header location. The remaining 2 broken locations documented in #762 require separate PRs:

  • scss/components/dxpr-theme-header--top.scss:53 (overlay header)
  • features/sooper-header/header-theme-settings-css.inc:70 (desktop fixed header)

Related

Checklist

  • I have read the CONTRIBUTING.md document.
  • My commit messages follow the contributing standards and style of this project.
  • My code follows the coding standards and style of this project.
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Need to run update.php after code changes
  • Requires a change to end-user documentation.
  • Requires a change to developer documentation.
  • Requires a change to QA tests.
  • Requires a new QA test.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@drale1 drale1 requested a review from jjroelofs January 26, 2026 12:49
Copy link
Collaborator

@jjroelofs jjroelofs left a comment

Choose a reason for hiding this comment

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

(See latest review below)

Copy link
Collaborator

@jjroelofs jjroelofs left a comment

Choose a reason for hiding this comment

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

(Superseded)

Copy link
Collaborator

@jjroelofs jjroelofs left a comment

Choose a reason for hiding this comment

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

Changes Requested: Fix Root Cause

Why Root Cause Analysis Matters

This bug has now spanned 4 PRs (#677#678#759#760). Each iteration adds complexity without resolving the underlying issue. When a bug keeps returning, it signals the need to stop and investigate why rather than applying another patch. The time invested in RCA pays off by preventing future iterations.


The Root Cause

PR #678's body padding uses:

padding-top: calc(var(--dxt-setting-header-mobile-height) + var(--drupal-displace-offset-top, 0))

The bug: For anonymous users, Drupal sets --drupal-displace-offset-top to an empty string "", not undefined. CSS fallbacks don't apply to empty strings, so calc(40px + "") is invalid and ignored → 0px padding.

User Type CSS Variable Body Padding Status
Anonymous "" (empty) 0px ❌ Broken
Admin "83px" 40px ✓ Works

Issues with This PR

  1. Workaround, not fix - Adds .wrap-containers padding while body padding stays broken
  2. Unexplained 3% - Varies by viewport width (CSS % padding is width-based)

Screenshots

The bug is subtle on this landing page due to its hero section styling. Look at the gap between the language switcher bar (teal) and the "Save time" headline:

Without fix — minimal spacing (body padding: 0px):
without-fix

With PR 760 — more spacing (wrap-containers padding: 55px):
with-fix

The visual difference is subtle here, but the underlying issue is significant: on pages with page titles or content starting immediately after the header, the 0px body padding causes the fixed header to overlap content when scrolling.


Recommended Fix

// Anonymous/regular users (displacement API returns empty string)
&:has(#navbar.header-mobile-fixed):not(.toolbar-horizontal, .toolbar-vertical, .toolbar-fixed) {
  padding-top: var(--dxt-setting-header-mobile-height) !important;
}

@drale1
Copy link
Collaborator Author

drale1 commented Jan 28, 2026

@jjroelofs I fixed it according to your instructions. However, since I didn’t use 3% for padding-top, there’s missing blue space above the top of the letters:
image

On the other hand, when this is used:

padding-top: calc(var(--dxt-setting-header-mobile-height) + 3%) !important;

there is a proper blue spacing above the top of the letters:
image

If you want, we can leave like it is on first screenshot, or you can give me some advice on what would be wiser to use instead of 3%.

@drale1 drale1 requested a review from jjroelofs January 28, 2026 18:13
@jjroelofs
Copy link
Collaborator

jjroelofs commented Jan 29, 2026

@drale1 Thanks for implementing the root cause fix for the displacement API bug!

This PR fixes #762 (displacement bug only). The current fix is correct:

padding-top: var(--dxt-setting-header-mobile-height) !important;

The spacing issue (3%) is tracked in #763. The visual breathing room between header and page title is a separate enhancement. Please create a new PR for that issue.

Once you confirm the current code has no extra spacing (just header height), this PR is ready for approval.

@drale1
Copy link
Collaborator Author

drale1 commented Jan 29, 2026

@jjroelofs thanks for the review.
Confirmed: the current code uses only the header height for padding — no extra spacing (no 3% or any other additional value). The rule is:
padding-top: var(--dxt-setting-header-mobile-height) !important;
Ready for approval when you are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CSS displacement API variable fallback broken for anonymous users

3 participants