The secondary links heading, "Secondary menu", is wrong

I think you're right. Please, please, I'm hoping that all menus will be blocks in D8 and we can get rid of these.

+1. I support this title change. The patch had some unrelated .htaccess changes, but otherwise the approach looks good.

Regarding translation, my philosophy is that user-provided content is not subject to translation with t(). We simply output user-entered content in the language in which it was entered. Right?

Hope this patch passes the bot. I edited it to cut out the .htaccess info so hopefully it applies.

bot?

Thanks for prodding the bot yoroy. Hopefully this works better this time.

Marked #878772: Primary and Secondary links headings are hard coded as duplicate, as this issue has some code.

Issue summary from #878772: Primary and Secondary links headings are hard coded:

In D7 we added H2 headings to theme_links because "Headings should be used on navigation menus and any list of links that consistently appears on multiple
pages."
1.

At the moment the headings for the Main and Secondary menus are hard coded which is OK if those are in fact what you require, if not then its less than
idea - the heading should be the name of the source menu so it has meaning in the context of the users site.

And comment from David_Rothstein

Hm. Looking into this more, I'm not so convinced we want to change those headings to be dynamic after all.

Primary/secondary links are just a list of links. They are derived from "skimming" the top set of links off of a menu, but they are not themselves a complete
menu. And currently (unlike when a menu is displayed in a block) we never display the menu title to anyone within the primary/secondary links.

So if we start making it display to screen readers, that could have unexpected results. For example, the menu title might be something that only makes sense
to administrators and that they never intended or expected an end user to see (!).

The way it is now is indeed confusing, but I think that's just a duplicate of
#698014: Theme settings for main/secondary variables mismatch with menu settings With that issue, we would switch to having the headings say something like "main links" and "secondary links" (rather than "main menu" and "secondary menu")
which would accurately describe to screen readers what they are and their relation to the overall site. Currently, the headings are very confusing, but
correct - because Drupal 7 is using the phrases "main menu" and "secondary menu" to refer to these links in the admin interface also. I don't think we
can fix them in isolation.

And comment from David_Rothstein

...And currently (unlike when a menu is displayed in a block) we never display the menu title to anyone within the primary/secondary links.

Do you (and David) mean the title of a source menu from which those links are taken, or do you mean something else? There are titles at least in Stark, and those titles are (hard-coded) "Main menu" and "Secondary menu".

Related: #945654: Default menus have hardcoded titles which cannot be edited

Re-rolling the patch.

#14: 1079010-secondary-menu-heading-13.patch queued for re-testing.

Re-roll.

#22: secondary_heading.patch queued for re-testing.

fix search module tests

#26: secondary_heading.patch queued for re-testing.

Re-roll to account for template location change.

tagging.

#29: secondary_menu_heading-1079010-29.patch queued for re-testing.

Hmm... looks like the user-menu menu has been renamed to account.

Also fixing a missing space.

Re-rolling.

#33: secondary_links_heading-1079010-33.patch queued for re-testing.

Looks good. It's finally showing the proper menu:
<h2 class="element-invisible">User account menu</h2>

I think we should close this issue as won't fix or duplicate of #1869476: Convert global menus (primary links, secondary links) into blocks - which apparently removes this custom primary/secondary menu links markup generation entirely.

Also, I think the issue I raised that is linked to in #11 is still relevant here; why should we start exposing a potentially admin-facing menu name to end users of the site?

The only extra information that a sighted user has here which a screen reader user does not is that the visual cues on the page indicate this menu is of secondary importance in the navigation structure.... they do not have any information about the stored title. Therefore, I still think "Secondary X" should always be an appropriate screen reader label for it. (As mentioned earlier, though, "menu" may not really be an appropriate word for the X.)

I'm not opposed to that. However, what happens if that patch doesn't get in?

#33: secondary_links_heading-1079010-33.patch queued for re-testing.

#33: secondary_links_heading-1079010-33.patch queued for re-testing.

#33: secondary_links_heading-1079010-33.patch queued for re-testing.

Ok, this needs to be re-written in twig now.

What happens to the code in core/includes/theme.inc

Gotta wrap my head around this new templating system.

