Force last breadcrumb to be inactive

[maximpn]

Related to: #161540, #161539

Summary

Always force the last breadcrumb to be inactive.

Details

Usual UX expects the last breadcrumb to be inactive as it represents the current page. The same can be seen from EUI examples. It turns out Serverless Security Solution plugin does't remove href and onClick fields from the last breadcrumb and passes it to chrome.setBreadcrumbs() or serverless.setBreadcrumbs() which renders the last breadcrumb as active but clicking on it only refreshes the page. ESS Security Solution on the other hand processes breadcrumbs currently. The same behavior may be the case for the other plungs as well.

As it's much simpler to strip off undesired fields at one place instead of processing them in plugins it's done in packages/core/chrome/core-chrome-browser-internal/src/ui/header/header_breadcrumbs.tsx. Security Solution codebase has been updated accordingly.

A side effect of this PR is consistent ESS and Serverless breadcrumbs behavior and it will help to reuse ESS tests for Serverless.

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 7038f43

Failed CI Steps

• FTR Configs #8
• Explore - Security Solution Cypress Tests #1

Test Failures

• [job] [logs] Explore - Security Solution Cypress Tests #1 / Cases Creates a new case with timeline and opens the timeline Creates a new case with timeline and opens the timeline
• [job] [logs] FTR Configs #8 / serverless security UI Case View "before all" hook in "Case View"

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
core	365.8KB	365.9KB	+64.0B
securitySolutionEss	8.3KB	8.2KB	-112.0B
total			-48.0B

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @maximpn

[vadimkibana]

Before we go ahead with this, curious to hear opinions if it is worth losing the features the link in the last item brings:

1. It is consistent—always linked.
2. When page has extra parameters /last-breadcrumb?extra-param=value by clicking on the breadcrumb it removes the ?extra-param=value param.
3. User can right-click and copy the link.
4. User can cmd-click to open it in new tab.
5. User can click to "refresh" the page.
6. It is more consistent in testing—as automated tests can always rely that the link is present.

[maximpn]

@vadimkibana thank you for sharing you concerns 👍 I'm curious to heard the other opinions as well. I'll share my thoughts and observations below

Checking different plugins in the current ESS solution I haven't found any active last breadcrumb. You can see some screenshots below

For now we definitely have inconsistency between ESS and Serverless.

[Dosant]

I agree we have inconsistency between ESS and Serverless,
Don't want to overcomplicate this, but what do you think if instead of always forcing the last breadcrumb to be plain text, we'd check in serverless nav where we generate the root breadcrumbs from the navigation if the last breadcrumb is the exact path match with the deep link matching this navigation item. if it is - we remove the link, if it isn't (e.g. contains state or nested path), we keep the link so that it can be used to reset state or get back.
I can take a look into that, if you think this makes sense. I think this still should make this a lot more consistent.

[maximpn]

@Dosant it could be a good solution but I'm afraid matching breadcrumb's and actual paths will always give an active last breadcrumb in Security Solution. We have sourcerer parameter added on almost all pages. I haven't checked but it can be the case for the other plugins and there is chance such change is just an unnoticed side effect. Mostly we (I mean plugin engineers) work with routes and breadcrumbs get handled automatically.

Taking it into account we have the following

• Users can refresh the page by clicking browser's refresh button or the last breadcrumb. Do they know what's the difference? Should they know it? Should they have more than one way to refresh the page?
• Added url parameters usually represent the current default filters state. Users share the page by copying the url or clicking share button (like on a screenshot below.

If the filters changed and a link without filters was shared another user will see a different page's state. When a user wants to share the "default" page's link?

• The last breadcrumb is usually inactive because it represents the current page. If it's actionable users may be confused. If it's not the current page so where am I? Why url params affect navigation behavior if they are just params? It also may be perceived as a bug as all breadcrumbs look the same and one can think the last one (active) is missing.
• We should test all the scenarios. By having the last breadcrumb active we have more scenarios to test.

I don't mind having the last breadcrumb active, I'm just afraid it will make things more complex.

I'm also curious what @elastic/eui-team thinks.

[julianrosado]

In my opinion, the current pattern of a non-linking final breadcrumb element adheres to industry-accepted best practices by utilizing breadcrumbs solely for wayfinding. See: https://www.nngroup.com/articles/breadcrumbs/

Specifically

4. Include the current page as the last item in the breadcrumb trail.
5. In the breadcrumb trail, the breadcrumb corresponding to the current page should not be a link.

You should never have a link that does nothing. The last breadcrumb (denoting the current page) should not be a link. To avoid confusing users, visually differentiate between the current page and the preceding linked breadcrumb items, preferably using underlined or blue text.

This aligns with what I believe the user would expect in terms of behavior, which is consistent with common usage both in and out of Elastic. Introducing additional functionality that is effectively hidden within the link, such as clearing variables or opening a new tab, would be inconsistent with its usage elsewhere (again both in the product and out).

If you believe the juice worth the squeeze, I could see those actions potentially being implemented separately as a part of a share button or a different UI pattern elsewhere outside the breadcrumbs. My .02c!

[semd]

Hi all, I am aligned with @maximpn and @julianrosado here.

Actually, this is something we have been asking for ❤️

Solutions have been disabling the last breadcrumb intentionally with the traditional breadcrumbs API for ESS, using separate implementations. In serverless, we have the opportunity to unify this behavior using the navigationTree API (automatic breadcrumbs). I think this new API should solve what the solutions need, otherwise, we would be changing the pre-existing behavior everywhere.

And UX-wise, I agree it is confusing to have a breadcrumb link to the same page. Those side-functionalities could be implemented separately.

[Dosant]

Thanks for the feedback and explanation. Let's give it a go.

And UX-wise, I agree it is confusing to have a breadcrumb link to the same page. Those side-functionalities could be implemented separately.

The only thing that worries me is that, especially, in serverless with changed breadcrumbs there are still places that don't properly set the deeper breadcrumbs yet. These places are still to be found and fixed.
This means that you can go deeper in the navigation, but the last breadcrumb doesn't correctly reflect the navigation state. Such cases are definitely bugs, but when the last breadcrumb forced not to be a link, I think it makes the bug a bit worse

[maximpn]

Good point @Dosant 👍 Do you have examples of such places (that don't properly set the deeper breadcrumbs yet)? I know it can be missing tab navigation. If we are talking about a situation page changes url path without updating breadcrumbs it's definitely a severe bug. Just thinking if it makes sense to detect such situations and yield a warning like a console warning and telemetry so it can be easily tracked.

[spong]

Read up on the above discussion and approving on behalf of security-solution. Thank you everyone for all the details, and @julianrosado for the reference behavior here. And thank you @maximpn for adding more consistency here. LGTM! 👍