Regression: Copy action now scrolls up the page on the privacy policy guide.

[garrett-eclipse]

While testing trunk with the Privacy Policy Guide I found the copy action now leaves the user scrolled back on the page with the copy button no longer visible. This is caused by the tutorial text being hidden then unhidden but the user view isn't scrolled back down.
Specifically this functionality changed in [47147] / #44621 and was done to ensure the copied content doesn't contain multiple empty lines.

[garrett-eclipse]

Gif to show what happens when you use the copy action now. The users view is moved up and the button is lost from view

[garrett-eclipse]

Single-line patch to address the issue and scroll the button back into view once the copy action is complete.

[garrett-eclipse]

In patch 49540.diff I've added a event.target.scrollIntoView() call to have the view return back to the button. This happens quite fast so doesn't seem to cause the page to jump.

[davidbaumwald]

Confirmed the bug and tested the patch. It seems to resolve the issue.

[garrett-eclipse]

Thanks @davidbaumwald for testing, let's get a committers review here from @SergeyBiryukov and get this fixed.

[birgire]

Hi, I tried to replicate the issue on my test install on latest Chrome and trunk but wasn't successful. Wonder what I'm missing in my late testing :-)

I wonder if the discussion here about scrollIntoView() changing the native tab sequence in (older) Chrome:

https://core.trac.wordpress.org/ticket/24048#comment:56

is relevant for this ticket here?

[garrett-eclipse]

Thanks @birgire I'll look into that Chrome issue and we can maybe adopt the code snippet there that uses scrollIntoViewIfNeeded if we find that issue affects us here as well. I'll do some testing on that over the weekend. I've removed commit for now while I investigate.

As to reproducing, are you using the latest trunk? And which section are you copying as it will only visually affect the sections with long copy that contains lots of tutorial text like the WP default privacy content as what the new copy does is hide that content making the whole page shorter momentarily and then when made visible again the user scroll position is left up the page.
*I think you have to be almost scrolled to the base of the page to cause the issue.

If you're unable to reproduce ping me on DM and we'll want through that together.

Cheers

[birgire]

thanks @garrett-eclipse, I was copying the default one on version 5.4-beta3-20200301.162127

I wonder how 49540-2.diff​ works for you, as I still can't verify it :-)

It uses the fallback approach for scrollIntoViewIfNeeded() from @afercia here,

also changes event.target to $target and uses false input to scrollIntoView().

[garrett-eclipse]

Refresh to use event.target instead of $target as this is a javascript and function and remove false as we want to return to the base of the page.

[garrett-eclipse]

Thanks for the patch @birgire

After testing I had to revert the $target to event.target as these javascript functions expect the js element and not the jquery representation of it. Also found using the default true on the scrollIntoView returned me to the same space, with false I was slightly moved up the page.

Currently testing cross-browser so if I come across other nuances will update.

[garrett-eclipse]

Changes to use combination of document.documentElement.scrollTop and document.body.scrollTop to ensure no screen jump occurs

[garrett-eclipse]

After some more testing I found the use of scrollIntoView with either the true/false option set for indicating top/bottom alignment causes the screen to jump in certain scenarios and as such isn't overly desirable.

With that in mind I switched implementations in 49540.4.diff to store the current scrollTop and then reset it after the copy action. This will ensure the users is brought back exactly where they were before leaving no jump and the only visual cue that anything occurred is the appearance of the scroll bar.

Testing cross-browser I found not all browsers supported document.documentElement.scrollTop, but those did support document.body.scrollTop so I opted for a compatible workaround to support all browsers.
Firefox - Never had the issue so no scroll action occurs.
Safari/Opera - Only supported document.body.scrollTop.
Chrome/Brave - Only supported document.documentElement.scrollTop
IE - I haven't been able to test.

I feel this is the best approach and am happy to move forward with this if a confirmation on IE can be done.

[birgire]

Thanks for update.

I think the reason I'm not able to replicate the jumps is that

$container.addClass( 'hide-privacy-policy-tutorial' );

is not hiding the current section of the tutorial on my install (latest Chrome and WP trunk).

Seems the corresponding CSS for the class is:

.hide-privacy-policy-tutorial {
        background-color: #fff;
}

.hide-privacy-policy-tutorial .wp-policy-help, /* For back-compat, see #49282 */
.hide-privacy-policy-tutorial .privacy-policy-tutorial {
        display: none;
}

...

.hide-privacy-policy-tutorial p:not(.privacy-policy-tutorial):not(.wp-policy-help) {
        margin: 1em 0;
        padding: 0;
}

Is your CSS different?

[garrett-eclipse]

Hi @birgire that matches my install. It's the change to using display: none that causes the issue of the jump and is what the latest patch addresses. Previously it was just changing visibility but that left large gaps in the copied text so the display none ensured that space was removed but causes a jump in the page on certain installs.
Are you on Windows per-chance? I noticed Firefox wasn't affected by this and some other browsers might not have the issue either so I ensured the patch didn't negatively affect those browsers while addressing the ones of issue.
If you inspect and put a breakpoint just after that addClass javascript line it should pause what's happening to the point you should see your page has gotten much shorter. And then when that class is removed the page will become longer again but your scroll position may be left half-way up the page. If it returns your view back then maybe try to reproduce on another browser. The ones i found of issue were all on mac; Chrome, Opera, Brave, Safari.

[pbiron]

I just tested 49540.4.diff on IE 11.657.18362.0 and it works great.

Marking for commit.

[birgire]

@garrett-eclipse I'm testing in latest Chrome on Windows 10 and it seems your latest patch in 49540.4.diff​ is not having any negative effect on the copy button behavior there.

... same when I tested also on FireFox 72.0.2 (64-bit) and 73.0.1 (64-bit)

thanks for testing @pbiron

let's commit

[garrett-eclipse]

Thanks for testing @pbiron & @birgire

[SergeyBiryukov]

In 47420:

Privacy: Prevent unexpected scrolling when clicking the "Copy this section to clipboard" button on Privacy Policy Guide screen.

Props garrett-eclipse, birgire, davidbaumwald, pbiron.
Fixes #49540.