Page title should contextualize the local navigation

I tested it on my local and it works fine, it’s definitely an improvement 👍🏻

But this would be applied for all the pages but on some pages we don't need it like for login page('user/login') and I don't think we need it as it will show title as 'My account' instead of 'Log in'.

Just paired with @lauriii and we found that our current change in \Drupal\Core\Render\MainContent\HtmlRenderer::prepare() is incorrect as it also affects the browser tab title which we don't want. I'll revert that change.

Currently the title shown on page 'admin/structure/block-content/manage/basic' is 'Edit Basic block block type' because we override the title in '\Drupal\block_content\BlockContentTypeForm::form' but now in this current MR we again override it in 'PageTitleBlock' based on base route and now it shows the title as 'Edit Basic block'. So I think we need to decide what we want (that could be done in a follow-up). I think it should be 'Basic block' and not 'Edit Basic block block type' because the 'Edit' word is already shown on the tab and 'Block Types' is shown on the breadcrumbs. See the which shows edit basic block page based on the current MR.

The tests are failing because now we use the correct service '@router'(AccessAwareRouter) in 'RequestGenerator' but what this does is it checks the access for the route but PageTitleBlock uses '@router' for the matching of request for base route so it will check it's access and and it'll fail as the tests obviously don't grant the permissions for base route.
I think for PageTitleBlock we'll have to use 'router.no_access_checks' for the matching of request for base route so it will not check it's access.

I don't know how much usability consideration was given to this change. I didn't find any mentions of usability in these issues.

When the original router patch was committed, there wasn't even thought given to the relationship between a router item, its access, and a menu link (see #1793520-7: Add access control mechanism for new router system downwards), and it took several people years, blocking Drupal 8's release, to repair some (but not all) of the damage done to the menu system. #2407505: [meta] Finalize the menu links (and other user-entered paths) system and sub-issues has some of the pain, and you can see that wasn't even opened until two years after the original router commit landed. So short version: none, but also worse than none.

re #16

First about the points under "What could we implement here?":

In regards of the separator I don't have a strong opinion. I've just tested the latest state of the MR with VoiceOver on the Mac. The only downside with separators is see for screenreader users is that Manage fields | Article | Drupal turns into Manage fields vertical line article vertical line drupal even if the default speech verbosity is set to low. vertical line is a bit verbose and wordy. maybe pick another separator? if a separator is used for the h1 between Manage fields | Article it would make things consistent with the title as well. the separator would be visually hidden to sighted users anyway and for screenreader users it might be confusing otherwise if the title uses a separator while the h1 uses the word for instead?

The problem of translations you've brought up in the context of the word for is a relevant point - good catch. It would be interesting to make a brief ad-hoc survey in one of the translation related channels on the Drupal Slack and ask translators how easy or difficult a snippet like Manage fields for Article content type would be to translate?
As I've said I have no strong opinions in that regard neither for the use of a separator nor the use of the word for instead. but if i remember correctly, back when we've discussed the topic, either @benjifisher or @aaronmchale voted strongly for the usage of the word for instead of : (the separator we've planned to use back then). So i think it would be a good thing to bring them into the discussion. or better put this issue on one of the next ux meetings and restart the discussion around the topic (only next friday won't work because benji is away).

In regards of making the title and the h1 more concise and clear I think getting some momentum back into #2521780: The 'Edit', 'Manage display' and 'Manage form display' tabs were hard to understand would be a good thing. We've discussed it during the UX meetings back then as well. The following Slack comment summarizes the outcomes. We were in line with the idea voted for in the issue to ideally make the tab labels one worded. It was mostly about removing the verbs Manage. The suggestions from the issue and our discussions back then were:

