WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#30366 closed defect (bug) (fixed)

Twenty Fifteen: make the sidebar sticky for everyone

Reported by: celloexpressions Owned by: iandstewart
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.1
Component: Bundled Theme Keywords: needs-patch
Focuses: Cc:
PR Number:

Description

The general consensus in #29979 was that the sidebar would ideally scroll then fix when it gets to the bottom, then scroll up when the user scrolls up, then fix itself at the top until the scroll direction changes again. This is how the admin menu and other places behave.

Per @johnbillion, we can still get this in if we get a patch. Attached patch is based on the work from @avryl on #29979 and addresses the mobile and Safari issues. Needs testing on iOS. Works in Chrome and IE11 on Windows and Safari on Mac.

See also #30208.

Attachments (9)

30366.diff (4.5 KB) - added by celloexpressions 5 years ago.
30366.2.diff (4.5 KB) - added by celloexpressions 5 years ago.
Fix missed instance of potential negative margin.
30366.2.2.diff (4.6 KB) - added by celloexpressions 5 years ago.
Fix Safari, admin bar case.
30366.3.diff (4.6 KB) - added by celloexpressions 5 years ago.
Fix #30208 case.
30366.4.patch (5.3 KB) - added by honeysilvas 5 years ago.
Added scrolling for when the sidebar is longer than the content.
30366.4.1.patch (7.1 KB) - added by honeysilvas 5 years ago.
Corrected patch.
30366.5.diff (1.5 KB) - added by iamtakashi 5 years ago.
Provide portrait layout to landscape mode to avoid fixed element issues on iPad
30366.4.diff (832 bytes) - added by boonebgorges 5 years ago.
30366.6.patch (861 bytes) - added by honeysilvas 5 years ago.
Patch for jumping sidebar when scrolling to bottom.

Download all attachments as: .zip

Change History (40)

#1 @ehang
5 years ago

Tested in Safari, Firefox, and Chrome. Works fine in Firefox and Chrome. At first, it seemed like it worked in Safari, but if you scroll really quickly to the top or if you accidently keep scrolling up a few times, you are still losing the Site Title.

@celloexpressions
5 years ago

Fix missed instance of potential negative margin.

#2 @ehang
5 years ago

The second patch uploaded on the ticket fixes the Safari issue!

@celloexpressions
5 years ago

Fix Safari, admin bar case.

This ticket was mentioned in Slack in #core-themes by jcastaneda. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.


5 years ago

@celloexpressions
5 years ago

Fix #30208 case.

This ticket was mentioned in Slack in #core-themes by celloexpressions. View the logs.


5 years ago

#6 @iamtakashi
5 years ago

Since the beginning I didn't really want neither of the sidebar and the main content to go off the screen, and this approach achieves it without an additional scrollbar so I'd be very happy if this is going to work without causing big issues on our target browsers and OSs.

#7 @iamtakashi
5 years ago

Well, the main content will go off the viewport if the sidebar is really longer than the content but I still like it with the fact that the sidebar never goes off.

#8 @iandstewart
5 years ago

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

In 30388:

Twenty Fifteen: Making the sidebar sticky for everyone. When we have a long sidebar, let it scroll with the content, but fixing the sidebar and no longer scrolling when we get to the end of the sidebar content. Scroll up and the sidebar starts scrolling up to, eventually staying fixed when it gets back to the top.

Props celloexpressions, avryl, fixes #30366.

#9 @iandstewart
5 years ago

  • Keywords needs-patch added; has-patch needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Type changed from enhancement to defect (bug)

Reopening. @iamtakashi has some CSS adjustments he'd like to get in.

@honeysilvas
5 years ago

Added scrolling for when the sidebar is longer than the content.

#10 @iandstewart
5 years ago

@honeysilvas I can't reproduce an issue with scrolling when the sidebar is longer than the content. I made a video of it working correctly for me in chrome.

https://cloudup.com/clIXwza0osf

#11 @honeysilvas
5 years ago

@iandstewart This is what it looks like with the patch. When the sidebar is longer than the content, the content is the one that becomes sticky.

https://cloudup.com/c24pCVfFeWv

#12 @iandstewart
5 years ago

@honeysilvas Ah, OK. That makes sense now. That's really cool. I have two concerns about the patch.

1) We're introducing new structural HTML with #main-wrapper and moving #content one level lower. Which makes me worry that we'll break something else.
2) Is that patch missing a closing tag for #main-wrapper?

@honeysilvas
5 years ago

Corrected patch.

#13 @honeysilvas
5 years ago

1) We're introducing new structural HTML with #main-wrapper and moving #content one level lower. Which makes me worry that we'll break something else.

Yes, I thought about that. I'll try to think of a way where I won't have to add a #main-wrapper. The problem is, there is no div that contains the #content and the #footer that does not also contain the #sidebar so it's difficult to select just the #content and the #footer as a unit.

This is more a nice-to-have rather than absolutely necessary so it's up to you guys if it's worth the extra div.

2) Is that patch missing a closing tag for #main-wrapper?

Oops. Sorry about that. I must have missed a file when I created the patch. I uploaded a corrected patch with the missing div.

#14 @iamtakashi
5 years ago

