WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 7 days ago

Last modified 4 days ago

#48916 closed defect (bug) (fixed)

Twenty Twenty: anchor links don't work in mobile menu

Reported by: Giorgio25b Owned by: ianbelanger
Milestone: 5.4.2 Priority: normal
Severity: normal Version: 5.3
Component: Bundled Theme Keywords: has-patch commit fixed-major
Focuses: javascript Cc:

Description

I’m testing a scenario where there is only 1 single page and therefore the main and mobile menu are structured on a #anchor-links structure: #project, #about, #and-so-on
I did try to use the shortcut url structure /#project and also the full domain path https://my-site.com/#project but there is nothing that is actually triggering the mobile menu to close and reach that url; though the url query changes in the browser without actually getting there.
This issue was reported and fixed on the Chaplin theme https://wordpress.org/support/topic/mobile-menu-doesnt-work-for-anchor-links/#post-12134002 though despite the same code approach, Chaplin is using jQuery while TwentyTwenty is plain JS.

Attachments (1)

48916.diff (911 bytes) - added by ianbelanger 3 weeks ago.
Updated patch that adds modal check

Download all attachments as: .zip

Change History (21)

#1 @SergeyBiryukov
6 months ago

  • Component changed from Menus to Bundled Theme

#2 @ianbelanger
6 months ago

  • Milestone changed from Awaiting Review to Future Release

Just tested this and can confirm that anchor links do not work in the Mobile Menu or Desktop Expanded Menu locations.

#3 follow-up: @suzylah
5 months ago

I also have this issue – one-pager website with the main navigation all linking to #anchors within the page. The problem is, they DO actually work (smooth scroll), but only on the second click. In fact, even if the second click is yet on another link, it then scrolls to the previous one.
Example (when performed in this order):
1) Click link #A – Scrolls to #A as expected.
2) Click link #B – nothing happens
3) Click link #C – scrolls to #B
4) Click link #D – scrolls to #C
5) Click link #B, scrolls to #D
And so on...
It appears as if the target is just saved on first click, and acted upon on the next click – but I couldn't find the place in the code yet where this happens.

PS. In the mobile menu, there was a second (CSS) issue on top, the html element gets the inline styles {overflow-y: scroll;position: fixed; ... etc...} set when menu opens, which fixes the whole window and nothing seem to happen on click.
I solved it with a quick&dirty html {position: static!important;overflow-y: unset!important;}

#4 in reply to: ↑ 3 @yuhin
3 months ago

I have exactly the same issue. They don't go all the way down to the right anchor after the first anchor.

Replying to suzylah:

I also have this issue – one-pager website with the main navigation all linking to #anchors within the page. The problem is, they DO actually work (smooth scroll), but only on the second click. In fact, even if the second click is yet on another link, it then scrolls to the previous one.
Example (when performed in this order):
1) Click link #A – Scrolls to #A as expected.
2) Click link #B – nothing happens
3) Click link #C – scrolls to #B
4) Click link #D – scrolls to #C
5) Click link #B, scrolls to #D
And so on...
It appears as if the target is just saved on first click, and acted upon on the next click – but I couldn't find the place in the code yet where this happens.

PS. In the mobile menu, there was a second (CSS) issue on top, the html element gets the inline styles {overflow-y: scroll;position: fixed; ... etc...} set when menu opens, which fixes the whole window and nothing seem to happen on click.
I solved it with a quick&dirty html {position: static!important;overflow-y: unset!important;}

#5 follow-up: @samful
3 weeks ago

The patch made by @bdcstr (https://wordpress.org/support/topic/mobile-menu-doesnt-work-for-anchor-links/#post-12487136) works for me, but he says it is "temporary-and-not-perfect". I'm not sure why this is, but it seems to make the mobile menu go away and go to the correct anchor when a hyperlink on the mobile menu is clicked for me, and this doesn't occur without the fix.

I know you are busy @anlino, but can you comment on this patch? It looks like it's almost fixed to me and after fixing the Chaplin version hopefully you know what to do.

#6 in reply to: ↑ 5 @bdcstr
3 weeks ago

"Temporary" because the fix could be overwritten when you update the theme to the future version -> you need to keep that in mind
"Not perfect".. Honestly I don't recall why I said that 2 months ago :D It worked for me, and I'm happy if it helps someone ;)

Replying to samful:

