Make WordPress Core

Opened 6 years ago

Last modified 5 years 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:

Description

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 (14)

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


6 years ago

#2 @jbpaul17
6 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.

@jasonlcrane
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

@jasonlcrane
6 years ago

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

#7 @pento
5 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: 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!

#11 @laurelfulford
5 years ago

  • Milestone changed from 5.1 to Future Release

@rehanali
2 years ago

Added patch

Note: See TracTickets for help on using tickets.