One suggestion for the decision process forward. It would be a good thing to discuss that aforementioned list and come up with a list of 4 to 5 different options for each tab everyone involved in the discussion would be comfortable with. then run a one-two punch survey for each tab label. the first question is a quantitive one about "the what" (a radio button list where the user is able to select the preferred term for example the options would be "view, display, view modes, other"). and an open ended question to get qualitative feedback. something like "tell us why you chose this answer. In case you chose "other" why would you use this word instead? Walk us through your thinking." That way the choice would be grounded in actual feedback and you could quickly get a feeling how comprehensible the choice of a one worded tab label would be. Because the folks involved in the issue already have an inherent bias.

re #18

I've tested the suggestion of @bnjmnm to use : instead of |. The colon is the only separator which is not announced by any screenreader. Someone created a comparison in the context of gender inclusive language in mainly german (but the introduction and conclusion are in english and the comparison table is quickly translatable. but it is a good resource: https://taner-aydin.dev/a11y-up/genderinklusive-sprache-und-barrierefrei... ). So as I already said in my previous comment personally i would be fine with the suggestion of using the colon but i would include benji and aaron as well.

about the current state of the MR - a few comments and suggestions.

[Section page title]: [Current page title] was suggested for the h1 in #18. I would strongly vote against that and use it the other way around: [Current page title] : [Section page title] . we've intentionally front-loaded the order to ease the scan and processabilitiy. the current page title is the most important piece of information then comes the section page title. and when the follow up issue gets in then the bundle name. (that bundle name follow up is essential otherwise it is impossible to determine which bundle type is in front of you. just based on the title and or h1 it is impossible right now)

when the h1 is using the colon as the seperator now how about [Current page title] : [Section page title] | [site title] for the title. that way you wouldnt have three separate pieces of information divided by the announcement of vertical line. instead you have manage fields article vertical line my drupal 9 site. that way things would be way more clear to screenreader users.

for other sections of the admin interface i have to take a closer look and think about it. back then we were entirely focused on the scope for #2514218: [regression] Pages Manage Fields, Manage form, Manage display should include name of content type or entity. we already saw the need for other parts in the admin ui but havent had an in depth discussion yet.

Addendum: One detail I forgot to test. If you go to /admin/structure/types/manage/article/display/full you currently have the following output:

title: Manage display | Article | My Drupal site
h1: Article: Manage Display

The secondary nav context is missing from the title as well as the h1. One detail we forgot in our discussions back then as well. But i am uncertain if something like Full content | Manage display | Article | My Drupal site . Have to think about that a bit more but just wanted to note that the secondary task nav has to be taken into consideration as well.

To get a feeling and an overview aside the bundle types from the postponed regression issue I quickly went through the admin pages and created a spreadsheet with all the titles, h1s, primary task labels, and secondary task labels based on the MR in this issue. i always prefer to have all the content in a spreadsheet next to each other. that way it is easier to assess the readability and consistency https://docs.google.com/spreadsheets/d/1XhQtXZA1JCwa8qnE3vp8gyW8HJT8zZxg...

@rkoller the spreadsheet doesn't have permissions to be viewed :)


the current page title is the most important piece of information then comes the section page title

Agreed. The point is that the existing value for page title is not correct: it was wrongly switched to the value of the tab as comment #18 and #19 pointed out. Here’s an screenshot of Drupal 7: you’re in the Article content type itself and we use tabs to navigate across the options available for it.

To note the <title> here was Article | Site name.

