Twenty Seventeen: Sticky header offset issue

[anevins]

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

[jbpaul17]

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

[jasonlcrane]

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.

[jasonlcrane]

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

[laurelfulford]

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: ​https://cloudup.com/iDM59YBpLGs

Logged in: ​https://cloudup.com/iN4RO_HtF9K

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!

[rehanali]

Added patch

[poena]

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.

Patch https://core.trac.wordpress.org/attachment/ticket/42598/%2342598.patch
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.

[sabernhardt]

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.

[karmatosed]

I think this is worth considering for the release but I do think we need to consider if we want this now.

[karmatosed]

Assigning to myself to test and see if still an issue or not.

[karmatosed]

I am struggling to recreate this. Now, this might be me or it might be what @sabernhardt is saying:

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

For now, I am going to recommend we have more testing on this and move out of release until are sure it is something still an issue with more steps.