Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#48763 closed defect (bug) (fixed)

Twenty Twenty: SmoothScroll is broken

Reported by: joostdevalk's profile joostdevalk Owned by: audrasjb's profile audrasjb
Milestone: 5.3.1 Priority: normal
Severity: normal Version: 5.3
Component: Bundled Theme Keywords: has-patch has-screenshots commit has-dev-note
Focuses: accessibility, javascript, css Cc:

Description

Go to:

https://joost.blog/cms-market-share-november-2019-analysis/

Click any link in the table of contents. You'll be scrolled to the right place. Then go up, and click another link. You're smooth scrolled to the first link you clicked on.

This break the usefulness of all anchor links on a page.

Attachments (3)

89661d8246796247adf8a17c4d88edab.gif (2.7 MB) - added by audrasjb 5 years ago.
48763.diff - Remove SmoothScroll JS bit and replace it with a CSS scroll-behavior property. Scroll to top example.
3672237f281fd2f087d7753d2ab6ac5d.gif (957.1 KB) - added by audrasjb 5 years ago.
48763.diff - Remove SmoothScroll JS bit and replace it with a CSS scroll-behavior property - internal link + anchor example
48763.diff (5.3 KB) - added by audrasjb 5 years ago.
48763.diff - Remove SmoothScroll JS bit and replace it with a CSS scroll-behavior property

Change History (30)

#1 @joostdevalk
5 years ago

  • Component changed from General to Themes

This ticket was mentioned in Slack in #core-themes by joostdevalk. View the logs.


5 years ago

#3 @tobifjellner
5 years ago

  • Summary changed from SmootScroll is broken to SmoothScroll is broken

#4 @SergeyBiryukov
5 years ago

  • Component changed from Themes to Bundled Theme
  • Summary changed from SmoothScroll is broken to Twenty Twenty: SmoothScroll is broken

#6 @ianbelanger
5 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.3.1

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


5 years ago

#9 @melchoyce
5 years ago

Could we use https://css-tricks.com/almanac/properties/s/scroll-behavior/ instead, and let unsupported browsers gracefully degrade?

@audrasjb
5 years ago

48763.diff - Remove SmoothScroll JS bit and replace it with a CSS scroll-behavior property. Scroll to top example.

@audrasjb
5 years ago

48763.diff - Remove SmoothScroll JS bit and replace it with a CSS scroll-behavior property - internal link + anchor example

@audrasjb
5 years ago

48763.diff - Remove SmoothScroll JS bit and replace it with a CSS scroll-behavior property

#10 @audrasjb
5 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to audrasjb
  • Status changed from new to accepted

Hi,

This is a brillant idea @melchoyce , thanks! ⭐️

In 48763.diff, I removed smoothScroll JS function and I replaced it with scroll-behavior CSS property.

It works pretty well (see screenshots above).

Also worth noting that I added a small accessibility enhancement by using prefers-reduced-motion: reduce media query property for users that don't want motion effects. For further explanation on this media query, see MDN documentation: https://developer.mozilla.org/en-US/docs/Web/CSS/scroll-behavior#Accessibility_concerns

Cheers,
Jb

#11 @audrasjb
5 years ago

@ianbelanger @anlino that would be great to get a review on this proposal :-)

#12 @audrasjb
5 years ago

  • Focuses accessibility added

This ticket was mentioned in Slack in #core-themes by audrasjb. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-css by audrasjb. View the logs.


5 years ago

#15 @audrasjb
5 years ago

  • Keywords has-screenshots needs-dev-note added

#16 @audrasjb
5 years ago

  • Focuses css added

#17 @mauteri
5 years ago

Another issue related to SmoothScrolling... I noticed this with our WordCamp site using the new Twenty Twenty theme with CampTix plugin.

When you sign up and receive your WordCamp ticket, you are sent an email with "Edit Information" from what you provided. That link looks something like this...

https://2020.WORDCAMP.wordcamp.org/tickets/?tix_action=access_tickets&tix_access_token=TOKEN#tix

Since it's the same URL and ends with a hash, the JS code for SmoothScrolling takes over so you cannot actually load the link to edit your information. In addition to checking that the URL is the same, you should also check for different URL parameters for cases like this.

If this should be a separate ticket let me know. Be happy to open another, but figured it was sort of related and might make sense tied to this ticket. Thanks!

This ticket was mentioned in Slack in #meta-wordcamp by mauteri. View the logs.


5 years ago

#19 follow-up: @audrasjb
5 years ago

HI @mauteri ,
Yes I think this specific issue should be addressed in another ticket :)
Maybe even a meta ticket on meta.trac.wordpress.org, since it could be related to Tix (WordCamp Tickets).

#20 in reply to: ↑ 19 @mauteri
5 years ago

Replying to audrasjb:

HI @mauteri ,
Yes I think this specific issue should be addressed in another ticket :)
Maybe even a meta ticket on meta.trac.wordpress.org, since it could be related to Tix (WordCamp Tickets).

Hi @audrasjb it's not related to a bug in the CampTix plugin. This is with the SmoothScroll not taking URL parameters into consideration before implementing smooth scrolling preventing a URL from loading. CampTix just made the bug apparent. I can create a separate ticket for this if you like. I've already let the Meta WordCamp team know of the issue if they want to patch it sooner rather than later as it's a big issue for WordCamps using the TwentyTwenty theme. Thanks!

#21 @audrasjb
5 years ago

OK! Thanks for clarifying! In this case, the issue could indeed be fixed with the above patch.
But to be sure, it would be nice to test it directly. Maybe a meta team member could help with that? (ping @SergeyBiryukov as Sergey knows -almost- everything :D).

In any case, I think the above patch would be a great fix for the scrolling issues that happen in Twenty Twenty.

#22 @Anlino
5 years ago

@audrasjb Sorry for the late reply – thumbs up from me!

#23 @audrasjb
5 years ago

  • Keywords commit added

Great thanks for the review @Anlino ! ⭐️

Adding commit keyword so we can get this committed as soon as possible.

#24 @joostdevalk
5 years ago

Tested the patch and it works wonderfully.

#25 @SergeyBiryukov
5 years ago

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

In 46824:

Twenty Twenty: Replace Smooth Scroll JS implementation with scroll-behavior CSS property.

The JS implementation had multiple issues and did not work as expected.

This change includes an accessibility enhancement by using prefers-reduced-motion: reduce media query property for users that don't want motion effects. For further explanation on this media query, see MDN documentation: https://developer.mozilla.org/en-US/docs/Web/CSS/scroll-behavior#Accessibility_concerns

Props audrasjb, melchoyce, joostdevalk, Anlino, mauteri, sergiomdgomes, littlebigthing, williampatton, netweb, andraganescu, joyously, acosmin, mukesh27, hareesh-pillai.
Fixes #48763, #48551, #48866.

#26 @SergeyBiryukov
5 years ago

In 46825:

Twenty Twenty: Replace Smooth Scroll JS implementation with scroll-behavior CSS property.

The JS implementation had multiple issues and did not work as expected.

This change includes an accessibility enhancement by using prefers-reduced-motion: reduce media query property for users that don't want motion effects. For further explanation on this media query, see MDN documentation: https://developer.mozilla.org/en-US/docs/Web/CSS/scroll-behavior#Accessibility_concerns

Props audrasjb, melchoyce, joostdevalk, Anlino, mauteri, sergiomdgomes, littlebigthing, williampatton, netweb, andraganescu, joyously, acosmin, mukesh27, hareesh-pillai.
Merges [46824] to the 5.3 branch.
Fixes #48763, #48551, #48866.

#27 @audrasjb
5 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.