From b7de49e1b88398852f75b25bf606e0b165ec3a53 Mon Sep 17 00:00:00 2001
From: Martin Geisler <mgeisler@google.com>
Date: Thu, 12 Jan 2023 12:26:07 +0100
Subject: [PATCH] Simplify state transitions for speaker notes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Before, we attempted to change state from “popup” to “inline-open”
when the speaker note window was closed.

We did this by listening to “pagehide” and change the state there. The
event fires every time you navigate away from the page, so we had a
complex setup where we would reset the state to “popup” when the next
page was loaded into the speaker note window.

The problem with this is that it’s racy: we could end up in a
situation where we set the state to “inline-open” right after the
speaker note window was updated. When that happened, we would mark the
window as “defunct”, meaning that it was supposed to be closed.

With this change, we no longer try to change the state from the
speaker note window. If the window is lost (closed), the user will
have to click the “Close speaker notes” button in the top-right to
reset the state. This should be much more reliable.

Long-term, a better solution would be to let the speaker notes fetch
the current URL using JavaScript instead of doing it via an actual
page navigation. That should allow us to react to “pagehide” events
again (since they won’t fire on every page transition).
---
 speaker-notes.css | 13 +++++++------
 speaker-notes.js  | 33 ++++++++++++++++++---------------
 2 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/speaker-notes.css b/speaker-notes.css
index 394d4c4b..7c14a17f 100644
--- a/speaker-notes.css
+++ b/speaker-notes.css
@@ -19,12 +19,13 @@
   width: 40px;
 }
 
-.content details summary a {
-  margin-left: 0.5em;
+.content details summary .pop-out {
+  color: var(--icons);
+  padding: 0 8px;
+  cursor: pointer;
+  transition: color 0.5s;
 }
 
-@media only screen and (max-width: 1080px) {
-  .content details summary a {
-    display: none;
-  }
+.content details summary .pop-out i:hover {
+  color: var(--icons-hover);
 }
diff --git a/speaker-notes.js b/speaker-notes.js
index 22d353ef..e856c4ce 100644
--- a/speaker-notes.js
+++ b/speaker-notes.js
@@ -78,9 +78,9 @@
     popIn.setAttribute("title", "Close speaker notes");
     popIn.setAttribute("aria-label", "Close speaker notes");
     popIn.classList.add("icon-button");
-    let i = document.createElement("i");
-    i.classList.add("fa", "fa-window-close-o");
-    popIn.append(i);
+    let popInIcon = document.createElement("i");
+    popInIcon.classList.add("fa", "fa-window-close-o");
+    popIn.append(popInIcon);
     popIn.addEventListener("click", (event) => {
       setState("inline-open");
       applyState();
@@ -108,10 +108,20 @@
     // Create pop-out button.
     let popOutLocation = new URL(window.location.href);
     popOutLocation.hash = "#speaker-notes-open";
-    let popOut = document.createElement("a");
-    popOut.setAttribute("href", popOutLocation.href);
-    popOut.setAttribute("target", "speakerNotes");
-    popOut.classList.add("fa", "fa-external-link");
+    let popOut = document.createElement("button");
+    popOut.classList.add("icon-button", "pop-out");
+    popOut.addEventListener("click", (event) => {
+      let popup = window.open(popOutLocation.href, "speakerNotes", "popup");
+      if (popup) {
+        setState("popup");
+        applyState();
+      } else {
+        window.alert("Could not open popup, please check your popup blocker settings.");
+      }
+    })
+    let popOutIcon = document.createElement("i");
+    popOutIcon.classList.add("fa", "fa-external-link");
+    popOut.append(popOutIcon);
     summary.append(popOut);
   }
 
@@ -129,11 +139,6 @@
 
   // Create controls for a speaker note window.
   function setupSpeakerNotes() {
-    // Show the notes inline again when the window is closed.
-    window.addEventListener("pagehide", (event) => {
-      setState("inline-open");
-    });
-
     // Hide sidebar and buttons.
     document.querySelector("html").classList.remove("sidebar-visible");
     document.querySelector("html").classList.add("sidebar-hidden");
@@ -215,9 +220,7 @@
   // We encode the kind of page in the location hash:
   switch (window.location.hash) {
     case "#speaker-notes-open":
-      // We are on a page in the speaker notes. We need to re-establish the
-      // popup state so that the main window will hide the notes.
-      setState("popup");
+      // We are on a page in the speaker notes.
       setupSpeakerNotes();
       break;
     case "#speaker-notes-defunct":