Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Carousel: closing carousel view brings you back to the top of the page #1125

Closed
jeherve opened this issue Sep 22, 2014 · 47 comments · Fixed by #3033 or #12268
Closed

Carousel: closing carousel view brings you back to the top of the page #1125

jeherve opened this issue Sep 22, 2014 · 47 comments · Fixed by #3033 or #12268
Assignees
Labels
Customer Report Issues or PRs that were reported via Happiness. Previously known as "Happiness Request". [Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. [Type] Bug When a feature is broken and / or not performing as intended
Milestone

Comments

@jeherve
Copy link
Member

jeherve commented Sep 22, 2014

Steps to reproduce:

  1. Open a page with a gallery
  2. Scroll down, and click on one of the images
  3. The Carousel view appears
  4. Close the carousel view: you're back at the top of the page.

It'd be nice if closing the Carousel view brought you back to where you were on the page before you opened Carousel.

Suggested in 1924086-t

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. labels Sep 22, 2014
@jeherve jeherve added this to the vFuture milestone Sep 22, 2014
@chrisdc
Copy link
Contributor

chrisdc commented Sep 22, 2014

Sorry, but If I may ask are you talking about focus going back to the top? I can see that, but actual view isn't moving back to the top of the page for me (unless I start tabbing afterwards of course).

@jeherve
Copy link
Member Author

jeherve commented Sep 22, 2014

The actual view moves back to the top of the page for me, on 3 different test sites of mine.

@chrisdc
Copy link
Contributor

chrisdc commented Sep 22, 2014

Haven't got it yet sorry. Don't suppose it could be browser specific? (tested Chrome and Firefox in Windows 7)

@jeherve
Copy link
Member Author

jeherve commented Sep 24, 2014

Behaviour seems to change depending on the theme you use, but I can't figure out what's causing the issue.

@chrisdc
Copy link
Contributor

chrisdc commented Sep 24, 2014

Not sure if I'll be able to get it or not, but I'd like to have a go. Is
there a publicly available theme where you are experiencing the jump to top?
On 24 Sep 2014 09:55, "Jeremy Herve" [email protected] wrote:

Behaviour seems to change depending on the theme you use, but I can't
figure out what's causing the issue.


Reply to this email directly or view it on GitHub
#1125 (comment).

@jeherve
Copy link
Member Author

jeherve commented Sep 24, 2014

You can reproduce the problem on all posts on this site:
http://tsadri.hu/en/en-route-uton/

@chrisdc
Copy link
Contributor

chrisdc commented Sep 24, 2014

I don't quite get why yet, but acting on a hunch it seems to go away if you take height: 100% off the body tag in your css.

@Luegge
Copy link
Contributor

Luegge commented Nov 6, 2014

I can confirm the height: 100% solution for this issue. In a custom theme of my own, the view is back to top if height: 100% is specified for the body element and it does not when it isn't.

@csonnek
Copy link
Member

csonnek commented Aug 10, 2015

Reported in #2296217-t

@jgehrcke
Copy link

I have just spent hours debugging this issue, until I came here. I do not have a definite explanation, but let me summarize my findings. First of all, all these are related:

The issue seems to be depending on the exact combination of browser (only Chrome has been affected in my tests) and the body/html height and width and overflow setting. The issue also seems to be subject to "timing", because slowly stepping through the JS code via breakpointing "fixed" the issue for me, sometimes.

In my special case, I am using the Brooklyn theme (quite established!) together with the Jetpack Carousel, and the page scrolls to the top after having closed the lightbox/overlay. Breakpointing has shown me that the following lines in jetpack/blob/master/modules/carousel/jetpack-carousel.js are not working reliably:

// make sure to stop the page from scrolling behind the carousel overlay, so we don't trigger
// infiniscroll for it when enabled (Reader, theme infiniscroll, etc).
originalOverflow = $('body').css('overflow');
$('body').css('overflow', 'hidden');
// prevent html from overflowing on some of the new themes.
originalHOverflow = $('html').css('overflow');
$('html').css('overflow', 'hidden');

Debug output via

var scroll = $(window).scrollTop();
console.log("scroll position:", scroll);

before and after the mentioned code block has shown that the quoted code lines can reset the scroll position to 0 (top).

From the comments in this code one can already infer that this method is a little "flaky" and maybe (w/o offense) not perfectly thought-through. At least the comments do not explain why this should work in all cases. The comments also suggest that the body change has been implemented at first, and then the html change has been added to fix something later.

Now, it looks like working on both, html and body has added the unreliability on Chrome, especially considering jquery/jquery#2215 and kswedberg/jquery-smooth-scroll#53

A quote:

But this bug occurs only if the html and the body have the overflow-x, so you can
just remove the overflow-x for the html and it's okay for you I think.

Considering a thread on StackOverflow about preventing page scrolling, one can infer that the topic is rather complex and that the safest method is to actually, temporarily, store the scroll position:

http://stackoverflow.com/a/3656618

Considering the references above and the discussion in this thread here so far, it seems clear that the body element CSS is critical. In my specific scenario, the website behaves properly if only one of $('body').css('overflow', 'hidden'); and $('html').css('overflow', 'hidden'); is used.

So, outcommenting either the one or the other fixes the problem for my scenario.

I think this piece of code should be designed more reliably. It should be compatible with as many themes as possible out-of-the-box, and I think we do not want to require the theme to not have a 100 % height setting for the body.

There are two options now I think:

  • only use $('html').css('overflow', 'hidden'); -- would that be enough in all cases? Can we reliably test this?
  • temporarily store the scroll position as proposed in the SO thread, and restore it in the close handler.

Feedback welcome! I hope someone can come up with the perfect explanation and a conceptually safe solution.

Cheers!

@samhotchkiss samhotchkiss modified the milestones: vFuture, Needs Triage Aug 29, 2015
@dereksmart dereksmart modified the milestones: Not Core Jetpack Team, Needs Triage Aug 29, 2015
@alexphelps
Copy link

I've been troubleshooting my theme for hours looking for a solution to the scroll to top on click.

I found the solution recommended here actually conflicts with our theme and makes it scroll to the top now. Is there a way around this?

// make sure to stop the page from scrolling behind the carousel overlay, so we don't trigger
// infiniscroll for it when enabled (Reader, theme infiniscroll, etc).
originalOverflow = $('body').css('overflow');
$('body').css('overflow', 'hidden');
// prevent html from overflowing on some of the new themes.
originalHOverflow = $('html').css('overflow');
$('html').css('overflow', 'hidden');

Example site that has the issue - http://cpteam.bypronto.com/examples/image-galleries/tiled-gallery/

@MisterWP
Copy link

MisterWP commented Nov 5, 2015

Hello guys,

I don't know why, with the last update of JetPack, I got the same problem of going back to top when clicking the close button.

I tried to downgrade JetPack, disable all my plugins, switch to the Twenty Fifteen theme... nothing works. I still had the issue. Maybe I think a problem with the last release of JQuery !? I don't know.

So, I read you different ideas and this is what I did to solve the problem :

line 484 :

just after

            originalOverflow = $('body').css('overflow');
            $('body').css('overflow', 'hidden');
            // prevent html from overflowing on some of the new themes.
            originalHOverflow = $('html').css('overflow');
            $('html').css('overflow', 'hidden');

add this

            // Save the scroll top position and track opening
            console.log('open !');
            scrollPos = $(window).scrollTop();
            console.log("scroll position:", scrollPos);

line 538 :

just after

            return container
                .trigger('jp_carousel.beforeClose')
                .fadeOut('fast', function(){
                    container.trigger('jp_carousel.afterClose');

add this

                    // After close, set again the scroll top position 
                    $(window).scrollTop(scrollPos);
                    console.log('close final !');

It works fine for me now. I'm just not relax with the idea to change my js JetPack file.

@jeherve
Copy link
Member Author

jeherve commented Nov 17, 2015

This issue seems worse in Jetpack 3.8, as an extra # is added to the URL when you close the Carousel view. It brings you back to the top of the page every time.

This could be related to #2719.

@jeherve jeherve modified the milestones: 3.8.2, Not Core Jetpack Team, 3.8.1 Nov 17, 2015
@jeherve jeherve added the [Type] Bug When a feature is broken and / or not performing as intended label Nov 17, 2015
@dcgavril
Copy link

dcgavril commented Sep 2, 2016

@jeherve It took a while to reply because I was trying everything possible, and finally I gave up and integrated https://wordpress.org/plugins/mpcx-lightbox/ that is basicaly a Lightbox 2 barebones integration with wordpress galleries, and it can be twiked a bit to also work with single images. This one is working perfectly and not having the scroll issue (PS: also tested a lot of lightbox plugins, and the all seem to work, without creating the scroll issue, but they are adding too many "features"). I believe the issue is related to something else, in fact the site like is changing while you click on an image - it adds #image_name .. and this could cause the browser to scroll + it just messes up the pageviews analytics - see #3356

Anyway, thank you for your work and your reply.

@jeherve
Copy link
Member Author

jeherve commented Sep 2, 2016

the site like is changing while you click on an image - it adds #image_name

That's the expected behaviour with Carousel. It allows your readers to link directly to an image when sharing your posts. It doesn't affect the scroll position, though.

it just messes up the pageviews analytics - see #3356

That's also done on purpose, as you've seen in the other issue. I'll reply there. :)

@Auskrause
Copy link

Auskrause commented Nov 17, 2016

Same issue happening here, except the scroll bar is gone as well: http://www.groovypost.com/deals/black-friday-ads-deals-doorbusters-discounts/

I've followed the above instructions above while working in developer mode in Chrome. It seems something with Jetpack is injecting the overflow into the page because it is not there until after clicking on a gallery (in the list of ad scans at the bottom). Of course, removing the overflow in the HTML header and the overflow in the body makes the issue go away after the fact.

After manually removing overflow in Chrome's developer mode, then clicking on a gallery again, the problem happens once again.

I'm not really sure where to proceed to prevent these overflow from being injected.

@RafaelDeJongh
Copy link

I'm still experiencing this problem when the template has a body height of 100%! Very annoying as the support blames it on other plugins/themes rather than this problem!

@sageWebster
Copy link

Reported here: 3285515-t

@sageWebster sageWebster reopened this Jun 29, 2017
@jeherve jeherve removed this from the 3.8.1 milestone Sep 6, 2017
@jeherve
Copy link
Member Author

jeherve commented Nov 16, 2017

This also seems to happen when the Google Translate Widget is active, as reported in 785210-zen.

@danielbachhuber
Copy link
Contributor

I've been experiencing this issue too. It seems to be tied directly to:

html { height:100% }

And:

originalHOverflow = $('html').css('overflow');
$('html').css('overflow', 'hidden');

If I remove either, the issue goes away.

@danielbachhuber
Copy link
Contributor

danielbachhuber commented Jan 15, 2018

Here's the hack I put together to address the issue:

$( document.body ).on( 'click.fix-jp-carousel', 'div.gallery,div.tiled-gallery, a.single-image-gallery', function() {
	$('html').css('height', 'auto');
});
$( document.body ).on( 'jp_carousel.beforeClose', '.jp-carousel-wrap', function(){
	$('html').css('height', ''); // Reset to original
});

Because I don't know the impact of html { height: auto; } for my entire theme, I thought it easiest to conditionally set only when the gallery is open.

@fabwu
Copy link
Contributor

fabwu commented Jan 29, 2018

@danielbachhuber Your fix worked for me. Thanks!

Can we include this in a future release?

I can provide a PR or test any change if this is working for other people...

@jeherve
Copy link
Member Author

jeherve commented Jan 30, 2018

@fabwu Feel free to submit a PR, we'll review it and see if it can be included in Jetpack 👍

@SlowTG
Copy link

SlowTG commented Feb 21, 2018

@danielbachhuber Since I am a newbie, I must be doing something wrong as your proposed fix didn't work. Could you specify on what line you have put it, please?

@SlowTG
Copy link

SlowTG commented Feb 23, 2018

Anyone who would be able to point me in the right direction? :-) @jeherve @fabwu ?
Would be so appreciated!

@jeherve
Copy link
Member Author

jeherve commented Feb 23, 2018

@SlowTG You should be able to add this to your theme's header.php file, below the wp_head call, inside a script tag.

@SlowTG
Copy link

SlowTG commented Feb 24, 2018

@jeherve Thanks for clarifying! Unfortunately, that breaks the theme. Anything else I could try?

@jeherve
Copy link
Member Author

jeherve commented Feb 24, 2018

Could you clarify what you mean by "that breaks the theme"? What happens when you try to add that code?

If you'd like to provide us with more details about your site, do not hesitate to contact us via this form:
https://jetpack.com/contact-support/?rel=support

@SlowTG
Copy link

SlowTG commented Feb 24, 2018

It completely breaks the view of the home page. Sliders and Parallax effects are gone, it turns into a white page with some links on it. I had used the contact support form, but haven't heard back from them yet.

@jeherve
Copy link
Member Author

jeherve commented Feb 24, 2018

Alright! We'll get back to you soon!

@SlowTG
Copy link

SlowTG commented Feb 24, 2018

Thanks! I love the tiled gallery with lightbox, just need to get this part fixed. :-)

@dereksmart dereksmart removed their assignment Jun 15, 2018
@jenhooks
Copy link

Reported in 1481929-zen -- active theme is Bento.

@stale
Copy link

stale bot commented Apr 8, 2019

This issue has been marked as stale. This happened because:

  • It has been inactive in the past 6 months.
  • It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

@stale stale bot added the [Status] Stale label Apr 8, 2019
@matticbot matticbot added the Customer Report Issues or PRs that were reported via Happiness. Previously known as "Happiness Request". label May 9, 2019
@jeherve jeherve added this to the 7.4 milestone May 9, 2019
@jeherve jeherve self-assigned this May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Customer Report Issues or PRs that were reported via Happiness. Previously known as "Happiness Request". [Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet