Make WordPress Core

Opened 7 years ago

Closed 2 years ago

#40492 closed defect (bug) (fixed)

Twenty Fifteen: Jumping if there is content just before the closing body tag

Reported by: stephenharris's profile stephenharris Owned by: audrasjb's profile audrasjb
Milestone: 6.0 Priority: normal
Severity: minor Version: 4.7.1
Component: Bundled Theme Keywords: has-patch has-screenshots commit assigned-for-commit
Focuses: javascript Cc:

Description

Steps to reproduce:

  1. Activate the TwentyFifteen theme
  2. Add enough widgets so that it is longer than both the screen and the main content.
  3. Ensure the viewport is wide enough so to you are not seeing a 'mobile view'.
  4. Insert content (i.e. using wp_footer) hook. In my case I have inserted an image which sits between the </body> and #page </div>. Here's what the HTML looks like:
</div><!-- .site -->

<span class="report-a-concern">
<a ...><img ..></a>
</span>

</body>
  1. View the page and scroll down. As soon as the bottom of the screen reaches the bottom of the sidebar, the page jumps back up.

Here what I found:

When the main content is longer than the sidebar TwenyFifteen's behaviour is that when you hit the bottom of the sidebar, it sticks to the bottom of the screen as you scroll further down. Scrolling back up again, the sidebar sticks at the top once the screen reaches the top of the sidebar.

However, in calculating whether the sidebar is longer than the main content or not is it using the height of body. In this instance, the main content is shorter than the sidebar, but the body is longer because of the inserted image.

Consequently, the theme thinks, when it reaches the bottom of the sidebar that it needs to fix the sidebar at the bottom of the screen. This in turn adjusts the height of the page (the body), causing the screen to jump up. The theme responds to this change in screen position by unsticking the sidebar (because you are no longer at the bottom of the page).

The fix:

Essentially replace bodyHeight = $body.height(); with bodyHeight = $('#page).height();

Context:

Inserting content using wp_footer is rather unorthodox, so I appreciate if this closed as 'wontfix' because that is not something that should be supported. The context for this is a large multisite for students of ages 5-16 to create their own blogs. A requirement is that a link appears at the bottom of all the blogs (which us a variety of themes) allowing users to report any inappropriate content (indecent images, abusive or threatening language, etc). Inserting the link using the wp_footer is the most reliable, theme-agnostic way of doing that.

Patch to follow.

Attachments (8)

40492.diff (1.4 KB) - added by stephenharris 7 years ago.
40492.mov (1.1 MB) - added by stephenharris 7 years ago.
Screencast of bug
40492.1.diff (805 bytes) - added by sabernhardt 3 years ago.
40492.2.diff (807 bytes) - added by sabernhardt 3 years ago.
adding spaces around quotes
#40492.2.patch (690 bytes) - added by rehanali 2 years ago.
Added patch
5bac9815a83620c97a0d9cef3efd6c57.mp4 (9.8 MB) - added by audrasjb 2 years ago.
Not reproductible on my side
460a4a99e7a526f46a609fa7e765e774.gif (1.3 MB) - added by audrasjb 2 years ago.
d0e091f9ee7b6c3ee83a3a84d63b1c53.gif (1.1 MB) - added by audrasjb 2 years ago.
after patch: works fine!

Change History (18)

@stephenharris
7 years ago

@stephenharris
7 years ago

Screencast of bug

#1 @SergeyBiryukov
7 years ago

  • Component changed from Themes to Bundled Theme
  • Summary changed from TwentyFifteen theme jumps if there is content just before the closing body tag. to Twenty Fifteen: Jumping if there is content just before the closing body tag

#2 @ocean90
7 years ago

  • Focuses javascript added
  • Version changed from trunk to 4.7.1

@sabernhardt
3 years ago

#3 @sabernhardt
3 years ago

  • Keywords needs-testing added; 2nd-opinion removed

Thanks for the report and patch!

I refreshed the patch. Adding an element within wp_footer may be uncommon, but I was able to think of a use case: a back-to-top link.

function sabwp_twenty_fifteen_top_link() {
	echo '<a href="#page" style="background:rgba(255,244,111,0.8);color:#157;position:fixed;bottom:10px;right:10px;padding:10px;">Back to top</a>';
}
add_action('wp_footer', 'sabwp_twenty_fifteen_top_link', 10);

Changing the variable from body height to #page height seems to work.

@sabernhardt
3 years ago

adding spaces around quotes

#4 @sabernhardt
2 years ago

  • Milestone changed from Awaiting Review to 6.0

@rehanali
2 years ago

Added patch

@audrasjb
2 years ago

Not reproductible on my side

#5 @audrasjb
2 years ago

  • Keywords dev-feedback added

Hi,

I tried to reproduce the issue on Twenty Fifteen using the following hook but I wasn't able to see the bug (see the above video capture). Does it still occurs on your side @sabernhardt?

function sabwp_twenty_fifteen_top_link() {
	echo '<a href="#page" style="background:rgba(255,244,111,0.8);color:#157;position:fixed;bottom:10px;right:10px;padding:10px;">Back to top</a>';
}
add_action('wp_footer', 'sabwp_twenty_fifteen_top_link', 10);

#6 @sabernhardt
2 years ago

  • Keywords dev-feedback removed

I found my mistake: using fixed position to show the link made the body and #page height values the same. Text alignment with static position displays the link, and the body height is greater than #page.

function sabwp_twenty_fifteen_top_link_static() {
	echo '<div style="text-align:right;"><a href="#page" style="background:rgba(255,244,111,0.8);color:#157;padding:10px;">Back to top</a></div>';
}
add_action('wp_footer', 'sabwp_twenty_fifteen_top_link_static', 10);

The post content area needs to be shorter than the header, too.

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


2 years ago

#8 @audrasjb
2 years ago

Thanks @sabernhardt I successfully reproduced the issue :)

@audrasjb
2 years ago

after patch: works fine!

#9 @audrasjb
2 years ago

  • Keywords has-screenshots commit assigned-for-commit added; needs-testing removed
  • Owner set to audrasjb
  • Status changed from new to accepted

Looks good to me!

#10 @audrasjb
2 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 53247:

Twenty Fifteen: Use #page height instead of body height in sidebar scroll calculation.

This changeset replaces bodyHeight = $body.height(); with bodyHeight = $('#page).height();. It fixes a JS issue in some specific context, when there is content generated right before the closing body tag. For example, this happens when the HTML content is added using the wp_footer filter.

Props stephenharris, sabernhardt, audrasjb.
Fixes #40492.

Note: See TracTickets for help on using tickets.