We load each tab in different pages because it was built like this, but tabs are a standard pattern where you separate content in different groups that are part of the same “piece”. Call it Merge Request, Settings or Content type, but you can see patterns of this everywhere. So this is what users expect (you can see it was perceived as an improvement in our recent tests in #3379160: Toolbar Prototype Usability Testing Phase I) and there is no need to repeat the value of the tab in the title: the rest of the UI elements give enough clues to understand the hierarchy.
 And another thing to add here, is that a solution like “Manage fields for Article content type” is worst for h1 than “Article”. It’s more verbose, but it results in worst scannability to quickly understand where you are in a first glance. For sighted users, a short and concise title is better if it’s combined with visual clues that help orientate/locate yourself. We’ve got already a complex UI and we should push to make it more clear, not more crowded if there is not a real need. I understand you also want to add the bundle, but that should be solved with UI elements (which we’re taking into account in #3076820: [META] Layout redesign) and not more text in the title. I can show a lot of examples, but we better discuss it in the follow-up and focus on the topic at hand here.

[Section page title]: [Current page title] was suggested for the h1 in #18. I would strongly vote against that and use it the other way around: [Current page title] : [Section page title]

Here we go back to the same: what we call currently the page title is not conceptually correct for Manage display for example. What we’re calling here “section title” (Article) is what should be the page title (and what it was already). So agreed that the page title should be first element because it’s the most important piece of info, it’s just that the value we’re getting here is not correct. Here it should be Article: Manage fields.

About the for vs :, as @lauriii said for doesn't tranalate well to other languages (AFAIK Finish is already an example) so is a strong enough reason already without the need to prove it with a survey. On top of that, patterns like "Update for Extend", "Extend: Update", "Update: Extend" make more sense with a colon even in English. It’s “agnostic” of the conceptual relationship between elements. So +1 to the “:” with the current info, subject to any new recommendation by @bnjmnm.

Also, totally agree with getting to #2521780: The 'Edit', 'Manage display' and 'Manage form display' tabs were hard to understand again. Let’s discuss the changes for that in there and focus on the discussion here for the changes in the title :)

> This works more consistently across the board, e.g. something like "Uninstall for Extend" isn't nice, but "Extend: Uninstall" works fairly well. This

True, but having 'Edit display for Article' would be *really nice*.

At the risk of derailing and adding complexity, what if there were a way to vary how the local page title & the tab parent title are combined?

Off the top of my head, could we pass the tab parent title in to the page title callback as a parameter, and let the callback decide how to combine them?

This could be done in a follow-up -- the perfect is the enemy of the good :)

This could be done in a follow-up -- the perfect is the enemy of the good :)

Agreed! I understand wanting to nail the page title, but I'd recommend arriving to a minimum agreement with something not bad and refine it later in a follow-up, and land the h1 changes soon because it's a big UX improvement for sighted users.

I'd personally go with the pattern that results in the following structures because it avoids assuming an specific combination of order and connectors (from English):

• Article: Manage fields
• People: Roles
• Extend: Uninstall
• Synchronize: Import

