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

Created on 12 October 2024, 9 months 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
    8 months 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
    8 months ago
    Total: 520s
    #335140
  • Pipeline finished with Failed
    8 months ago
    Total: 775s
    #335142
  • Pipeline finished with Success
    8 months 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 months 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
    8 months 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.

  • 🇨🇦Canada m4olivei Grimsby, ON
  • 🇬🇧United Kingdom scott_euser

    Thanks for the updates!

    Not a hill I'm willing to die on but FWIW...
    I don't quite agree and the UX research backs that up:

    1. e.g. version 2 (current MR) lower rate of failure, lower time to complete task
    2. A less clear UX article on it from NN hints at this too in item 12, but the separation only becomes clear when you actually visit the example they point to)

    But I am fully aware I am late to the party and decisions have been made, so fully happy for you to ignore this; just saying my piece :)

    Next steps
    Before this gets work on I have a few questions:

    1. From #30: . Actually thinking about this further:
      1. There is value in consistency particularly if it won't be obvious to the end user why some show overview and some not
      2. There is value in the overviews: some people actually might rely on the overview to understand what the options are in an area; ie, the descriptions do have value (or otherwise why are we providing descriptions in the first place). If we sometimes skip the overview we make that value inaccessible

      So would be good to agree on whether we add that to scope or not (my vote is not FWIW)
       

    2. Do we just output 'Overview' and link to the parent, overriding its label effectively? Or do we make it controllable; override the menu link in some sort of pre-processing of the navigation menu links, but still allowing modules to override it back in case 'Overview' as a label is inappropriate (e.g., what if its not providing an overview?)

    Then from there we can update issue summary and get to work on it (which I am happy to help with).

  • 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland

    I was also around for some of those early discussions and so was going to say something similar to what @lauriii said in comment #26, so I'll try not to repeat, but I still support the decisions that were made around thsi.

    Paragraphs is an interesting example, because what it does is non-standard, there are no example in core of these kind of "add" links appearing in the menu in this way, neither are there examples of all the different "types" appearing in the menu. Similarly, admin_toolbar adding links for all the Views a site has is also very non-standard. In both of these examples, if you have a lot of Paragraph types or a lot of Views, they don't all appear in the menu, so you end up going to the full page anyway. I think it's really important that we have consistent patterns for how things should be done.

    I also still agree that in the vast majority of cases the Overview links are not useful, therefor adding them results in a net-negative user experience because it adds more visual noise. For screen reader users, having a lot of links that all say "Overview" is really unhelpful and could be confusing. Not just because they have to hear the word "Overview" every time the user opens a sub-menu, but also because if a screen reader user asks for a list of all links on the page, having a bunch of links that read simply "Overview" or "Add" without any other context could be very confusing.

    So my feeling is that we shouldn't encourage a pattern which results in all of these generally redundant "Overview" links.

    I think we can probably find a pattern that addresses what Paragraphs, Views and Admin toolbar are attempting to do, but right now I don't know what that pattern should be.

  • 🇨🇦Canada joelpittet Vancouver

    The split button proposed by @scott_euser in #6 🐛 Second level menu items can't be reached if they have children Active seems like the most intuitive direction here. I also share the concerns raised about the additional ‘Overview’ injected links.

    @ckina and @lauriii could you give this a second look?

    This affects a number of modules I have co-maintainership of such as:

    And simple_block as mentioned previously and core's Display modes all under Structure I might add. Overview seems to make sense for Configuration at a glance but not Structure. Though I believe the Split button approach is more versatile.

  • 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland

    The issue with split buttons, in the context of menu items, is that it introduces different interactions patterns depending on which part of the menu item the user interacts with. It can be challenging to communicate those different interactions to users and complicates the interface. These can also be challenging to use on touch devices, for users who struggle with precise mouse clicks/taps,

    There's a really good article from Nielsen Norman Group that goes into much more detail as to why these should not be used for navigation:

    Don’t Use Split Buttons for Navigation Menus - nngroup.com

    That article also proposes some alternative option, some of which we've discussed here, like providing a link to the "landing page" inside the sub-menu, which is perfectly fine; But as we've discussed if we do this for all sub-menus, in the vast majority of cases those "Overview" links are entirely redundant.

    Ultimately I don't think we're going to find a good technical solution here, and that's because this is not a technical problem, it's an information architecture problem.

    In cases where the "Overview" page is relevant, because maybe that's the content type list or similar, we should simply provide that as a link in the sub-menu, and we should call that link something like "All content types". Or ultimately reorganize the entire admin menu (but that's probably out of scope here 😀). What we shouldn't do is add a lot of repeating links which use the label "Overview".

    I think when we treat this as a problem of information architecture, and not a technical problem, we'll end up with a much better user experience.

  • 🇨🇦Canada joelpittet Vancouver

    Yes, “reorganizing the entire admin menu” is definitely out of scope! 😄 The issue largely stems from contrib modules adding their own admin pages under existing ones or creating nested subpages (but core too as screenshoted in #44 🐛 Second level menu items can't be reached if they have children Active ). Addressing this by fixing contrib would likely require more effort and education than working with the existing patterns commonly used.

    The article raises valid concerns about split buttons in navigation, it might overstate their universal unsuitability. Instead of relying solely on these arguments, we should test those concerns ourselves, perhaps against @scott_euser propposal in #6 🐛 Second level menu items can't be reached if they have children Active , to see how they hold up in practice. A balanced approach that evaluates context, user needs, and new design tricks might reveal a more nuanced view of this pattern. Safe enough to try?

  • Status changed to Needs work 7 months ago
  • 🇺🇸United States michaellander

    We've been using the 'Overview' approach patch from Allow modules to hook into top of content/footer sections of new core navigation Active as a stop gap because users were getting so frustrated they couldn't access the 2nd level when a third level item was added. In our particular example it was a combination of the commerce and scheduler that was making it impossible to access the commerce dashboard.

    In this particular use case I don't believe commerce should define a submenu link to 'Products' because as far as it's aware, it has no other children. However, we also can't put the onus on 'scheduler' because any number of modules could extend commerce in ways that they feel warrant adding children to the 'Products' menu item and then you have an issue with many modules determining how to keep from breaking the 2nd level 'Products' link.

    I'll be honest, so long as we treat the dynamic between the 1st and 2nd level different from the 2nd and 3rd level, where the 1st level is linked at the top of 2nd level, but 2nd level is not accessible at all if 3rd level exists, I believe we'll always have this confusion. I think this also includes hiding/showing behavior as related to \Drupal\system\Controller\SystemController. Perhaps an additional property could be added for Menu links to define an 'Override' title when children exist, but again that seems like something that should be consistent between the 1st and 2nd levels as well.

    It's for these reasons that I think the split button is most elegant, but I'd even take the 'Overview' solution over no solution.

  • 🇩🇪Germany anruether Bonn

    > In this particular use case I don't believe commerce should define a submenu link below 'Products' because as far as it's aware, it has no other children. However, we also can't put the onus on 'scheduler' because any number of modules could extend commerce in ways that they feel warrant adding children to the 'Products' menu item and then you have an issue with many modules determining how to keep from breaking the 2nd level 'Products' link.

    Looks like this would also be the case with the first example from the IS, where Simple Block is the only child of

    Block layout

    .

  • Merge request !10900Resolve #3480321 "Add overview" → (Open) created by scott_euser
  • 🇬🇧United Kingdom scott_euser

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

  • 🇬🇧United Kingdom scott_euser

    I think we are at an impasse and better not to let perfect be the enemy of good. Overview is certainly a million times better than inaccessible. Adding an MR + test coverage with overview in place.

    This is a guess as I believe my questions in #42 have not been answered. My concern with this approach is that contrib modules cannot change 'Overview' to something else when 'Overview' is not appropriate. In which case instead of adding via Twig, it may be better to modify NavigationMenuBlock::build() and loop through the tree to add in the additional links. Then contrib would be able to change 'Overview' to something else. I suppose that could be a follow-up (again not to let perfect be enemy of good)?

  • Pipeline finished with Success
    6 months ago
    Total: 1900s
    #395463
  • 🇬🇧United Kingdom scott_euser

    After MR, default drupal 11 install + enable Navigation module.

  • 🇬🇧United Kingdom scott_euser

    Updated issue summary

  • Pipeline finished with Success
    6 months ago
    Total: 2440s
    #395719
  • 🇬🇧United Kingdom scott_euser

    Thanks for the review, resolved the feedback, ready for review again. Thanks!

  • 🇬🇷Greece arx-e

    Works fine for me! Resolves a very frustrating issue that I couldn't escape from since I upgraded to Gin Afdmin theme 4.x

  • 🇫🇮Finland Laurie Lim Helsinki

    Reviewed the code change and installed the patch, resolves the main issue neatly, which is making the 2nd level menu accessible. I agree with Scott that we shouldn't let "perfect be the enemy of good here" and there are certainly ways that it can be improved but the current state is just frustrating and this serves well for core menu items.

  • 🇫🇷France Grimreaper France 🇫🇷

    Hi,

    Thanks for the issue and the work done on it.

    I confirm too, that in current state at least it removes blocker of the current state.

    Attaching patch from MR for Composer usage.

  • Status changed to RTBC 5 months ago
  • 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland

    I'd say Overview is probably the lesser of two evils. On the one hand its not ideal, but on the other hand not being able to get to actually useful pages is also not ideal, and is arguably much worse from a UX perspective.

    I also wondered if maybe we could do something clever and auto detect if a parent item is a "useful" page, but that requires us to define what a "useful" page is, and that's probably an unsolvable task given the variety of different implementation of "useful" pages in contrib and custom. That in itself would also not be great because it would introduce an inconsistent pattern where sometimes there's an "overview" link and sometimes there isn't.

  • 🇨🇭Switzerland berdir Switzerland

    > I also wondered if maybe we could do something clever and auto detect if a parent item is a "useful" page, but that requires us to define what a "useful" page is, and that's probably an unsolvable task given the variety of different implementation of "useful" pages in contrib and custom

    I think it's not that unsolvable. We can treat it as an ignore list and only list things we know are save to exclude. On top of my mind, there's one obvious candidate, and that is the controller being \Drupal\system\Controller\SystemController::systemAdminMenuBlockPage. That should cover all the config overview pages that just list the links below. I don't know how readily that information is available where we need it, we'd need to fetch the route object I guess.

    Also interested in this being resolved, I've already been getting bug reports about this in contrib projects, so I don't want to hold this up, could be a follow-up to optimize it?

  • 🇪🇸Spain ckrina Barcelona

    First, thank you all for pushing to solve this and all the work that happened so far.

    Agreed with @aaronmchale and @berdir here, which is the main follow-up planned after removing the link from the title+dropdown because of the UX issues it was causing. I can't find the issue so I guess we forgot to open it with all the work happening the months we were sprinting to get Navigation into core.

    How to determine what gets a "see all / see the parent/whatever" is what needs to be defined. I don't have a strong opinion on that and I'm sure we can come out with a good strategy if we can research patterns/needs. What I do know for sure is that adding an "Overview" to all sub-menus is not the right strategy because it creates a worst experience: it's adding extra links that in most cases will only get you to a page with the exact same links you already have in the menu (old admin list pages).

    So I'm moving this to Needs works to define out the best strategy to print those "Overview" links only when they are really needed.

    Some of the first ideas we had was to print anything that wasn't an admin list page, which is what @berdir is already suggesting in #59. I'd be happy to ship only with that (I understand you all need this) and we make incremental improvements later.

    PD. @joelpittet the results of the tests for the navigation can be found in 🌱 [PLAN] Usability Testing and Research on the Toolbar Active , the ones specifically about the splitbutton pattern in 🌱 Toolbar Prototype Usability Testing Phase I Fixed .

  • Pipeline finished with Failed
    5 months ago
    Total: 160s
    #434332
  • 🇬🇧United Kingdom scott_euser

    Okay I believe this should do it, but needs new tests again I assume:

    With new MR, no Overview in Display Modes:

    But yes Overview in Paragraphs:

    I attempted to add tests myself but could use a hand - getting the mocking working to use the navigation menu tree and give access to the route name is a bit of a learning curve for me. Wondering whether it would be okay to make a functional test out of it and leverage menu_test test module from system as that appears to already have a good mix of systemAdminMenuBlockPage. Otherwise worried this gets stuck for lack of tests and continues to be a problem in contrib.

  • Pipeline finished with Failed
    5 months ago
    Total: 232s
    #434342
  • Pipeline finished with Success
    5 months ago
    Total: 456s
    #434399
  • First commit to issue fork.
  • Pipeline finished with Failed
    4 months ago
    Total: 587s
    #446604
  • Status changed to Needs work about 2 months ago
  • 🇬🇧United Kingdom catch

    Wondering whether it would be okay to make a functional test out of it and leverage menu_test test module from system as that appears to already have a good mix of systemAdminMenuBlockPage

    A functional test seems fine here, or if it's just the links that are needed a kernel test might be enough?

  • 🇦🇺Australia pameeela

    Using this patch on a project and it works quite well! Would be great to get it in before 11.2.

  • First commit to issue fork.
  • Merge request !12148Resolve #3480321 "Overview menu manipulator" → (Closed) created by godotislate
  • Pipeline finished with Success
    about 2 months ago
    Total: 609s
    #499271
  • Pipeline finished with Success
    about 2 months ago
    Total: 619s
    #499280
  • Pushed up MR 12148.

    Based this on work done in the previous MR 10900, but I decided to create a new menu tree manipulator to inject the overview links instead. This provides a lot of different ways for contrib or custom modules to override this new behavior, whether by altering/decorating the new manipulator service, implementing hook_navigation_menu_link_tree_alter(), or altering the block render array.

    Also, since there were at least a couple comments expressing concern about multiple links titled "Overview", I changed it so that the new child link's title is the same as the parent's, except with " overview" appended. I guess that will be awkward if someone titles a menu link "Something overview", so the child link title becomes "Something overview overview", but I think that is an edge case and the link can be altered as mentioned above.

    Added a unit test for the new manipulator service. I think the unit test class has pretty exhaustive coverage, so while I did also update the navigation block kernel test as well, I added fewer cases there because I think the coverage would be mostly duplicative.

    This should ready for review again. If the other approach is preferred, we can probably grab the kernel test changes in MR 12148 and add them to 10900.

  • Pipeline finished with Success
    about 2 months ago
    Total: 539s
    #499308
  • Pipeline finished with Success
    about 2 months ago
    Total: 626s
    #499917
  • Pipeline finished with Success
    about 2 months ago
    Total: 779s
    #499948
  • 🇬🇧United Kingdom marcelovani London

    Any updates on this?

  • Personally, I'd prefer if the menu item containing children could itself be clicked with ctrl+click or middle-click in order to open its link in a new window. That's how admin_toolbar works. You hover to open the submenu, and you can click to open its page.

  • I would consider the solution proposed in Navigating to first level menu items is not obvious Active to be the best approach to solving the problem, because it solves both the issue there and this issue.

  • 🇳🇱Netherlands rolf van de krol

    Rolled MR 10900 into a patch against 11.1.x

  • 🇦🇺Australia pameeela

    Personally I prefer the original approach, adding just 'Overview' is simpler and cleaner and avoids repetition. Screenshots below showing the difference between these. I've asked @ckrina to weigh in so we can have a final decision and ideally get this committed soon.

    MR 10900:

    MR 12148:

  • The wording of the menu link is a trivial change to either MR whichever way, and I'm not strongly opinionated either way.

    That said, my one small pushback to just using "Overview" is that it can be considered an accessibility issue on a page for there to be multiple links with the same text that go to different URLs.

  • 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland

    Personally, I'd prefer if the menu item containing children could itself be clicked with ctrl+click or middle-click in order to open its link in a new window. That's how admin_toolbar works. You hover to open the submenu, and you can click to open its page

    I believe (from memory) this was discussed earlier in this issue and we agreed that it wouldn't be ideal as there's some accessibility issue around that approach.

  • 🇪🇸Spain ckrina Barcelona

    Personally, I'd prefer if the menu item containing children could itself be clicked with ctrl+click or middle-click in order to open its link in a new window. That's how admin_toolbar works. You hover to open the submenu, and you can click to open its page

    This is not an standard pattern, and we don't want users to have to learn custom patterns. We already did user tests that proved that the item containing children has to perform the action of opening the children menu, and adding other options confused the users (even if they knew they could do it).

    Thanks @pameeela for adding the screenshots! For the rest, please remember that issues with UI changes need screenshots to be evaluated (and you're adding a new approach like in here) :)

    Also, since there were at least a couple comments expressing concern about multiple links titled "Overview", I changed it so that the new child link's title is the same as the parent's, except with " overview" appended.

    Let's keep the initial approach from MR 10900 that uses the Overview text for all the original reasons: repetitive pattern that can be remembered, shorter solution to avoid longer lines in a small space (remember translations: just in Spanish it's "Descripción general"). If people really really have a big concern with this we can add it to the new round of user tests.

    I decided to create a new menu tree manipulator to inject the overview links instead. This provides a lot of different ways for contrib or custom modules to override this new behavior, whether by altering/decorating the new manipulator service, implementing hook_navigation_menu_link_tree_alter(), or altering the block render array.

    Thanks for the thoughtful work on this! I understand you've built in flexibility, but before moving forward, I'd love to understand the problem this is solving. What specific product need or requirement drove the decision to add this new menu tree manipulator? Since this was flagged as a stable blocker to get Navigation into Stable (adding the missing tag), we need to make sure we're addressing a real pain point rather than adding complexity in the name of future flexibility. Is there any specific need you've detected in a project? Understanding the 'why' will help us evaluate whether this approach is the right fit, or if we're fine with the previous path to achieve the same goal.

  • Pipeline finished with Success
    about 1 month ago
    Total: 544s
    #513586
  • Pipeline finished with Failed
    about 1 month ago
    Total: 106s
    #513598
  • we need to make sure we're addressing a real pain point rather than adding complexity in the name of future flexibility. Is there any specific need you've detected in a project?

    I'm not sure it's that much more complex. The code to add the links is essentially just moved to live in its own place. There's a bonus that it's also unit testable.

    As for flexibility, I don't know about specific needs, other than some comments in this issue talking about there being various contrib/custom solutions to this problem already in place, so the flexibility is to try to accommodate whatever those may be.

    Meanwhile, I have copied the kernel test changes from 12148 to 10900. Unfortunately, there's a test failure, so some investigation is needed whether there's an issue with the test applied to this MR, or if there's something in the original code changes that aren't covered. I'm in the middle of traveling, but I can try to look at what's going on later.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 701s
    #513606
  • 🇪🇸Spain plopesc Valladolid

    Checked and tested locally code in MR 12148 and it looks good to me.

    The manipulator approach adds useful flexibility for the future without significantly impacting the current codebase — I think it's a solid direction.

    I’ve added a couple of comments to the MR, but this is very close to being ready!

  • Pipeline finished with Success
    about 1 month ago
    Total: 1473s
    #514314
  • Pipeline finished with Success
    about 1 month ago
    Total: 405s
    #514374
  • Pipeline finished with Success
    about 1 month ago
    Total: 453s
    #514428
  • Pipeline finished with Success
    about 1 month ago
    Total: 489s
    #514432
  • I have updated MR 12148 based on MR feedback. It's ready for review again.

    Separate note: My assumption is that this has missed 11.2.0. In case that it's still possible to get in, and if adding a menu manipualtor service is a blocker to that at this point, I have resolved the test issues on MR 10900 and gotten to functional parity on the front end with the latest changes in MR 12148.

  • 🇦🇺Australia pameeela

    That's awesome, thanks @godotislate! @catch said this can probably get committed during RC since the module is not stable yet.

  • 🇪🇸Spain plopesc Valladolid

    Feedback has been addressed and the new 3rd level item functionality works according to previous comments.

    I would say this can be marked as RTBC now.

  • 🇺🇸United States xjm

    Since Navigation is still an experimental module, and since this is resolving a major, user-facing bug, this is eligible for backport during RC (or even a patch release). The beta experimental phase is precisely for fixing user-facing issues like this. The small risk of the API addition of the service and new methods in an experimental module is outweighed by the benefit of resolving this issue for users.

    Crediting @catch who also +1ed this as an RM signoff for the backport in Slack.

  • godotislate changed the visibility of the branch 3480321-add-overview to hidden.

  • Pipeline finished with Canceled
    about 1 month ago
    Total: 93s
    #515039
  • Pipeline finished with Failed
    about 1 month ago
    Total: 229s
    #515041
  • 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦

    This patch is for Drupal 10.4.8, based on merge request !10900.

  • 🇬🇧United Kingdom catch

    While this is a bug fix, there's a nice user-facing change that we could potentially put into the 11.2.0 announcement so tagging just in case.

    • catch committed 7a689709 on 11.2.x
      Issue #3480321 by scott_euser, godotislate, catch, grimreaper, kensae,...
    • catch committed 25f4ab84 on 11.x
      Issue #3480321 by scott_euser, godotislate, catch, grimreaper, kensae,...
  • 🇬🇧United Kingdom catch

    fwiw while I don't find menu tree manipulators particularly intuitive, it does help to avoid a long 'alter method of doom' that's trying to handle too many different things, and given I think there's a non-zero chance this approach could be revisited again (e.g. if we ever move away from SystemAdminMenuBlockPage as a way to build the admin structure, or if a new design approach appears), it's good to have it encapsulated.

    Did my best with issue credit but this is a long issue with lots of contributors so it may not be perfect.

    Committed/pushed to 11.x and 11.2.x, thanks!

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024