Make WordPress Core

Opened 7 years ago

Last modified 2 weeks ago

#42598 new defect (bug)

Twenty Seventeen: Sticky header offset issue

Reported by: anevins's profile anevins Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch needs-testing
Focuses: javascript Cc:


Theme: Twenty Seventeen
Version: 1.3+ (not tested below 1.3)

The theme has an option to make the header/ menu "sticky" or fixed position.

The theme also has a function that handles the position of the viewport, so that the menu does not sit on top of content navigated from internal links.

For example if you were to jump to a section of the page using '#about' the theme's JS would fire and amend the viewport in relation to the height of the fixed menu.

The bug
If a link goes to a section of content another page the theme's JS that amends the viewport no longer works.

I've tried debugging this from the support forums on someone's website and I think the issue is related to the order in which the JS fires;
1) When the page loads the header height is not fully calculated;
2) The theme's JS then moves the viewport to fix the sticky menu overlap, which uses the position of the scrollbar as reference;
3) The theme's JS then applies a margin-bottom on the header, causing a shift in the scrolling position
4) The result is that the offset is incorrect and the fixed menu sits on top of the content.

Steps to replicate:

  • Enable the sticky header
  • Create a "Continue reading" link that goes to a new page (with a hash to the section)
  • Press that "Continue reading" link to see the problem in the linked page

Attachments (3)

42598.diff (1.2 KB) - added by jasonlcrane 6 years ago.
42598.2.diff (895 bytes) - added by jasonlcrane 6 years ago.
Update to conform to WordPress' JavaScript Coding Standards, JSHint checks passed
#42598.patch (914 bytes) - added by rehanali 2 years ago.
Added patch

Download all attachments as: .zip

Change History (16)

This ticket was mentioned in Slack in #core-customize by jeffpaul. View the logs.

7 years ago

#2 @jbpaul17
7 years ago

  • Milestone changed from Awaiting Review to 5.0

Per today's 4.9 bug triage, we'll look to resolve this during the 5.0 release cycle.

6 years ago

#3 @jasonlcrane
6 years ago

  • Keywords has-patch added

I understand the issue to be this:

  1. User is on Page A.
  2. User selects a link that goes to Page B, and the href in that link also contains a hash for a specific element on Page B. For example, "/page-b#comments".
  3. Upon loading, Page B automatically scrolls to the element with the ID of "comments", but because of the fixed navigation, the top part of the "comments" section is covered by the navigation.

The patch I added fixes this issue in my testing.

#4 @SergeyBiryukov
6 years ago

  • Component changed from Themes to Bundled Theme

#5 @jasonlcrane
6 years ago

  • Keywords needs-testing added

This ticket was mentioned in Slack in #core by jasonlcrane. View the logs.

6 years ago

6 years ago

Update to conform to WordPress' JavaScript Coding Standards, JSHint checks passed

#7 @pento
6 years ago

  • Milestone changed from 5.0 to 5.0.1

#8 @laurelfulford
5 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#9 @laurelfulford
5 years ago

  • Milestone changed from 5.0.2 to 5.1

#10 @laurelfulford
5 years ago

Thanks for reporting this issue, @anevins, and for the patch, @jasonlcrane!

When testing in Firefox (64.0.2, OS 10.14), the patch works exactly as described for non-logged in users - when I follow a Continue Reading link, it sets the scroll position so the navigation isn't covering the text I would be reading next.

But when I test when logged in, the link initially goes to the scroll position it should, then offsets it.

I've created a couple quick screen captures to show what I mean -- the first line of the paragraph where the link should go is bolded and red, to make it easier to spot. I increased the timeout in the JavaScript, so it's easier to see where the link first goes, but I was getting the same effect at regular speed, too:

Not logged in:

Logged in:

In Chrome (71, OS 10.14), the logged in and non-logged in behaviour both work consistently. The only issue I ran into is that the offset is a bit off when you're logged in, so the first line of the paragraph you should be going to is covered by the menu. I'm betting this is because the admin toolbar height needs to be included in the offset.

@jasonlcrane would you be able to take another crack at this? Thanks!

#11 @laurelfulford
5 years ago

  • Milestone changed from 5.1 to Future Release

2 years ago

Added patch

#12 @poena
2 weeks ago

  • Keywords needs-refresh added; needs-testing removed

At first, I got confused because it says that the theme has an option for enabling a sticky menu.
I tried to look for an actual option in the Customizer, but did not find one.
So I assume the sticky menu is the default behavior when a menu is assigned to the "Top Menu" menu location.

I am able to reproduce the issue on WordPress trunk on Windows 11 using Chrome and Firefox.

can not be applied, it seems it is missing the path to the file.
When I add the changes manually to the file, it solves the problem. I get somewhat different results in Chrome and Firefox, it does not scroll to the exact same position.
I do not see a visible "second" layout shift, like in the videos shared in the ticket.

#13 @sabernhardt
2 weeks ago

  • Keywords needs-testing added; needs-refresh removed

The overlap hopefully is fixed already by [52198] for sites running at least WordPress 5.9 and visitors using a modern browser.

If the issue still needs work, try 42598.2.diff. That patch applies and has the same script.

Note: See TracTickets for help on using tickets.