Page MenuHomePhabricator

Enable smooth/animated scrolling between links on pages
Open, LowPublic

Description

Background

The recent design for moved paragraphs in mobile diffs (T197491) called for an animated scroll when clicking from one instance of the paragraph in question to the other. The purpose of this animation is to help the user stay oriented in terms of where the paragraph got moved to.

We initially thought this behavior would benefit Minerva in general (e.g. when using the table of contents to navigate to a page section it would be awesome to animate the scroll), but after trying it out decided to scope this change just to the mobile diffs view for now.

Developer notes

https://phabricator.wikimedia.org/T200927#4590778 provides a simple lightweight solution to this.

Let's try it out.

If it doesn't have the desired effect we should not spend time investigating alternatives. Instead we should put it back in the backlog and re-evaluate and estimate.

Note it's important that scroll-behaviour applies to a scrollable element, otherwise it won't work as we found out when we previously attempted this task.

QA instructions

*Note that this feature is not intended to work on iOS

Environment: staging
Browser & device: Android/Chrome
Skin: MFE
Steps:

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 1 2018, 6:04 PM
Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptSep 17 2018, 3:18 PM

We could do smooth scrolling with one line of CSS:

scroll-behavior: smooth;

If we're OK with it working only in Firefox, Chrome, and Chrome for Android (right now).

https://caniuse.com/#feat=css-scroll-behavior
https://developer.mozilla.org/en-US/docs/Web/CSS/scroll-behavior
https://css-tricks.com/almanac/properties/s/scroll-behavior/

...but it's one line!

Sounds great to me : )

alexhollender added a subscriber: Jdlrobson.

@Jdlrobson how do you feel about Jan's proposed solution? T200927#4590778

Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
ovasileva set the point value for this task to 1.Sep 26 2018, 4:32 PM

Which code base is this task about? MobileFrontend?

Change 480443 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] Enable smooth scrolling on in-page links for Chrome and Firefox

https://gerrit.wikimedia.org/r/480443

@alexhollender I put the patch up on staging. It should work in Chrome and Firefox.

Unfortunately this CSS property doesn't allow us to tweak the timing at all :/

http://readers-web-staging.wmflabs.org/w/index.php?title=South_Africa#Name

Due to the slow speed of the scroll the affect is a bit frustrating on long articles. Additionally, if you've used the TOC to navigate to multiple sections, then use the back button, it results in slightly confusing behavior. My apologies for not anticipating this earlier.
Here is a screen recording

For now can we restrict the implementation to anchor links within diff views, which was the initial place we realized we needed this feature?

Change 481156 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Enable smooth scrolling on mobile diff page for Chrome and Firefox

https://gerrit.wikimedia.org/r/481156

Change 480443 abandoned by Jdrewniak:
Enable smooth scrolling on in-page links for Chrome and Firefox

Reason:
This has been superseded by https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/ /481156/

https://gerrit.wikimedia.org/r/480443

Change 481156 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Enable smooth scrolling on mobile diff page for Chrome and Firefox

https://gerrit.wikimedia.org/r/481156

phuedx added a subscriber: phuedx.

It looks like this introduced a regression in the automated browser tests (N.B. the failures started on 22nd December as they were run 2 hours prior to T200927#4840811): https://integration.wikimedia.org/ci/job/selenium-MobileFrontend/

Android / Chrome: the scroll effect is working, however I have to press the up/down arrow twice in order to trigger the scroll.

iOS / Chrome, Safari: the scroll effect is not working. When I press the up/down arrow the page seems to reload at the appropriate location.

Curious if others get the same results? @ABorbaWMF @Ryasmeen @ovasileva

OS / Chrome, Safari: the scroll effect is not working. When I press the up/down arrow the page seems to reload at the appropriate location.

To quote Jan "If we're OK with it working only in Firefox, Chrome, and Chrome for Android (right now)." T200927#4590778 - this will not work on iOS.

ah, thank you for the reminder @Jdlrobson : )

I've added QA instructions to the task. Also, I just tried this again on Android/Chrome, however the scroll is not animating as it was before.

Tested on Chrome 71.0 in production and I cannot see smooth scrolling (just jumps for me).

Jdlrobson updated the task description. (Show Details)Jan 17 2019, 8:52 PM

Confirmed. also cannot see this working. Let's pull this back into needs analysis and work out what went wrong?

So it seems to work when I put the scroll-behavior: smooth; rule on the <html> element instead of the <body>... kinda.
It only starts scrolling after the second tap. I have no idea why that is. I haven't found anything intercepting the click events (yet?).

Possibly unrelated, I did notice the anchor tags have sightly malformed HTML

<a class="mw-diff-movedpara-left data-title-tag="old" href="#movedpara_3_10_rhs">&#9660;</a> - unclosed class attribute there. The only place I see that modified is here in core.

Test page

Oh, when I saw an anchor tag with href="#movedpara_3_10_rhs" I assumed there was an element with that same ID on the page. Nope.

There are instead anchor tags with a name attribute that matches the href. Wow! I guess that was a thing... in 1993.
https://eager.io/blog/the-history-of-the-url-path-fragment-query-auth/#fragments

I think if we change the name attribute to an id attribute it might work better (fingers crossed).

Jdlrobson removed the point value for this task.

<a class="mw-diff-movedpara-left data-title-tag="old" href="#movedpara_3_10_rhs">&#9660;</a> - unclosed class attribute there. The only place I see that modified is here in core.

Opened T214435 for the malformed HTML issue

I think if we change the name attribute to an id attribute it might work better (fingers crossed).

names and ids are interchangeable. This is valid HTML.

So it seems to work when I put the scroll-behavior: smooth; rule on the <html> element instead of the <body>... kinda.

I also see this and it makes sense as the scroll-behaviour CSS property only applies to scrolling boxes and body is not scrollable.

This works for example:

#mw-mf-minidiff {
  height: 200px; overflow: scroll; scroll-behavior: smooth;
}

Next steps: Let's revert the patchset that's been merged since it's a no-op re-discuss this (do we want to add the class to HTML via JS for example?) and re-estimate.

Jdlrobson removed ovasileva as the assignee of this task.Jan 22 2019, 8:55 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson lowered the priority of this task from Normal to Low.Wed, Jul 31, 6:56 PM