The patch made by @bdcstr (https://wordpress.org/support/topic/mobile-menu-doesnt-work-for-anchor-links/#post-12487136) works for me, but he says it is "temporary-and-not-perfect". I'm not sure why this is, but it seems to make the mobile menu go away and go to the correct anchor when a hyperlink on the mobile menu is clicked for me, and this doesn't occur without the fix.

I know you are busy @anlino, but can you comment on this patch? It looks like it's almost fixed to me and after fixing the Chaplin version hopefully you know what to do.

This ticket was mentioned in PR #254 on WordPress/wordpress-develop by giorgioriccardi.


3 weeks ago

https://core.trac.wordpress.org/ticket/48916 patch mobile menu anchor links, modal menu won't close after click.

<!--
Hi there! Thanks for contributing to WordPress!

Pull Requests in this GitHub repository must be linked to a ticket in the WordPress Core Trac instance (https://core.trac.wordpress.org), and are only used for code review. No pull requests will be merged on GitHub.

See the WordPress Handbook page on using PRs for Code Review more information: https://make.wordpress.org/core/handbook/contribute/git/github-pull-requests-for-code-review/

If this is your first time contributing, you may also find reviewing these guides first to be helpful:

-->

Trac ticket:

#8 @Giorgio25b
3 weeks ago

  • Keywords has-patch needs-testing added; needs-patch removed

#9 @Giorgio25b
3 weeks ago

I've created a [PR](https://github.com/WordPress/wordpress-develop/pull/254) with the patch suggested by @bdcstr
The patch has been tested and it works; the PR carries comments and console.log lines.

Last edited 3 weeks ago by Giorgio25b (previous) (diff)

@ianbelanger
3 weeks ago

Updated patch that adds modal check

#10 follow-up: @ianbelanger
3 weeks ago

  • Milestone changed from Future Release to 5.5

Thanks for all of your work on this @Giorgio25b, @bdcstr, @samful, @yuhin and @suzylah. I have just uploaded an updated patch that includes a check to see if the modal is active. After applying @Giorgio25b patch clicking on an anchor link that was not in the modal would throw an error in the console.

Uncaught TypeError: Cannot read property 'dataset' of null

My patch fixes this by adding a check to make sure that modal is not null.

Testing on multiple different devices would be appreciated.

#11 in reply to: ↑ 10 @Giorgio25b
3 weeks ago

  • Resolution set to worksforme
  • Status changed from new to closed

Hi @ianbelanger , thanks for the attention to details!
I've pushed a comprehensive PR with a fully tested patch (local, live, mobile, tablet, win/mac, etc).
You can find it here, I wasn't sure if your diff push was requiring a new PR, but just in case ;-)
https://github.com/WordPress/wordpress-develop/pull/254/commits/b58ce5062a390413cea7aed317186dd417b4071f#diff-d25e57a01a8a4bcabdb796cfefbe4b63R141

Last edited 3 weeks ago by Giorgio25b (previous) (diff)

#12 @ianbelanger
3 weeks ago

  • Keywords commit added; needs-testing removed
  • Resolution worksforme deleted
  • Status changed from closed to reopened

Thanks for testing @Giorgio25b. There is no need to submit a PR on the github repo, as trac (here) is where WordPress is actually patched. I am reopening this ticket because the patch I submitted has not been committed to core yet. I will commit this on Monday. Thanks again!!

#13 @ianbelanger
3 weeks ago

  • Owner set to ianbelanger
  • Status changed from reopened to reviewing

#14 follow-up: @yuhin
2 weeks ago

@ianbelanger Thanks for working on this!

Last edited 2 weeks ago by yuhin (previous) (diff)

#15 in reply to: ↑ 14 @Giorgio25b
2 weeks ago

@yuhin @ianbelanger I thoroughly tested again on different mobile devices and even older ones, different browsers passed the test. On Desktop I did not experience any issue with the developer tool on any browser.

#16 @samful
2 weeks ago

Just tested 48916.diff on Linux (Firefox 75.0), iPhone XR (latest Safari), and Google Pixel 3a (latest Chrome). All seems ok! Good job with the modal fix @ianbelanger

#17 @ianbelanger
2 weeks ago

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

In 47784:

Bundled Themes: Twenty Twenty anchor links don't work in mobile menu.

Modifies the mobile modal menu javascript, so that anchor links will close the modal and scroll to the anchor within the page.

Props Giorgio25b, suzylah, yuhin, samful, bdcstr.
Fixes #48916.

#18 @ianbelanger
2 weeks ago

  • Keywords fixed-major added
  • Milestone changed from 5.5 to 5.4.2
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#19 @whyisjake
7 days ago

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

In 47829:

Bundled Themes: Twenty Twenty anchor links don't work in mobile menu.
Modifies the mobile modal menu javascript, so that anchor links will close the modal and scroll to the anchor within the page.

This brings the changes from [47784] to the 5.4 branch.

Props Giorgio25b, suzylah, yuhin, samful, bdcstr.
Fixes #48916.

Note: See TracTickets for help on using tickets.