The old patch did not apply anymore so I don't have an interdiff.

Here are some notable changes:

• The "Block test theme" test is now at core/modules/block/tests/themes/block_test_theme/block_test_theme.info.yml, but it doesn't seem to exist anymore? Certainly it doesn't have it's own template file.
• The SearchConfigSettingsFormTest doesn't need to be changed anymore.
• The call to the theme('links'... function previously in the page templates is now done at the bottom of template_preprocess_page(), so no changes needed to be made to the page template files.

I've manually tested this patch by changing which menu is currently selected as the "secondary menu" at admin/structure/menu/settings and changing the name of the currently-selected secondary menu, and confirmed that the markup changes correctly.

I did not see an automated test in the previous patch for this feature: does anyone know if there is one? I can add one if not.

Whoops.

Rats, had unstaged changes. The patch from #45 probably won't work; but the notes there are still valid. Try again.

#47: secondary_links_heading-1079010-47.patch queued for re-testing.

The secondary links heading shouldn't affect the search config form or unmanaged remote file deletion. Let's try this again.

#47: secondary_links_heading-1079010-47.patch queued for re-testing.

#47: secondary_links_heading-1079010-47.patch queued for re-testing.

Just an update from #47.

#56: secondary_links_heading-1079010-56.patch queued for re-testing.

Re-rolled.. Although I also nested the code within an existing if statement since that's the only place it seems to be used. Should help performance.

It should all be within if (!empty($variables['secondary_menu'])) { now.

re-roll.

@Jalandhar Drat.. I was hoping you'd get to fixing the tests. What stopped you from producing a new patch?

@mgifford : Actually, I was not sure about few things in the failing tests and more over I was working on other tickets. Unassigned so that someone else can pick this up in the meanwhile

Checked for active menu for current page in this patch.

Ok, so I can see that with the patch the 2ndary menu is looking better.

But when I change the name of the menu to something other than "User account menu" it still comes out as:
<h2 id="links__system_secondary_menu" class="visually-hidden">User account menu</h2>

I really don't think this is related to this patch, but wanted to flag it as a concern before marking it RTBC.

Patch #74, no longer applies.

Here is the update for the patch.

Another try at the patch.

module_exists() is deprecated.

No longer seems to apply.

Rerolled patch.

I completely changed the menu here:
http://sfe2853d5ccc8e91.s2.simplytest.me/

In admin/structure/menu/settings I set a different menu to be the Source for the Secondary links.

After clearing cache I still have the User account menu header <h2 id="links__system_secondary_menu" class="visually-hidden">User account menu</h2> even when it's for the footer menu.

Similar problem as in #75 when you change the name of the menu to something other than "User account menu" it doesn't show up in the header.

Fixed it locally, will add a patch tomorrow.

This should nail it. While at it, I found out that this goes wrong for primary links too. The use case is limited, but one is able to change the source of the main menu. Then, the heading should reflect this too, just as with the secondary links. This patch fixes both.

It now uses the site config, so the labels are translatable too.

This looks great. It now follows the value of the secondary menu! Tested this on SimplyTest.me.

The comment in #37 seems legit. Sounds like the hardcoded name is actually the more useful label.

@lokapujya - Right now you can edit the "Main navigation" text so that an administrator could provide different information to a screen reader user through the UI. This isn't possible at the moment for the "Secondary navigation".

It is possible that "Secondary navigation" is more descriptive than the actual name of the menu, but since you can't change it now without adjusting the theme, it's kinda limiting.

@David_Rothstein's always got interesting/useful comments and #37 is no exception.

"why should we start exposing a potentially admin-facing menu name to end users of the site?"

The Main menu name is editable (as a block), so this isn't really new.

"The only extra information that a sighted user has here which a screen reader user does not is that the visual cues on the page indicate this menu is of secondary importance in the navigation structure"

Adding the name of the menu is more semantic. Adding more queues than are available to sighted users shouldn't be seen as a problem.

Just looking at #36 actually where @sun suggests this might not be needed due to #1869476: Convert global menus (primary links, secondary links) into blocks. And indeed it is!

Both menu headings can be edited now without this patch. Thanks everyone for their work on this patch.