WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 7 months ago

#49540 closed defect (bug) (fixed)

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

Reported by: garrett-eclipse Owned by: garrett-eclipse
Milestone: 5.4 Priority: normal
Severity: normal Version: 5.4
Component: Privacy Keywords: has-patch has-screenshots commit
Focuses: ui Cc:

Description

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.

Attachments (5)

d2dd73321c3fba9e6ec5f8544f9c285d.gif (2.3 MB) - added by garrett-eclipse 7 months ago.
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
49540.diff (441 bytes) - added by garrett-eclipse 7 months ago.
Single-line patch to address the issue and scroll the button back into view once the copy action is complete.
49540-2.diff (571 bytes) - added by birgire 7 months ago.
49540.3.diff (563 bytes) - added by garrett-eclipse 7 months ago.
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.
49540.4.diff (1.0 KB) - added by garrett-eclipse 7 months ago.
Changes to use combination of document.documentElement.scrollTop and document.body.scrollTop to ensure no screen jump occurs

Change History (21)

@garrett-eclipse
7 months ago

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
7 months ago

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

#1 @garrett-eclipse
7 months ago

  • Keywords has-patch needs-testing has-screenshots added
  • Milestone changed from Awaiting Review to 5.4
  • Owner set to garrett-eclipse
  • Status changed from new to accepted

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.

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


7 months ago

#3 @davidbaumwald
7 months ago

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

#4 @garrett-eclipse
7 months ago

  • Keywords commit added; needs-testing removed

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

#5 @birgire
7 months ago

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?

#6 @garrett-eclipse
7 months ago

  • Keywords commit removed

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

#7 @birgire
7 months ago

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().

Last edited 7 months ago by birgire (previous) (diff)

@birgire
7 months ago

@garrett-eclipse
7 months ago

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.

#8 @garrett-eclipse
7 months ago

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
7 months ago

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

#9 @garrett-eclipse
7 months ago

  • Keywords needs-testing added

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.

#10 @birgire
7 months ago

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?

#11 @garrett-eclipse
7 months ago

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.

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


7 months ago

#13 @pbiron
7 months ago

  • Keywords commit added

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

Marking for commit.

#14 @birgire
7 months ago

@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

Last edited 7 months ago by birgire (previous) (diff)

#15 @garrett-eclipse
7 months ago

  • Keywords needs-testing removed

Thanks for testing @pbiron & @birgire

#16 @SergeyBiryukov
7 months ago

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

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.

Note: See TracTickets for help on using tickets.