(Again, thanks @rkoller for the spreadsheet, it's super useful)

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 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.

Postponed https://www.drupal.org/project/drupal/issues/2938129 on this one.

Work can continue in the MR, but for the short term here is a snapshot of the changes backported to 10.1.x

Will work on remaining things like change record, release note, clean-up etc.

Created change record https://www.drupal.org/node/3386683

Created follow-ups #3386724: Change the separator in document title and #3386732: Refine the page title if possible.

Added release note

Looks like all feedback has been addressed and works as expected. MR looked good to me. Followups have also been created to revisit the formatting if need be but this is already a big UX improvement. Also checked how the titles read on VoiceOver.

Woo this is huge improvement! Congrats everyone!

Testing this as Drupalcon today. I'm trying to understand the reasoning behind this change, and talked with lauriii about this. We added one more sentence to the change record to help existing sites/themes to make a decision.
I clicked through some example pages:

- /admin
- /admin/content (On this one 'Content' appears multiple times, but that seems correct, given that admin/content/overview is the default tab.
- /admin/modules
- ...

When the original router patch was committed, there wasn't even thought given to the relationship between a router item, its access, and a menu link (see #1793520-7: Add access control mechanism for new router system downwards), and it took several people years, blocking Drupal 8's release, to repair some (but not all) of the damage done to the menu system. #2407505: [meta] Finalize the menu links (and other user-entered paths) system and sub-issues has some of the pain, and you can see that wasn't even opened until two years after the original router commit landed. So short version: none, but also worse than none.

It is an interesting aspect that this moves the responsibility from the developer towards the site builder/theme, the layer that actually knows what sensible. From my experience this makes the most sense.
While reading the code I had two minor nitpicks

FYI gitlab and drupalCi seem happy but when I did a diff of the MR to apply locally I got

 trailing whitespace.
�`4n6�U�[�g[3�(Ng��^z��D�XYN�b�N��*QiN�Lyt��h�RQ        A=�     ]߸W�8z�0f����H\��P[LH�P`q���`��j�>oW��3
warning: 1 line adds whitespace errors.

Probably nothing but wanted to note.

Applied the MR and cleared cache several times but when I go to a manage display for any entity type it still just says "Manage Display" nothing changed from before.

If that's no longer the goal or it's something different could the issue summary be updated please

Thanks!!

Didn't realize it needed an update.

That did solve the issue but am noticing some differences based on entity type
If I go to manage display for the Basic block I get title "Edit Basic block"
If I go to manage display for the Article content type I get title "Article"

Thanks for pointing out that follow up @lauriii.

Believe @kunal.sachdev has addressed the feedback.

As this is targeting 10.3 and won't be backported to 10.2, here's a patch for 10.2

Hiding patches because there's an MR

Updating issue credits

Left a review, this is looking really good and is a great improvement - still a fair bit to do though - thanks!

I tested this manually on secondary local tasks, e.g. on 'teaser' under 'manage display'

For all the secondary tasks it will show something like this:

<h1 class="page-title">Article<span class="visually-hidden">: Manage display</span></h1>

I was expecting it might show the context of the view mode (e.g. Teaser)

But I checked what happens in HEAD and it shows Manage display so there's a definite improvement.

@lauriii mentioned he was working to improve things further and that would definitely be a nice thing to do in a future issue (follow up).

Postponed on #3376566: Display the page title, even if "0" in olivero and #3412316: Use 'search.view' instead of 'search.plugins:' . $default for base route in SearchLocalTask::getDerivativeDefinitions() to keep the scope of this issue small.

All tests are passing now and I think there is no need to postpone this issue anymore.

Left some small comments.

All the feedback was addressed @larowlan if you want to do a second pass until then RTBC'ed

Needs a rebase.

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.

Added some review comments to the MR. I think is we apply the change to the HTML title only to admin routes tests will fail... so it'll need some work.

I'm not sure, but this issue might be slightly related here #3344200: PathBasedBreadcrumbBuilder needs to be able to exclude pointless paths.

One thing I see here is that if the page title is consistent across all local tasks, then the barodcrumb probably doesn't need to contain the primary local task. For example, when on the "Manage fields" tab of the Article content type, "Article" appears in the breadcrumb, but it probably doesn't need to be there because it simply goes back to the "Edit" tab.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

Rebased with latest changes.

The latest changes look good, @alexpott if you can approve this too that would be great.

Committed and pushed ad606a8fb7 to 11.x and 6e6f391806 to 10.3.x. Thanks!

Tagging to big this up in the release.

Congrats on this one!

Thanks! It looks like an image is missing after "Users can change their page title configuration ..." in the Change Record.

We were trying to reproduce a bug at Drupal South 2024 #2895531: Cannot uninstall Content Moderation and found that this issue has caused a fatal error for all entity-types when using PrepareModulesEntityUninstallForm

I discussed this with @quietone at Drupal South and we agreed because this has only been in core for 10 days and it has introduced a new fatal error that makes it impossible to uninstall any module that has entity-types - we decided to revert it.

Steps to reproduce the bug

• Install Umami profile
• Edit a piece of content to move it to draft
• Visit Extend page and click link next to Content moderation checkbox to 'Remove content moderation states'
• Fatal error

I'm adding credit for @yiySHAO, @dscl, @ameymudras and @samundrasharma who worked with me.

I've unpublished the three change notices.

Please add back the 10.3.0 release notes and 10.3.0 release highlights tags when this goes back in.

So this is happening because the route definition

system.prepare_modules_entity_uninstall:
  path: '/admin/modules/uninstall/entity/{entity_type_id}'
  defaults:
    _form: '\Drupal\system\Form\PrepareModulesEntityUninstallForm'
    _title_callback: '\Drupal\system\Form\PrepareModulesEntityUninstallForm::formTitle'
  requirements:
    _permission: 'administer modules'

has a bug. \Drupal\system\Form\PrepareModulesEntityUninstallForm::formTitle does not exist. The title for this page is set by $form['#title'] = $this->getQuestion(); in \Drupal\Core\Entity\ContentEntityConfirmFormBase::buildForm - this take precedence. But now with title logic in \Drupal\Core\Block\Plugin\Block\PageTitleBlock::getTitleBasedOnBaseRoute() we are finally try to call the bogus callback.

The correct fix is to remove the bogus callback... but what should we do if contrib / custom has the same problem? Do we baby sit the broken code by doing something like:

    try {
      $controller_title = $this->titleResolver->getTitle($this->requestStack->getCurrentRequest(), $this->routeMatch->getRouteObject());
    }
    catch (\InvalidArgumentException $e) {
      $controller_title = '';
    }

in \Drupal\Core\Block\Plugin\Block\PageTitleBlock::getTitleBasedOnBaseRoute but this feels kinda wrong. Not sure about the best way to proceed.

There is an open issue for fixing the missing _title_callback: #2862702: PrepareModulesEntityUninstallForm::formTitle does not exist

@larowlan, @xjm and I have discussed it here at DrupalSouth 2024 just now, and we will be working on that issue to get it fixed.
And as consequence we agree to mark this as postponed.

I believe we still need to fix #2862702: PrepareModulesEntityUninstallForm::formTitle does not exist so we have the expected behaviour for a form that is part of core, but something like the baby sitting mentioned on #100 would still be necessary to ensure other modules wouldn't fall into the same problem.
I would say is needs to live inside \Drupal\Core\Controller\BaseRouteTitleResolver:getTitle, and this addition would be contextually fine to be part of this issue.

I agree it is kind of ugly and there may be a better way to throw some sort of warning to the logs and not a fatal error.

Then, thinking again, if the try/catch gets added here this issue may not depend on the other being fixed and could be back on track to be committed.

Hiding all the patches. I think given what we found with core we need to put something like the code babysitting discussed in #100 so this does not need to be postponed on #2862702: PrepareModulesEntityUninstallForm::formTitle does not exist

@dscl the exception is coming from the call to the regular title resolver in \Drupal\Core\Block\Plugin\Block\PageTitleBlock::getTitleBasedOnBaseRoute() - it's not the base route title resolver. So the fix needs to go there and not in BaseRouteTitleResolver.

I've implement the fix and tested manually. Things I'm not sure about... should we log this? Should we add test coverage - adding a broken title resolver feels funny.

Discussed with @catch and @longwave about whether we should put in defensive coding around broken routes and we concluded that letting Drupal error was better than either:

• Checking whether route controller exists on route build - this would result in loading too many classes into op cache
• Adding defensive coding around handling broken routes in the code here - this is likely to hide the error in a worse way that just displaying the error from PHP.

This look great! Thanks for working on this and improving the UX.

@longwave pointed out the adding RequestGenerator to the Utility namespace is not great. Moved to the Routing namespace as this already has:

• \Drupal\Core\Routing\RequestHelper
• \Drupal\Core\Routing\RequestFormatRouteFilter
• \Drupal\Core\Routing\RequestContext

So it not that surprising to find this there.

Added some minor feedback.

I've addressed @longwave's feedback - as it was all nits setting back to rtbc.

I think this is almost ready to commit but we need to decide which branches this goes in given it has an update path (and change the deprecation messages to match).

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

I really like this change, but I'm not sure about it being yet another setting. Our admin UI can be opinionated!

And I don't understand the bit about before/after local tasks.

Rerolled Patch for 10.2.10

Adding credits for in-person discussions.