Make WordPress Core

Opened 4 years ago

Closed 4 weeks ago

#53976 closed defect (bug) (duplicate)

Twenty Twenty-One: Anchor link on the same page causes the menu button icon to turn upside down

Reported by: joelbermudez's profile joelbermudez Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch
Focuses: javascript Cc:

Description

As reported and requested in https://wordpress.org/support/topic/menu-button-auto-scrolls-to-top-when-pressed-scroll-to-top-change-button-icon/.

If you have an anchor link on the same page, the icon turs upside down.
Example. In website.com/one/ if you add a link to website.com/one/#anchor, this will cause the issue. An example will be a scroll to top button as in the report mentioned.

Video showing (the bug report is for the second option. The first one was solved too in the report, but may not be the best solution, and, as the theme is not prepared to have a fixed header, and the problem is that when clicking the menu button, it automatically scrolls to the top, may not be relevant, but you can check on the report mentioned at first).

In the video you can see in 0:09 when I click "ARRIBA", that is a scroll to top in casael.com using casael.com/#inicio

https://youtu.be/zW7R58KM_BA

Problem is in primary-navigation.js

document.addEventListener( 'click', function( event ) {
            // If target onclick is <a> with # within the href attribute
            if ( event.target.hash && event.target.hash.includes( '#' ) ) {
                wrapper.classList.remove( id + '-navigation-open', 'lock-scrolling' );
                twentytwentyoneToggleAriaExpanded( mobileButton );
                // Wait 550 and scroll to the anchor.
                setTimeout(function () {
                    var anchor = document.getElementById(event.target.hash.slice(1));
                    anchor.scrollIntoView();
                }, 550);
            }
        }

Possible Solution:

Change

document.addEventListener( 'click', function( event ) {

to

document.querySelector('.primary-menu-container').addEventListener( 'click', function( event ) {

The function was being activated all the time, and that function should only work when the mobile menu is open. Now, you can use #anchors to move on the same page off the menu without “glitching” the menu icon.

That change is tested and working without any problems.

Attachments (1)

53976.patch (1.2 KB) - added by karthickmurugan 15 months ago.
Patch file for scroll issue when mobile menu closes

Download all attachments as: .zip

Change History (9)

#1 @sabernhardt
4 years ago

  • Keywords needs-patch added; has-patch removed

Thanks for the report!

Related: #53331

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


20 months ago

@karthickmurugan
15 months ago

Patch file for scroll issue when mobile menu closes

#3 @karthickmurugan
15 months ago

  • Keywords has-patch added; needs-patch removed
  • Version set to 6.7

Hello.,

I have created a patch for the second issue which is clicking on the mobile menu close button, scroll the site to top, if it is scrolled to the middle position. The first issue seems to be solved in this ticket 53331

#4 follow-up: @sabernhardt
15 months ago

  • Keywords needs-testing added
  • Version 6.7 deleted

(The issue occurred before 6.7, possibly as early as 5.6.)

#5 in reply to: ↑ 4 @karthickmurugan
15 months ago

Replying to sabernhardt:

(The issue occurred before 6.7, possibly as early as 5.6.)

Thanks @sabernhardt for updating it.

#6 @rinkalpagdar
15 months ago

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: https://core.trac.wordpress.org/attachment/ticket/53976/53976.patch

Environment

  • WordPress: 6.8-alpha-59274-src
  • PHP: 8.2.25
  • Server: nginx/1.27.2
  • Database: mysqli (Server: 8.0.40 / Client: mysqlnd 8.2.25)
  • Browser: Chrome 131.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-One 2.4
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Actual Results

  1. ✅ Issue resolved with patch.

#7 @ozgursar
4 weeks ago

  • Keywords reporter-feedback close added; needs-testing removed

Reproduction Report

Description

This report validates whether the issue can be reproduced.

Environment

  • WordPress: 7.0-alpha-61215-src
  • PHP: 8.2.29
  • Server: nginx/1.29.4
  • Database: mysqli (Server: 8.4.7 / Client: mysqlnd 8.2.29)
  • Browser: Chrome 144.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-One 2.7
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.1

Steps to Reproduce

  1. Install and activate Twenty Twenty-One
  2. Create a page and add some text or image as content
  3. Add an anchor link somewhere around the top of the page (e.g. pagetop)
  4. Create a back to top link at the bottom of the page or at the footer with anchor @pagetop
  5. View the page in frontend
  6. Open Chrome Dev Tools and narrow viewport to be able to see mobile menu
  7. Click the "back to top" link to trigger the anchor

Actual Results

  1. ❌ Error condition does not occur (unable to reproduce bug)

Additional Notes

#8 @sabernhardt
4 weeks ago

  • Keywords reporter-feedback close removed
  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #53331.

I'll close this. The icon issue should have been corrected as part of [55124].

The support topic also mentions an issue with scrolling if the site positions the header with fixed or sticky. I agree that it is better not to edit the theme for everyone so it would accommodate that special situation. (And in that case, changing the CSS to sticky might work better than editing the script.)

@media only screen and (max-width: 481.98px) {
	.lock-scrolling .site {
		position: sticky;
	}
}
Note: See TracTickets for help on using tickets.