Like I commented before, I wanted this theme to be like Pocket or Mac Mail where none of column goes off screen because one of the idea from the beginning is that the theme needs to be easy for users to consume lots of pages and posts that are listed on the sidebar as a navigation. So, the sidebar not being off the viewport is more important.

But I'm not saying that we shouldn't try to make the content not going off the viewport. Just I'm concerning about the new markup and I'm wondering if we can achieve this without it.

#15 @iamtakashi
5 years ago

On iPad landscape and probably android tablets, the sticky sidebar is currently not working very well when the sidebar is long but not as long as the content. The issue is hard to describe — sometime the sidebar disappears. We probably should concentrate to fix this first.

Last edited 5 years ago by iamtakashi (previous) (diff)

#16 @davidakennedy
5 years ago

I'm seeing a weird flicker on my home page: http://davidakennedy.com/ To reproduce:

  1. Visit my home page and scroll down to the bottom of the page.

It seems to only happen when the page is shorter than the sidebar.

I've reproduced in Chrome, Safari and Firefox.

I can't seem to reproduce this on a non-WordPress.com site, but I wanted to note it nonetheless. I will keep playing with it and see what else I can find out.

#17 @iamtakashi
5 years ago

Another thing with fixed position on tablet is that when we zoom the layout breaks. What do you guys think about disabling zoom? Is it really bad thing to do even type size in this theme is fairly big and readable?

Last edited 5 years ago by iamtakashi (previous) (diff)

#18 @iandstewart
5 years ago

Here's an example:

https://cldup.com/sGv0Iia6lN.png

It gets worse when you scroll. The sidebar becomes inaccessible.

#19 @iandstewart
5 years ago

@celloexpressions notes in Slack: "I tested on IE11 with touch and zooming works just fine (sticky sidebar even behaves reasonably as expected). Sounds like iOS is not being so friendly."

https://wordpress.slack.com/archives/core-themes/p1416587295000661

#20 @celloexpressions
5 years ago

Behavior in IE11 with touch, zooming and panning quickly and excessively: https://wordpress.slack.com/files/celloexpressions/F031V4XBC/twenty-fifteen-ie11-touch-zoom.gif. That's expected behavior, and anything else is likely a device/browser/webkit issue. I don't think it's worth doing anything crazy to address current behavior in specific devices, which are likely to improve over time anyway.

#21 @iamtakashi
5 years ago

Serving the desktop layout for iPad landscape is causing some tough issues because of fixed positioned elements.

  • Pinch zoom breaks the layout Slack log
  • Custom header image doesn't stay.
  • Flashing and jumping issue on fixed elements

We can find variety of solution on the web but there don't seem to be the solution that everything ties together at the moment.

So, I propose removing initial-scale=1 from the viewport meta to provide the portrait layout where there is no fixed elements to the landscape mode, and the zoom also works just fine.

@iamtakashi
5 years ago

Provide portrait layout to landscape mode to avoid fixed element issues on iPad

@boonebgorges
5 years ago

#22 @boonebgorges
5 years ago

[30388] introduced a failure to jshint. See 30366.4.diff.

This ticket was mentioned in Slack in #core-themes by netweb. View the logs.


5 years ago

#24 @iandstewart
5 years ago

r30562 takes care of 30366.4.diff

#25 @iandstewart
5 years ago

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

In 30565:

Twenty Fifteen: pinching to zoom in on landscape on iPads is breaking the layout with the sticky sidebar. Providing a one-column view on those screens looks good and will likely prevent other bugs we don't even know about.

Props iamtakashi, fixes #30366.

#26 @iandstewart
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening to make sure we cover the flickering sidebar @davidakennedy has noticed.

@honeysilvas
5 years ago

Patch for jumping sidebar when scrolling to bottom.

#27 follow-up: @honeysilvas
5 years ago

@davidakennedy Can you test if the patch I uploaded fixes your flicker issue? Thanks!

#28 @davidakennedy
5 years ago

I wanted to add my notes here, which I mentioned in the Core Themes chat today:

I successfully reproduced the bug I mentioned on a WordPress.com site other than my own. See this page: http://twentyfifteendemo.wordpress.com/a-parent-page/

It seems to be happening when the sidebar is only a certain amount longer than the page.

@honeysilvas: I'm queuing up your patch to test on a WordPress.com sandbox.

#29 in reply to: ↑ 27 @davidakennedy
5 years ago

Replying to honeysilvas:

@davidakennedy Can you test if the patch I uploaded fixes your flicker issue? Thanks!

I tested this on a WordPress.com sandbox and successfully reproduced the bug. But when I applied the patch, the bug is still there, so it doesn't look like it'll work.

Thanks for taking a shot in the dark though!

#30 @davidakennedy
5 years ago

It was also suggested during Core Themes chat that this changeset might fix it: https://core.trac.wordpress.org/changeset/30562

Specifically, taking out the unused sidebarWidth var. I tested it on my Wordpress.com sandbox and I'm still seeing the bug.

So I'll do some more testing to see if I can't isolate the issue further.

#31 @iandstewart
5 years ago

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

@iamtakashi has confirmed that the flashing on .com is a .com issue (which is getting fixed).

Note: See TracTickets for help on using tickets.