WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 weeks ago

Last modified 3 weeks ago

#37536 closed defect (bug) (fixed)

Twenty Fifteen: Improve sticky sidebar logic.

Reported by: DvanKooten Owned by: whyisjake
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.1
Component: Bundled Theme Keywords: bulk-reopened has-patch commit
Focuses: javascript Cc:
PR Number:

Description

The current logic in TwentyFifteen (#30366) for making the sidebar sticky while still allowing to scroll through it when the sidebar height is larger than the viewport height is flawed and massively overcomplicated.

This results in buggy behaviour when third-party code (in plugins) add body padding, for example.

Steps to reproduce buggy behavior:

<?php
add_action( 'wp_footer', function() {
   echo "<script>document.body.style.paddingTop = '50px';</script>";
});

Basically any plugin that implements admin bar like behavior will cause problems, with the sidebar thinking it has to apply a top offset where it actually doesn't have to do anything.

This can be mitigated by removing the admin bar specific logic and leaving most of the heavy lifting up to the browser.

Attachments (4)

37536.diff (3.4 KB) - added by DvanKooten 3 years ago.
37536.2.diff (3.4 KB) - added by DvanKooten 3 years ago.
Fix non-stickiness for small sidebars.
37536.2.2.diff (3.6 KB) - added by DvanKooten 3 years ago.
Fix non-stickiness for small sidebars.
37536.3.diff (837 bytes) - added by whyisjake 3 weeks ago.

Download all attachments as: .zip

Change History (13)

@DvanKooten
3 years ago

#1 @ocean90
3 years ago

  • Version changed from trunk to 4.1

@DvanKooten
3 years ago

Fix non-stickiness for small sidebars.

@DvanKooten
3 years ago

Fix non-stickiness for small sidebars.

#2 @lukecavanagh
3 years ago

@DvanKooten

Patch applies cleanly with no issues.

#3 @karmatosed
3 years ago

@DvanKooten do you have an example of a plugin that does this? I would like to test in a real case over outputting code to footer.

#4 @karmatosed
3 years ago

  • Keywords reporter-feedback added

#7 @ianbelanger
5 months ago

  • Keywords bulk-reopened has-patch added; reporter-feedback removed
  • Milestone set to 5.3

I tested this issue using the APEX NOTIFICATION BAR LITE plugin in the .org repo https://wordpress.org/plugins/apex-notification-bar-lite/

The issue still exists in Twenty Fifteen and the most recent patch 37536.2.2.diff does seem to fix it, with no adverse effects. Going to tag this for the 5.3 milestone. Been tested on Windows machine in Chrome, Firefox, IE11 and Edge.

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


5 weeks ago

#9 @davidbaumwald
3 weeks ago

  • Keywords commit added

Patch is still good. Was able to reproduce in Twenty Fourteen and verify that that latest patch caused no ill side effects.

#10 @whyisjake
3 weeks ago

  • Owner set to whyisjake
  • Resolution set to fixed
  • Status changed from new to closed

In 46308:

Themes: Improve Twenty Fifteen sticky sidebar logic

The current logic in TwentyFifteen (#30366) for making the sidebar sticky while still allowing to scroll through it when the sidebar height is larger than the viewport height is flawed and massively overcomplicated.
This can be mitigated by removing the admin bar specific logic and leaving most of the heavy lifting up to the browser.

Fixes #37536

Props DvanKooten, lukecavanagh, karmatosed, ianbelanger, davidbaumwald

@whyisjake
3 weeks ago

#11 @whyisjake
3 weeks ago

In 46310:

Themes: Fix javascript regression that linter found.

Tests were failing from line length optimizations and double quotes.

Fixes #37536

Note: See TracTickets for help on using tickets.