From aef3f17b56d93b298feb76990f4f40d511352625 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Fri, 7 Jun 2024 11:31:46 +1000
Subject: [PATCH] DEV: A different approach to breadcrumbs (#27365)

Really fully authored by Jarek, I only made the PR :)

The `DBreadcrumbItem` and `DBreadcrumbContainer` components
introduced in 1239178f496cba5d864adb7c118b17902b8b72dc have
some limitations, mainly that the container has no awareness of
its items, so nothing that requires positional knowledge can
be used. This is needed to use `aria-current` on the last breadcrumb
item, see https://www.w3.org/WAI/ARIA/apg/patterns/breadcrumb/examples/breadcrumb/.

We change `DBreadcrumbItem` to always be a link, removing
the need for `LinkTo`. Then, we introduce a service to keep
track of containers and items (since all items are rendered into
all containers) and make the item itself responsible for registering
to the service, and introduce the needed `aria-current` behaviour.

---------

Co-authored-by: Jarek Radosz <jradosz@gmail.com>
---
 .../components/admin-plugin-config-page.gjs   | 34 ++++---------
 .../admin/addon/templates/plugins-index.hbs   | 13 +----
 .../addon/templates/plugins-show-settings.hbs | 14 ++---
 .../components/d-breadcrumbs-container.gjs    | 48 ++++++++++++-----
 .../app/components/d-breadcrumbs-item.gjs     | 45 ++++++++++++----
 .../d-breadcrumbs-container-modifier.js       | 27 ----------
 .../app/services/breadcrumbs-service.js       | 40 ---------------
 .../discourse/app/services/breadcrumbs.js     |  7 +++
 ...dcrumbs-test.js => d-breadcrumbs-test.gjs} | 51 ++++++++-----------
 9 files changed, 113 insertions(+), 166 deletions(-)
 delete mode 100644 app/assets/javascripts/discourse/app/modifiers/d-breadcrumbs-container-modifier.js
 delete mode 100644 app/assets/javascripts/discourse/app/services/breadcrumbs-service.js
 create mode 100644 app/assets/javascripts/discourse/app/services/breadcrumbs.js
 rename app/assets/javascripts/discourse/tests/integration/components/{d-breadcrumbs-test.js => d-breadcrumbs-test.gjs} (52%)

diff --git a/app/assets/javascripts/admin/addon/components/admin-plugin-config-page.gjs b/app/assets/javascripts/admin/addon/components/admin-plugin-config-page.gjs
index 29b75c48248..afffeb8d3b1 100644
--- a/app/assets/javascripts/admin/addon/components/admin-plugin-config-page.gjs
+++ b/app/assets/javascripts/admin/addon/components/admin-plugin-config-page.gjs
@@ -1,6 +1,5 @@
 import Component from "@glimmer/component";
-import { LinkTo } from "@ember/routing";
-import { inject as service } from "@ember/service";
+import { service } from "@ember/service";
 import DBreadcrumbsContainer from "discourse/components/d-breadcrumbs-container";
 import DBreadcrumbsItem from "discourse/components/d-breadcrumbs-item";
 import i18n from "discourse-common/helpers/i18n";
