Second level menu items can't be reached if they have children

Created on 12 October 2024, about 1 month ago

Problem/Motivation

We can't click on second level menu items if they have children.

Examples

Core: Structure > Display modes
The menu link behind Display modes can't be clicked (/admin/structure/display-modes) because on clicking on Display modes the submenu opens to reach Form/View modes pages. I looks like the link is not even rendered.

Contrib: Simple Blocks
The Simple Blocks menu item is a child of Structure > Block Layout. Thus the Block Layout page can't be reached anymore!

Contrib: Paragraphs
Paragraphs module adds the menu items Structure > Paragraphs > Add paragraph type, where the menu item Paragraphs leads to the paragraphs overview page which now can't be reached anymore.

Steps to reproduce

  1. Install drupal 11.x (via simplytestme)
  2. Login as admin
  3. Enable navigation
  4. Try to click on Structure > Display modes, which does not bring you to its menu item link /admin/structure/display-modes

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Active

Version

11.0 🔥

Component

navigation.module

Created by

🇩🇪Germany anruether Bonn

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @anruether
  • First commit to issue fork.
  • Merge request !10130Resolve #3480321 "Second level menu" → (Closed) created by scott_euser
  • 🇬🇧United Kingdom scott_euser

    Not sure if this is the right approach, but this splits the menu and and expand so you can use both.

    Screen recording:

  • Pipeline finished with Failed
    10 days ago
    Total: 542s
    #335101
  • 🇬🇧United Kingdom scott_euser

    Before:

    After:

  • 🇬🇧United Kingdom scott_euser

    scott_euser changed the visibility of the branch 3480321-second-level-menu to hidden.

  • Merge request !10134Resolve #3480321 "Second level menu access" → (Open) created by scott_euser
  • Pipeline finished with Failed
    10 days ago
    Total: 520s
    #335140
  • Pipeline finished with Failed
    10 days ago
    Total: 775s
    #335142
  • Pipeline finished with Success
    10 days ago
    Total: 860s
    #335144
  • 🇺🇸United States smustgrave
    1) Drupal\Tests\navigation\Kernel\NavigationMenuBlockTest::testHtmlMarkup
    //li[contains(@class,'toolbar-menu__item--level-1')]/span[contains(@class, 'toolbar-button') and text()='title 3']
    Failed asserting that 0 matches expected 1.
    /builds/issue/drupal-3480321/core/modules/navigation/tests/src/Kernel/NavigationMenuBlockTest.php:352
    

    Verified test coverage.

    Never seem grid-column used with a negative before, nice

    Only moving to NW for the issue summary, as it's missing proposed solution

    Since navigation is still experimental assuming don't need a CR for the twig change.

  • 🇬🇧United Kingdom scott_euser

    Thanks for the review! Updated the issue summary. On the -1 I can only pass credit on to stack overflow :) Front-end is definitely not my forte.

  • Test status - Pass
    Testing steps -

    - Install drupal 11.x
    - Login as admin
    - Enable navigation
    - Try to click on Structure > Display modes, which does not bring you to its menu item link /admin/structure/display-modes

    Patch applied successfully.
    Issue looks fixed.
    But, can see that the design is breaking for children menu titles.

    Please refer the gifs in attachments.

  • 🇬🇧United Kingdom scott_euser

    Looks like you need to clear your cache - both the twig template and css change. Thanks!

  • 🇮🇳India sagarmohite0031

    Hello scott_euser,

    Test status - Pass
    Patch applied successfully.
    Issue looks fixed.
    Testing steps -
    - Install Drupal 11.x
    - Login as admin
    - Enable navigation
    - Try to click on Configure > system,

    But, can see that the design is breaking for children menu titles after clearing cache.
    Check attachments

  • 🇬🇧United Kingdom scott_euser

    Thanks for checking; yes I can see the bug - I fixed it now + added more test coverage to validate.

  • 🇬🇧United Kingdom scott_euser

    Maybe this is tying loosely into 💬 Support Deeper Navigation Levels Active

  • 🇺🇸United States smustgrave

    #16 did you mean to push some changes up?

    Are #15 and #13 testing the same thing btw? Re-uploading additional screenshots or clips are not needed that are same or similar are usually not needed jsut an FYI.

  • 🇬🇧United Kingdom scott_euser

    Ah sorry, pushed to the old closed branch! Fixed now

  • 🇬🇧United Kingdom scott_euser

    Seems like random unrelated test failure (seeing that sometimes lately), rerunning

  • Pipeline finished with Success
    8 days ago
    Total: 4749s
    #337939
  • 🇺🇸United States smustgrave

    Removing summary update tag as it appears fine now

    1) Drupal\Tests\navigation\Kernel\NavigationMenuBlockTest::testHtmlMarkup
    //li[contains(@class,'toolbar-menu__item--level-1')]/span[contains(@class, 'toolbar-button') and text()='title 3']
    Failed asserting that 0 matches expected 1.
    /builds/issue/drupal-3480321/core/modules/navigation/tests/src/Kernel/NavigationMenuBlockTest.php:355
    FAILURES!
    Tests: 3, Assertions: 37, Failures: 1.
    Exiting with EXIT_CODE=1
    

    Shows the test coverage

    Manual testing appears to have been done in #13

    Code wise see nothing wrong

    LGTM

  • 🇷🇸Serbia finnsky

    Thank you for work here.

    1. We had something like this in initial versions of design. Then it was replaced with current variant. So i added required label.

    2. Visual regression in focus ring https://gyazo.com/5b8989e769ebf73709d00bf94f2e0004

    3. Keyboard navigation seems not worked as before. EG: Right and left arrow behaviour.

    4. Also small note in MR

  • Pipeline finished with Failed
    7 days ago
    Total: 162s
    #338277
  • 🇬🇧United Kingdom scott_euser

    1. Sorry I didn't quite follow, where should I add a required label? Or is that a process FYI thing?

    2. Fixed:

    3. This seems to work: if you tab to the down arrow and press right/left it opens closes. I am not sure it makes sense to have both the button and link both respond to the right/left arrow? If so, do you have any tips? I tried duplicating these bits from the down arrow:

    'aria-expanded': 'false',
    'data-toolbar-menu-trigger': menu_level,
    

    I might be a bit in over my head there if that is what you are expecting.

    4. Changed to SDC, thanks!

  • 🇬🇧United Kingdom scott_euser

    In any case thank you very much for the reviewing and tips!

  • 🇫🇮Finland lauriii Finland

    For context, it was decided earlier that access to the second level menus would be limited in the menu. This is due to the reason that Drupal Core uses those primarily to display the same information as what would be in the navigation but as a page. As a result, users would likely develop a pattern of not accessing those pages. The UX team did research to validate that the pages aren't considered to be useful.

    The tricky bit is that there are some edge cases where the second level menu is actually useful. This is not great from UX perspective, because if the user has developed a pattern of not accessing the second level pages, they would have challenges to find these pages. The original plan was to provide an alternative affordance for this use case, to make it easy to differentiate between the two use cases so that users know when to expect something useful.

    What is in the MR doesn't fulfill this. I think we need some direction from @ckrina on how we should handle this scenario.

  • 🇬🇧United Kingdom scott_euser

    I suppose it could be in contrib extending the template and overriding the css?

  • 🇬🇧United Kingdom scott_euser

    If in Core it sounds like maybe we need a mechanism either to opt in or opt out of having a direct access link. If opt in would need to update a lot of a contrib projects but at least that makes it a conscious decision. Perhaps a new attribute in menu link?

  • 🇫🇮Finland lauriii Finland

    Core needs to provide direction for how to fix the Simple Blocks and Paragraphs examples from the issue summary. This might lead into change in either Core or these modules.

    Ideally the fix for the Simple Blocks and Paragraphs would keep the Display mode use case intact because the Display mode example is as it is by design. This is where a contrib module could change the preference for the users that prefer a different behavior.

  • 🇫🇮Finland lauriii Finland

    If in Core it sounds like maybe we need a mechanism either to opt in or opt out of having a direct access link. If opt in would need to update a lot of a contrib projects but at least that makes it a conscious decision. Perhaps a new attribute in menu link?

    We could potentially automate that; the pages we don't want to display in the navigation are always rendered by \Drupal\system\Controller\SystemController. It's a special controller designed to render menus. We would only want to link from the menu when the controller is something else, e.g. \Drupal\block\Controller\BlockListController.

  • 🇬🇧United Kingdom scott_euser

    That sounds sensible. I would be happy to work on that but sounds like we should get ckrina's opinion first? If its a go for that route, would that be okay to add to this issue's scope?

  • 🇫🇮Finland lauriii Finland

    @ckrina is out until next Wednesday but we could reach out to her then. I have discussed this a while back with @ckrina on a high level.

    Here's the latest design I've seen for this:

    Essentially in this design there would be an Overview link that would allow user to navigate to the parent link. I don't think this a finalized design so we probably want to wait for input from here before we get too far on the implementation.

  • 🇬🇧United Kingdom scott_euser

    Okay thanks for talking me through it! Happy to help on implementation when we get to that stage (that does look simpler to achieve)

  • The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • 🇺🇸United States michaellander

    This feels like a duplicate of Add "All" or overview links to parent links Active , which feels like a duplicate of 🐛 Second level menu items can't be reached if they have children Active .

  • 🇨🇦Canada m4olivei Grimsby, ON

    Let’s leave this one open for the time being until we can get @ckrina's input per @lauriii's comments. As you pointed out @michaellander, this has popped up a few times, so it's a pain point for some. We should at least document the decisions made. 📌 Parent menu items with URLs are not linked to, do we make these available? Closed: outdated comes close to that, but it was from a time when the design was much less settled than it is today.

    I know there have been more recent design discussions on this topic. I'm attempting to reach out to some folks that worked on the designs to see if they can lend some insight into more recent discussions. Otherwise, I agree we should get @ckrina's input here before closing it out.

  • 🇬🇧United Kingdom scott_euser

    To note, pages that list other things can still be useful, like /admin/structure/views is quite essential as many sites will have too many views to list in the menu + there is plenty of other useful information in that table that can help site builders choose which view edit that they lose out on if they stop being able to access it (fully aware that hiding it does not 'stop' them accessing it but certainly makes it hard to get to). I believe that would also be solved if we follow @laurii's suggestion in #30.

  • 🇫🇮Finland lauriii Finland

    I'm removing the Display Modes example from the issue summary since that is working by design. The remaining use cases are valid and should be addressed one way or another, either in core or by guiding how contrib modules should approach this.

  • 🇨🇦Canada m4olivei Grimsby, ON

    Had a chance to catch up with @ckrina today. The design that @laurii referenced in #30 is indeed the latest iteration of the design to accommodate this issue. Let’s update the MR and/or close the current MR and open a new one for this approach.

    Here is a representative screenshot:

    Here is a link to the Figma: https://www.figma.com/design/VCPAxetieAC2pzw7hgX3ij/Drupal-Admin-UI---De...

    A couple of caveats to keep in mind:

    • "Overview" is a working title for this link. Seeking some UX input on that. Other considerations might be "See all".
    • The Figma designs show a fourth level ("Great Grandchild Item"). This is the subject of 💬 Support Deeper Navigation Levels Active and is out of scope here.

    I've also updated the issue description to match.

Production build 0.71.5 2024