WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 9 months ago

Last modified 9 months ago

#35272 closed defect (bug) (fixed)

.menu-item-home class not applying to static front pages

Reported by: RachieVee Owned by: pento
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.4
Component: Menus Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

The wp_nav_menu is only applying the .menu-item-home class to menu items when the home url is created with a custom link on Appearance > Menus. The Codex and Code Reference mention this class should be added to menu items "that correspond to the site front page."

https://developer.wordpress.org/reference/functions/wp_nav_menu/

When there is a static page set as a front page from Settings > Reading Settings, and then added as a link in Appearance > Menus from the "Pages" metabox, the class is not applied to the menu item. So either the descriptions in documentation can be adjusted or the conditionals/if statements in wp-includes/nav-menu-template.php can be adjusted so this class applies to static page links without having to be custom links.

Attachments (7)

Screen Shot 2015-12-30 at 11.35.46 PM.png (147.3 KB) - added by RachieVee 19 months ago.
Screenshot of missing menu-item-home class from static page menu item
35272.diff (557 bytes) - added by christophherr 15 months ago.
Adds the class .menu-item-home to a page set as front page when it is added to a navigation menu through the "Pages" metabox in Apperance - Menus.
35272-2.diff (605 bytes) - added by christophherr 11 months ago.
Original patch didn´t apply. Added a refresh.
35272.2.diff (1.3 KB) - added by adamsilverstein 9 months ago.
35272.3.diff (1.3 KB) - added by adamsilverstein 9 months ago.
35272.4.diff (2.3 KB) - added by welcher 9 months ago.
Adds unit tests and small fix
35272.5.diff (2.9 KB) - added by welcher 9 months ago.
adding a no class applied unit test.

Download all attachments as: .zip

Change History (33)

@RachieVee
19 months ago

Screenshot of missing menu-item-home class from static page menu item

#1 @SergeyBiryukov
16 months ago

  • Keywords needs-patch added

@christophherr
15 months ago

Adds the class .menu-item-home to a page set as front page when it is added to a navigation menu through the "Pages" metabox in Apperance - Menus.

#2 @christophherr
15 months ago

  • Keywords has-patch added; needs-patch removed

@christophherr
11 months ago

Original patch didn´t apply. Added a refresh.

#3 @welcher
10 months ago

  • Keywords dev-feedback added

Looks good to me. Not sure what else we'd need to do on this.

#4 @welcher
9 months ago

  • Milestone changed from Awaiting Review to 4.7

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


9 months ago

#6 @welcher
9 months ago

  • Owner set to welcher
  • Status changed from new to assigned

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


9 months ago

#8 @welcher
9 months ago

  • Keywords commit added; dev-feedback removed

#9 @pento
9 months ago

  • Owner changed from welcher to pento

#10 @pento
9 months ago

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

In 38882:

Menus: Add the menu-item-home class to the static front page.

When a site is using a static front page, and that page is in a menu, it isn't given the CSS class menu-item-home, contrary to the developer documentation.

Props christophherr.
Fixes #35272.

#11 follow-up: @mdgl
9 months ago

  • Keywords needs-unit-tests added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Pardon me if I'm being stupid, but how is this patch supposed to work? The value being tested in the if statement looks just like a constant string to me. When I apply the patch, I get the class menu-item-home added on current taxonomy menu items and current but non-home pages as well as on the front page. I think a new patch and some unit tests are needed, see #32367.

#12 in reply to: ↑ 11 @adamsilverstein
9 months ago

Replying to mdgl:

Pardon me if I'm being stupid, but how is this patch supposed to work? The value being tested in the if statement looks just like a constant string to me.

@mdgl I think you are right, this conditional wasn’t doing anything. When I tested it actually seemed to work, I think the reason the patch mostly worked is that by the time logic reached this point only the home page was left?

In 35272.2.diff I added an actual check for the object id set as the 'front page'. When the ids match, the calss is added. Some unit tests here to verify behavior would be helpful.

#13 follow-up: @christophherr
9 months ago

I did not see the class added to other current pages / taxonomies but that only proves your point about unit tests @mdgl.

@adamsilverstein's patch looks much better and more robust.
Following the format of the other checks, may I suggest to cast the get_option check to (int)?

$front_page_id  = (int) get_option( 'page_on_front' );

I'd need some pointers to get started on unit tests for menu classes / items.
(Seems rather daunting after reading 32367)

#14 in reply to: ↑ 13 @adamsilverstein
9 months ago

Replying to christophherr:

Following the format of the other checks, may I suggest to cast the get_option check to (int)?

Yes, I agree we should cast as int here to match the rest of code and for safety. I had originally cast as int and removed when I discovered strict equality worked without it.

