From 78310464d5bb7efc74c1f98626cd5b9044b8a287 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9?= <34163393+amtins@users.noreply.github.com>
Date: Tue, 28 Nov 2023 23:11:15 +0100
Subject: [PATCH] fix(error-display): update display on consecutive errors
 (#8485)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When consecutive errors occur, the `ErrorDisplay` component is not updated with the new error message.
This results in an inconsistent state between the `player.error` and `player.errorDisplay.contentEl().textContent`.

|                         | player.error() | player.errorDisplay.content() | player.errorDisplay.contentEl().textContent |
| ----------------------- | -------------- | ----------------------------- | ------------------------------------------- |
| player.error('Error 1') | Error 1 ✔️      | Error 1 ✔️                     | Error 1 ✔️                                   |
| player.error('Error 2') | Error 2 ✔️      | Error 2 ✔️                     | Error 1 ❌                                  |

An example of a use case where updating the error message is useful is :
- user tries to play media 1 but the media doestn't exist
- user tries to play media 2 but the media is not compatible

- call the `close` function before each call to the `open` function.
  - if errorDisplay is not **open**, the `close` function does nothing
  - if errorDisplay is **open**, the `close` function executes and triggers the close events, then the open function executes and triggers the open events, ensuring that the content is updated.
---
 src/js/error-display.js         |  5 ++-
 test/unit/error-display.test.js | 63 +++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 1 deletion(-)
 create mode 100644 test/unit/error-display.test.js

diff --git a/src/js/error-display.js b/src/js/error-display.js
index d6713779cb..a2ab7d0e3f 100644
--- a/src/js/error-display.js
+++ b/src/js/error-display.js
@@ -23,7 +23,10 @@ class ErrorDisplay extends ModalDialog {
    */
   constructor(player, options) {
     super(player, options);
-    this.on(player, 'error', (e) => this.open(e));
+    this.on(player, 'error', (e) => {
+      this.close();
+      this.open(e);
+    });
   }
 
   /**
diff --git a/test/unit/error-display.test.js b/test/unit/error-display.test.js
new file mode 100644
index 0000000000..3248078545
--- /dev/null
+++ b/test/unit/error-display.test.js
@@ -0,0 +1,63 @@
+/* eslint-env qunit */
+import TestHelpers from './test-helpers.js';
+
+QUnit.module('ErrorDisplay');
+
+QUnit.test('should update errorDisplay when several errors occur in succession', function(assert) {
+  const player = TestHelpers.makePlayer({ techOrder: ['html5'] });
+  const events = {
+    beforemodalopen: 0,
+    beforemodalclose: 0,
+    modalopen: 0,
+    modalclose: 0
+  };
+
+  player.errorDisplay.on(
+    ['beforemodalopen', 'beforemodalclose', 'modalopen', 'modalclose'],
+    ({ type }) => {
+      events[type] += 1;
+    }
+  );
+
+  // Dummy case for initial state
+  assert.strictEqual(player.error(), null, 'error is null');
+  assert.strictEqual(
+    player.errorDisplay.contentEl().textContent,
+    '',
+    'error display contentEl textContent is empty'
+  );
+  assert.strictEqual(events.beforemodalopen, 0, 'beforemodalopen was not called');
+  assert.strictEqual(events.modalopen, 0, 'modalopen was not called');
+  assert.strictEqual(events.beforemodalclose, 0, 'beforemodalclose was not called');
+  assert.strictEqual(events.modalclose, 0, 'modalclose was not called');
+
+  // Case for first error
+  player.error('Error 1');
+
+  assert.strictEqual(player.error().message, 'Error 1', 'error has message');
+  assert.strictEqual(
+    player.errorDisplay.contentEl().textContent,
+    'Error 1',
+    'error display contentEl textContent has content'
+  );
+  assert.strictEqual(events.beforemodalopen, 1, 'beforemodalopen was called once');
+  assert.strictEqual(events.modalopen, 1, 'modalopen was called once');
+  assert.strictEqual(events.beforemodalclose, 0, 'beforemodalclose was not called');
+  assert.strictEqual(events.modalclose, 0, 'modalclose was not called');
+
+  // Case for second error
+  player.error('Error 2');
+
+  assert.strictEqual(player.error().message, 'Error 2', 'error has new message');
+  assert.strictEqual(
+    player.errorDisplay.contentEl().textContent,
+    'Error 2',
+    'error display contentEl textContent has been updated with the new error message'
+  );
+  assert.strictEqual(events.beforemodalopen, 2, 'beforemodalopen has been called for the second time');
+  assert.strictEqual(events.modalopen, 2, 'modalopen has been called for the second time');
+  assert.strictEqual(events.beforemodalclose, 1, 'beforemodalclose was called once');
+  assert.strictEqual(events.modalclose, 1, 'modalclose was called once');
+
+  player.dispose();
+});