[Sticky] Transitions when adjusting height

[colin-marshall]

@gehasia you recently introduced commit 26e716e. I think the lines about transitions have messed up the behavior in the sticky-nav example at the bottom of the Sticky docs. The transition is now jerky and the offset is messed up too. You can only see the bug in local docs, so compare to the live version of the Sticky docs for normal behavior.

@gehasia please have a look when you get a chance, we are working on polishing up the Sticky plugin this week. Thanks!

[gehasia]

Hi, i've done some more work on it last week, and changed the way container resizing is fired up. But I didn't PR for it yet.
I'll try to test it against the sticky-nav at the bottom of the sticky docs tomorrow (i won't be there for today) and try to debug it quickly.
Does something like an option to activate/deactivate the container resize automation seems reasonable to introduce too (autoresizeContainer=false by default ?) ? So it'd keep the stable behavior by default (the container does not resize) and people willing to have their sticky container resizes (for the rest of content to move up/down following the new size) could just activate the behavior.
(sorry for my bad english...).

Thanks for the report !

[colin-marshall]

@gehasia Thanks for the quick response. I am working on a fix for #8435 that is different from the suggested solutions. This may be the same thing you are trying to achieve.

I was going to see what people think about adding an option to Sticky, named something like staticHeight which defaults to true. If it's set to the true, Sticky will get the height of the .sticky element right before "sticky" gets removed (like when you scroll to an anchor set in the options), and then it will explicitly set the height of .sticky to what it was before the position property was changed to absolute.

You would set the option to false if the sticky element will change height when it becomes stuck/unstuck. When it's false, it won't do anything with the height (which is the current behavior), allowing you flexibility to alter the height in CSS or JS between sticky states.

I'll put in my PR tomorrow so you can check it out and see if we're talking about the same issue.

[gehasia]

@colin-marshall I made some videos to show different behaviors.
In each videos, i start by scrolling down and up with a mouse-click on the scroll bar. Then i scroll down and up with the mouse wheel. The sticky-container background is set to red, and the sticky element is the nav bar with black background. I made videos because i think it's more explicit than tons of textual explanations (in bad english...)

The current foundation "stable" behavior, as seen in the current online documentation :
Foundation stable branch

The sticky container does not resize so the nav-bar just resize into this "fixed height" element. It flash 'red' on mouse wheel, and if we scroll down with mouse-click on scroll bar we just see the container. When it's white container on white background (like it is in current documentation example) it's not odd, but if you start to have different colors it'll flash and be ugly... That's the behavior i tried to correct with my first patch proposal

Current develop branch (with my merged PR....):
Foundation Develop branch

Now the container resizes, BUT it resizes AFTER transition (because the code wait for the transition to end before firing container resizing, it needs to end to get the correct size by DOM inspection).
So nav-bar height change -> nav-bar transition -> container height change -> (optional container transition), It's jerky as you said...

So here is my new proposal, the first one is a patch to resize BEFORE transition :
Foundation Patch 1
When scrolling down using scrollbar everything is fine, resizing is made simultaneously on the sticky element and on the container, so no more red flash. But when using mouse-wheel, which scroll down a lot more at each step than using the scrollbar we see that the title disapear. That's because container is resized, so content is moving up, and as the mouse wheel scroll down a lot more thant the scroll bar, the content is immediately hidden behind the now stuck 'sticky bar'. This is a normal behavior but a little bit jerky for end user too...

So i made a little more to the patch to add a "slowdown this scroll down":
Foundation Patch 2

Behavior with scrollbar is still ok, behavior with mouse wheel is now OK too. The patch consists to force to scroll down only 1px when setting the element to is-stuck. So if the mouse wheel step said to scroll down 50px, the code force it to 1px only (just the scroll step who fired the is-stuck code) and then let the others 'scroll steps' to occurs normally. It can be seen in the video by seing the scroll bar wich seems to be a little bit stuck (glued) on top when starting to scroll down.

How does it work :
During the _init() the sticky element is cloned into an hidden element where we remove the transition and change the class to get the is-stuck final height. This is the only solution i found to let the user to keep the element height in CSS (instead of a sticky option) and get it before transition end.
During the setSticky we put the newHeight and force the scroll to be only 1 px.

As you said maybe your PR will achieve the same goal, i need to see it. i'll check when you'll link it here.

[gehasia]

Here is my PR if you can have a look.
#8658

I can add some option to enable/disable this behavior if needed.

[colin-marshall]

@gehasia thanks for your work in getting the PR up for me to check out. Do you think you could turn that sticky topbar example you were using in your videos to a visual test in the test/visual/sticky/ directory?

Please pull in the latest commits from develop to get a visual test I made (209e9e2) so you can check that against your PR branch. I think you will notice a few things about containers losing height when they become unstuck. This is fixed in my PR #8663, please check out when you have a moment.

I think the issue I was trying to solve was a bug with containers losing height and you were trying to solve a bug with transition timings and height changes. They are different but closely related. I will experiment tomorrow with a combination of both our PR's to see if I can get both of our solutions working with each other.

[gehasia]

I've merged your code in my repo gehasia@afa536f and made the visual test page + minor bugfix gehasia@68575a6

and everything seems to be ok, both seems to work flawlessly. I honestly don't know in which order you can merge the whole stuff because i'm using your this.staticHeight property in a if test for my code now. Let me now if you need a new clean PR after your merged yours.

I kept the anchor test in my transition height test, because i may like to fix the bug of the sticky element which stick to "windows top" instead of "stuck navbar bottom", and thus is hidden behind the sticky navbar.

[gehasia]

Another thing,

i tried to just add
css['display'] = 'inline-table';

in
_removeSticky(isTop) { ... css['left'] = ''; css['display'] = 'inline-table'; ... }

instead of your patch, and it seems to do the trick your are resolving with your patch but without the need to add an option (staticHeight). Maybe it's not a good solution to add those kind off inline css, but it's a one liner which is working for me.

[colin-marshall]

@gehasia thanks for looking into it and making the visual test.

I think your solution of inline-table keeps the height of the container between sticky states, but it also introduces a width issue. After it becomes unstuck the first time, the width decreases a little, causing a slight increase in height. I improved the callout anchors visual test on the develop branch so you can see it better.

[gehasia]

@colin-marshall
Ok, you were right the solution was not fine (working on firefox but not chrome).
I digged a lot more into the code and found the source of two bugs :
-The one you fixed
-The one which position badly a sticky element when it's container is in another div than his top anchor

Here are two independant PR :
One for fixing a little bit the doc and your visual test+ adding some more :
#8674

You'll see the badly positioned element when you reach the bottom of #foo8 for test4 (test4 after merging this PR). The element should anchor there but instead jump in th middle of #foo9 ...

The second one is the actual fix for height without any options :
#8676

It also fix this issue #8587

So you may apply both and test and compare to yours, i didn't see any bug remaining.

If it got merged i will redo a PR for the sticky nav bar height transition after that.

[gehasia]

Ok here are the final PR :
#8713
#8714
#8715

Doc + Test PR which need to be applied in order to test correctly :
#8674

[colin-marshall]

@gehasia did we forget to revert this?:
https://github.com/zurb/foundation-sites/blob/develop/js/foundation.sticky.js#L218

I'm still seeing the "jerky" transitions in the sticky docs for the sticky nav example.

[DanielRuf]

Not sure if this is still relevant. So far I did not see any of these issues in the last releases.

Feel free to reopen if it does still occur.