I'd need some pointers to get started on unit tests for menu classes / items.
(Seems rather daunting after reading 32367)

I have never tried writing these, might be worth asking @johnbillion how far he got on building out tests on #32367 or looking at existing core unit tests for menus.

#15 @adamsilverstein
9 months ago

35272.3.diff:

  • Cast page_on_front option as int

#16 @pento
9 months ago

  • Keywords commit removed

Ugh, that was my bad, I don't know how I committed that without noticing it.

Until #32367 happens, unit tests can probably live in post/nav-menu.php.

#17 @mdgl
9 months ago

The new patch looks better to me and from my limited testing it appears to work correctly. Are we sure that there are no additional conditions required to ensure that the comparison with front_page_id remains valid in all circumstances? I would guess so but can't be completely confident.

In general, I still find the overall logic in function _wp_menu_item_classes_by_context() very confusing and wouldn't be surprised if there were a few more bugs lurking in this area :-)

#18 @welcher
9 months ago

First, sorry for adding the commit keyword on this. I tested it, but clearly didn't review the code.

To (hopefully) redeem myself, I have added some unit tests and a small fix to @adamsilverstein's patch that came out of the tests.

@welcher
9 months ago

Adds unit tests and small fix

#19 @welcher
9 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#20 @adamsilverstein
9 months ago

  • Keywords commit added

Nice work, thanks @welcher!

#21 @christophherr
9 months ago

Thank you everybody for cleaning up my mess.
I apologize for trying a shortcut that seemed to work.

The unit test also passes with my patch which seems rather strange.
Would that be an indication that there is more to discover with greater unit test coverage for the nav menus in general?

#22 @mdgl
9 months ago

Yes! I suspect the unit tests should also include at least one negative case: a menu entry referring to a page that is not marked "on front" should not contain the menu-item-home class.

I think this is why John Blackbourn got a bit fed up with #32367 as to create good coverage here the number of test cases required is actually quite large when you include all possible classes, object types and hierarchic combinations. He did say, however that he was "nearly there" over a year ago!

@welcher
9 months ago

adding a no class applied unit test.

#23 @pento
9 months ago

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

In 38940:

Menus: Add the menu-item-home class to the static front page item.

When a site is using a static front page, and that page is in a menu, it isn't given the CSS class menu-item-home, contrary to the developer documentation.

An incorrect solution was originally added in [35272], and is now gone. Let us never speak of it again.

Props mdgl, adamsilverstein, welcher, pento.
Fixes #35272.

#24 @pento
9 months ago

In 38945:

Tests: Fix some query typos introduced in [38940].

Props piewp for the catch.
See #35272.

#25 follow-up: @mdgl
9 months ago

I think this is fine now and I don't want to reopen the ticket, but for completeness (and future work on #32367) I just wanted to raise a question about the unit tests, which I am not sure are strong enough to distinguish between the original (incorrect) patch and the final version.

I'm afraid I don't know how to run the unit tests, so this is just based on inspection.

The classes added by function _wp_menu_item_classes_by_context() are dependent on the value of the current query (i.e. the page being displayed), but this does not appear to be catered for by the tests.

For example, if I create a menu with two page items: "Front" and "Back" and set my home page to "Front", I see the following behaviour with the original patch:

Front Menu Item Back Menu Item
Front Page Displayed menu-item-home -
Back Page Displayed - menu-item-home

That is, the class menu-item-home is always and incorrectly added to the menu item corresponding to the currently displayed page, even if it is not actually the home page. With the final version of the patch, I see the correct behaviour as follows:

Front Menu Item Back Menu Item
Front Page Displayed menu-item-home -
Back Page Displayed menu-item-home -

Could somebody tell me whether the unit tests are able to pick up the difference between these two solutions? To my eyes, there doesn't appear to be quite enough code to do that.

#26 in reply to: ↑ 25 @welcher
9 months ago

Replying to mdgl:

I'm afraid I don't know how to run the unit tests, so this is just based on inspection.

Have a look at https://make.wordpress.org/core/handbook/testing/automated-testing/phpunit/ or http://ryanwelcher.com/2015/07/setting-up-unit-testing-for-wordpress-core/ to help you get setup.

Could somebody tell me whether the unit tests are able to pick up the difference between these two solutions? To my eyes, there doesn't appear to be quite enough code to do that.

The specific test of two menu items and one is set to the front page is not in place. The current tests do not check for that. However, there is a test that tests for a page that is not assigned to the home page and another that tests for one that is. It would not be a large lift to add a third to cover off your scenario

Note: See TracTickets for help on using tickets.