From 8957e4d9d06c65c5174906d944725eb8f4647496 Mon Sep 17 00:00:00 2001
From: Joffrey JAFFEUX <j.jaffeux@gmail.com>
Date: Mon, 8 Feb 2021 08:45:14 +0100
Subject: [PATCH] A11Y: makes user notifications list more accessible (#11992)

Previous markup used to be

```
<div>
  <div>
    <li>
```

Instead we will now have:

```
<ul>
  <li>
    <div>
```

Note this commit also adds two things:
- ability to override tagName of a widget when attaching it
- ability to pass opts and otherOpts to {{attach}}, it could be useful in templates but is mostly useful to test `tagName` for now
---
 .../app/widgets/user-notifications-large.js   |  9 ++++++++-
 .../discourse/app/widgets/widget.js           |  5 +++++
 .../tests/integration/widgets/widget-test.js  | 19 +++++++++++++++++++
 app/assets/stylesheets/common/base/user.scss  |  4 ++++
 lib/javascripts/widget-hbs-compiler.js        | 12 +++++++-----
 5 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/widgets/user-notifications-large.js b/app/assets/javascripts/discourse/app/widgets/user-notifications-large.js
index 9ca518313a3..cd36811af99 100644
--- a/app/assets/javascripts/discourse/app/widgets/user-notifications-large.js
+++ b/app/assets/javascripts/discourse/app/widgets/user-notifications-large.js
@@ -3,6 +3,8 @@ import { dateNode } from "discourse/helpers/node";
 import { h } from "virtual-dom";
 
 createWidget("large-notification-item", {
+  tagName: "li",
+
   buildClasses(attrs) {
     const result = ["item", "notification", "large-notification"];
     if (!attrs.get("read")) {
@@ -21,7 +23,10 @@ createWidget("large-notification-item", {
         `${notificationName.dasherize()}-notification-item`,
         attrs,
         {},
-        { fallbackWidgetName: "default-notification-item" }
+        {
+          fallbackWidgetName: "default-notification-item",
+          tagName: "div",
+        }
       ),
       h("span.time", dateNode(attrs.created_at)),
     ];
@@ -29,6 +34,8 @@ createWidget("large-notification-item", {
 });
 
 export default createWidget("user-notifications-large", {
+  tagName: "ul.notifications.large-notifications",
+
   html(attrs) {
     const notifications = attrs.notifications;
     const username = notifications.findArgs.username;
diff --git a/app/assets/javascripts/discourse/app/widgets/widget.js b/app/assets/javascripts/discourse/app/widgets/widget.js
index a69fee5a3b5..59253a09388 100644
--- a/app/assets/javascripts/discourse/app/widgets/widget.js
+++ b/app/assets/javascripts/discourse/app/widgets/widget.js
@@ -265,6 +265,11 @@ export default class Widget {
       const result = new WidgetClass(attrs, this.register, opts);
       result.parentWidget = this;
       result.dirtyKeys = this.dirtyKeys;
+
+      if (otherOpts.tagName) {
+        result.tagName = otherOpts.tagName;
+      }
+
       return result;
     } else {
       throw new Error(
diff --git a/app/assets/javascripts/discourse/tests/integration/widgets/widget-test.js b/app/assets/javascripts/discourse/tests/integration/widgets/widget-test.js
index e8b22828f92..e31e82e83c0 100644
--- a/app/assets/javascripts/discourse/tests/integration/widgets/widget-test.js
+++ b/app/assets/javascripts/discourse/tests/integration/widgets/widget-test.js
@@ -418,4 +418,23 @@ discourseModule("Integration | Component | Widget | base", function (hooks) {
       assert.equal(queryAll("div.test").text(), "Hello eviltrout");
     },
   });
+
+  componentTest("tagName", {
+    template: hbs`{{mount-widget widget="tag-name-override-test"}}`,
+
+    beforeEach() {
+      createWidget("test-override", { tagName: "div.not-override" });
+
+      createWidget("tag-name-override-test", {
+        template: widgetHbs`{{attach widget="test-override" attrs=attrs otherOpts=(hash tagName="section.override")}}`,
+      });
+    },
+
+    test(assert) {
+      assert.ok(
+        queryAll("section.override").length,
+        "renders container with overrided tagName"
+      );
+    },
+  });
 });
diff --git a/app/assets/stylesheets/common/base/user.scss b/app/assets/stylesheets/common/base/user.scss
index 2188b942465..fc9c89236e9 100644
--- a/app/assets/stylesheets/common/base/user.scss
+++ b/app/assets/stylesheets/common/base/user.scss
@@ -705,6 +705,10 @@
   }
 }
 
+.large-notifications {
+  margin: 0;
+}
+
 .large-notification {
   display: flex;
   align-items: center;
diff --git a/lib/javascripts/widget-hbs-compiler.js b/lib/javascripts/widget-hbs-compiler.js
index 63a66547d2f..58c911a1970 100644
--- a/lib/javascripts/widget-hbs-compiler.js
+++ b/lib/javascripts/widget-hbs-compiler.js
@@ -90,11 +90,13 @@ function mustacheValue(node, state) {
         node.hash.pairs.find((p) => p.key === "widget")
       );
 
-      let attrs = node.hash.pairs.find((p) => p.key === "attrs");
-      if (attrs) {
-        return `this.attach(${widgetName}, ${argValue(attrs)})`;
-      }
-      return `this.attach(${widgetName}, attrs)`;
+      const attrs = node.hash.pairs.find((p) => p.key === "attrs");
+      const opts = node.hash.pairs.find((p) => p.key === "opts");
+      const otherOpts = node.hash.pairs.find((p) => p.key === "otherOpts");
+
+      return `this.attach(${widgetName}, ${attrs ? argValue(attrs) : attrs}, ${
+        opts ? argValue(opts) : opts
+      }, ${otherOpts ? argValue(otherOpts) : otherOpts})`;
 
       break;
     case "yield":