@@ -28,27 +27,16 @@ export default class AdminPluginConfigPage extends Component {
     <div class="admin-plugin-config-page">
       <DBreadcrumbsContainer />
 
-      <DBreadcrumbsItem as |linkClass|>
-        <LinkTo @route="admin" class={{linkClass}}>
-          {{i18n "admin_title"}}
-        </LinkTo>
-      </DBreadcrumbsItem>
-
-      <DBreadcrumbsItem as |linkClass|>
-        <LinkTo @route="adminPlugins" class={{linkClass}}>
-          {{i18n "admin.plugins.title"}}
-        </LinkTo>
-      </DBreadcrumbsItem>
-
-      <DBreadcrumbsItem as |linkClass|>
-        <LinkTo
-          @route="adminPlugins.show"
-          @model={{@plugin}}
-          class={{linkClass}}
-        >
-          {{@plugin.nameTitleized}}
-        </LinkTo>
-      </DBreadcrumbsItem>
+      <DBreadcrumbsItem @route="admin" @label={{i18n "admin_title"}} />
+      <DBreadcrumbsItem
+        @route="adminPlugins"
+        @label={{i18n "admin.plugins.title"}}
+      />
+      <DBreadcrumbsItem
+        @route="adminPlugins.show"
+        @model={{@plugin}}
+        @label={{@plugin.nameTitleized}}
+      />
 
       <AdminPluginConfigMetadata @plugin={{@plugin}} />
 
diff --git a/app/assets/javascripts/admin/addon/templates/plugins-index.hbs b/app/assets/javascripts/admin/addon/templates/plugins-index.hbs
index ab19b70abda..7d5165b5ed7 100644
--- a/app/assets/javascripts/admin/addon/templates/plugins-index.hbs
+++ b/app/assets/javascripts/admin/addon/templates/plugins-index.hbs
@@ -1,16 +1,7 @@
 <DBreadcrumbsContainer />
 
-<DBreadcrumbsItem as |linkClass|>
-  <LinkTo @route="admin" class={{linkClass}}>
-    {{i18n "admin_title"}}
-  </LinkTo>
-</DBreadcrumbsItem>
-
-<DBreadcrumbsItem as |linkClass|>
-  <LinkTo @route="adminPlugins" class={{linkClass}}>
-    {{i18n "admin.plugins.title"}}
-  </LinkTo>
-</DBreadcrumbsItem>
+<DBreadcrumbsItem @route="admin" @label={{i18n "admin_title"}} />
+<DBreadcrumbsItem @route="adminPlugins" @label={{i18n "admin.plugins.title"}} />
 
 <div class="admin-plugins-list-container">
   {{#if this.model.length}}
diff --git a/app/assets/javascripts/admin/addon/templates/plugins-show-settings.hbs b/app/assets/javascripts/admin/addon/templates/plugins-show-settings.hbs
index c26d05b09e3..a8c17447199 100644
--- a/app/assets/javascripts/admin/addon/templates/plugins-show-settings.hbs
+++ b/app/assets/javascripts/admin/addon/templates/plugins-show-settings.hbs
@@ -1,12 +1,8 @@
-<DBreadcrumbsItem as |linkClass|>
-  <LinkTo
-    @route="adminPlugins.show.settings"
-    @model={{@model.plugin}}
-    class={{linkClass}}
-  >
-    {{i18n "admin.plugins.change_settings_short"}}
-  </LinkTo>
-</DBreadcrumbsItem>
+<DBreadcrumbsItem
+  @route="adminPlugins.show.settings"
+  @model={{@model.plugin}}
+  @label={{i18n "admin.plugins.change_settings_short"}}
+/>
 
 <div
   class="content-body admin-plugin-config-area__settings admin-detail pull-left"
diff --git a/app/assets/javascripts/discourse/app/components/d-breadcrumbs-container.gjs b/app/assets/javascripts/discourse/app/components/d-breadcrumbs-container.gjs
index f1420655b33..fd7dd82db1e 100644
--- a/app/assets/javascripts/discourse/app/components/d-breadcrumbs-container.gjs
+++ b/app/assets/javascripts/discourse/app/components/d-breadcrumbs-container.gjs
@@ -1,17 +1,37 @@
+import Component from "@glimmer/component";
+import { service } from "@ember/service";
+import { modifier } from "ember-modifier";
+import { eq } from "truth-helpers";
 import concatClass from "discourse/helpers/concat-class";
-import dBreadcrumbsContainerModifier from "discourse/modifiers/d-breadcrumbs-container-modifier";
 
-const DBreadcrumbsContainer = <template>
-  <ul
-    class="d-breadcrumbs"
-    {{dBreadcrumbsContainerModifier
-      itemClass=(concatClass "d-breadcrumbs__item" @additionalItemClasses)
-      linkClass=(concatClass "d-breadcrumbs__link" @additionalLinkClasses)
-    }}
-    ...attributes
-  >
-    {{yield}}
-  </ul>
-</template>;
+export default class DBreadcrumbsContainer extends Component {
+  @service breadcrumbs;
 
-export default DBreadcrumbsContainer;
+  registerContainer = modifier((element) => {
+    const container = { element };
+
+    this.breadcrumbs.containers.add(container);
+    return () => this.breadcrumbs.containers.delete(container);
+  });
+
+  get lastItemIndex() {
+    return this.breadcrumbs.items.size - 1;
+  }
+
+  <template>
+    <ul {{this.registerContainer}} class="d-breadcrumbs" ...attributes>
+      {{#each this.breadcrumbs.items as |item index|}}
+        {{#let item.templateForContainer as |Template|}}
+          <Template
+            @linkClass={{concatClass
+              "d-breadcrumbs__link"
+              @additionalLinkClasses
+            }}
+            aria-current={{if (eq index this.lastItemIndex) "page"}}
+            class={{concatClass "d-breadcrumbs__item" @additionalItemClasses}}
+          />
+        {{/let}}
+      {{/each}}
+    </ul>
+  </template>
+}
diff --git a/app/assets/javascripts/discourse/app/components/d-breadcrumbs-item.gjs b/app/assets/javascripts/discourse/app/components/d-breadcrumbs-item.gjs
index 5d2ecca03bf..42371be304f 100644
--- a/app/assets/javascripts/discourse/app/components/d-breadcrumbs-item.gjs
+++ b/app/assets/javascripts/discourse/app/components/d-breadcrumbs-item.gjs
@@ -1,16 +1,39 @@
 import Component from "@glimmer/component";
-import { inject as service } from "@ember/service";
+import { service } from "@ember/service";
 
 export default class DBreadcrumbsItem extends Component {
-  @service breadcrumbsService;
+  @service breadcrumbs;
+  @service router;
 
-  <template>
-    {{#each this.breadcrumbsService.containers as |container|}}
-      {{#in-element container.element insertBefore=null}}
-        <li class={{container.itemClass}} ...attributes>
-          {{yield container.linkClass}}
-        </li>
-      {{/in-element}}
-    {{/each}}
-  </template>
+  constructor() {
+    super(...arguments);
+    this.breadcrumbs.items.add(this);
+  }
+
+  willDestroy() {
+    super.willDestroy(...arguments);
+    this.breadcrumbs.items.delete(this);
+  }
+
+  get url() {
+    if (this.args.model) {
+      return this.router.urlFor(this.args.route, this.args.model);
+    } else {
+      return this.router.urlFor(this.args.route);
+    }
+  }
+
+  get templateForContainer() {
+    // Those are evaluated in a different context than the `@linkClass`
+    const { label } = this.args;
+    const url = this.url;
+
+    return <template>
+      <li ...attributes>
+        <a href={{url}} class={{@linkClass}}>
+          {{label}}
+        </a>
+      </li>
+    </template>;
+  }
 }
diff --git a/app/assets/javascripts/discourse/app/modifiers/d-breadcrumbs-container-modifier.js b/app/assets/javascripts/discourse/app/modifiers/d-breadcrumbs-container-modifier.js
deleted file mode 100644
index 58a96380791..00000000000
--- a/app/assets/javascripts/discourse/app/modifiers/d-breadcrumbs-container-modifier.js
+++ /dev/null
@@ -1,27 +0,0 @@
-import { registerDestructor } from "@ember/destroyable";
-import { inject as service } from "@ember/service";
-import Modifier from "ember-modifier";
-
-export default class DBreadcrumbsContainerModifier extends Modifier {
-  @service breadcrumbsService;
-
-  container = null;
-
-  modify(element, _, { itemClass, linkClass }) {
-    if (this.container) {
-      return;
-    }
-
-    this.container = { element, itemClass, linkClass };
-
-    this.breadcrumbsService.registerContainer(this.container);
-
-    registerDestructor(this, unregisterContainer);
-  }
-}
-
-function unregisterContainer(instance) {
-  if (instance.container) {
-    instance.breadcrumbsService.unregisterContainer(instance.container);
-  }
-}
diff --git a/app/assets/javascripts/discourse/app/services/breadcrumbs-service.js b/app/assets/javascripts/discourse/app/services/breadcrumbs-service.js
deleted file mode 100644
index 1a3f19466a9..00000000000
--- a/app/assets/javascripts/discourse/app/services/breadcrumbs-service.js
+++ /dev/null
@@ -1,40 +0,0 @@
-import { tracked } from "@glimmer/tracking";
-import { warn } from "@ember/debug";
-import Service from "@ember/service";
-
-export default class BreadcrumbsService extends Service {
-  @tracked containers = [];
-  #containers = [];
-
-  registerContainer(container) {
-    if (this.#isContainerRegistered(container)) {
-      warn(
-        "[BreadcrumbsService] A breadcrumb container with the same DOM element has already been registered before."
-      );
-    }
-
-    this.#containers = [...this.#containers, container];
-
-    this.containers = this.#containers;
-  }
-
-  unregisterContainer(container) {
-    if (!this.#isContainerRegistered(container)) {
-      warn(
-        "[BreadcrumbsService] No breadcrumb container was found with this DOM element."
-      );
-    }
-
-    this.#containers = this.#containers.filter((registeredContainer) => {
-      return container.element !== registeredContainer.element;
-    });
-
-    this.containers = this.#containers;
-  }
-
-  #isContainerRegistered(container) {
-    return this.#containers.some((registeredContainer) => {
-      return container.element === registeredContainer.element;
-    });
-  }
-}
diff --git a/app/assets/javascripts/discourse/app/services/breadcrumbs.js b/app/assets/javascripts/discourse/app/services/breadcrumbs.js
new file mode 100644
index 00000000000..b585690e62d
--- /dev/null
+++ b/app/assets/javascripts/discourse/app/services/breadcrumbs.js
@@ -0,0 +1,7 @@
+import Service from "@ember/service";
+import { DeferredTrackedSet } from "discourse/lib/tracked-tools";
+
+export default class Breadcrumbs extends Service {
+  containers = new DeferredTrackedSet();
+  items = new DeferredTrackedSet();
+}
diff --git a/app/assets/javascripts/discourse/tests/integration/components/d-breadcrumbs-test.js b/app/assets/javascripts/discourse/tests/integration/components/d-breadcrumbs-test.gjs
similarity index 52%
rename from app/assets/javascripts/discourse/tests/integration/components/d-breadcrumbs-test.js
rename to app/assets/javascripts/discourse/tests/integration/components/d-breadcrumbs-test.gjs
index 2d6b93f3b6a..30032874cdb 100644
--- a/app/assets/javascripts/discourse/tests/integration/components/d-breadcrumbs-test.js
+++ b/app/assets/javascripts/discourse/tests/integration/components/d-breadcrumbs-test.gjs
@@ -1,7 +1,9 @@
 import { render } from "@ember/test-helpers";
-import { hbs } from "ember-cli-htmlbars";
 import { module, test } from "qunit";
+import DBreadcrumbsContainer from "discourse/components/d-breadcrumbs-container";
+import DBreadcrumbsItem from "discourse/components/d-breadcrumbs-item";
 import { setupRenderingTest } from "discourse/tests/helpers/component-test";
+import i18n from "discourse-common/helpers/i18n";
 
 module(
   "Component | DBreadcrumbsContainer and DBreadcrumbsItem",
@@ -9,19 +11,11 @@ module(
     setupRenderingTest(hooks);
 
     test("it renders a DBreadcrumbsContainer with multiple DBreadcrumbsItems", async function (assert) {
-      await render(hbs`
-      <DBreadcrumbsContainer />
-      <DBreadcrumbsItem as |linkClass|>
-        <LinkTo @route="admin" class={{linkClass}}>
-          {{i18n "admin_title"}}
-        </LinkTo>
-      </DBreadcrumbsItem>
-      <DBreadcrumbsItem as |linkClass|>
-        <LinkTo @route="about" class={{linkClass}}>
-          {{i18n "about.simple_title"}}
-        </LinkTo>
-      </DBreadcrumbsItem>
-  `);
+      await render(<template>
+        <DBreadcrumbsContainer />
+        <DBreadcrumbsItem @route="admin" @label={{i18n "admin_title"}} />
+        <DBreadcrumbsItem @route="about" @label={{i18n "about.simple_title"}} />
+      </template>);
 
       assert
         .dom(".d-breadcrumbs .d-breadcrumbs__item .d-breadcrumbs__link")
@@ -29,14 +23,13 @@ module(
     });
 
     test("it renders a DBreadcrumbsItem with additional link and item classes", async function (assert) {
-      await render(hbs`
-      <DBreadcrumbsContainer @additionalLinkClasses="some-class" @additionalItemClasses="other-class" />
-      <DBreadcrumbsItem as |linkClass|>
-        <LinkTo @route="admin" class={{linkClass}}>
-          {{i18n "admin_title"}}
-        </LinkTo>
-      </DBreadcrumbsItem>
-  `);
+      await render(<template>
+        <DBreadcrumbsContainer
+          @additionalLinkClasses="some-class"
+          @additionalItemClasses="other-class"
+        />
+        <DBreadcrumbsItem @route="admin" @label={{i18n "admin_title"}} />
+      </template>);
 
       assert.dom(".d-breadcrumbs .d-breadcrumbs__item.other-class").exists();
       assert
@@ -47,15 +40,11 @@ module(
     });
 
     test("it renders multiple DBreadcrumbsContainer elements with the same DBreadcrumbsItem links", async function (assert) {
-      await render(hbs`
-      <DBreadcrumbsContainer />
-      <DBreadcrumbsContainer />
-      <DBreadcrumbsItem as |linkClass|>
-        <LinkTo @route="admin" class={{linkClass}}>
-          {{i18n "admin_title"}}
-        </LinkTo>
-      </DBreadcrumbsItem>
-  `);
+      await render(<template>
+        <DBreadcrumbsContainer />
+        <DBreadcrumbsContainer />
+        <DBreadcrumbsItem @route="admin" @label={{i18n "admin_title"}} />
+      </template>);
 
       assert.dom(".d-breadcrumbs").exists({ count: 2 });
       assert.dom(".d-breadcrumbs .d-breadcrumbs__item").exists({ count: 2 });