Fix screenshot popup hide (#753)

* Refactor Popup.setVisibleOverride

* Use event to observe visibility changes

* Add setAllVisibleOverride/clearAllVisibleOverride

* Add setAllVisibleOverride/clearAllVisibleOverride cross frame handlers

* Update how visibility is changed

* Wait for next frame to ensure visibility has been updated

5 files changed, 104 insertions, 33 deletions

diff --git a/ext/fg/js/frontend.js b/ext/fg/js/frontend.js
index caa2bd3e..61789473 100644
--- a/ext/fg/js/frontend.js
+++ b/ext/fg/js/frontend.js
@@ -62,7 +62,6 @@ class Frontend {
         this._updatePopupToken = null;
 
         this._runtimeMessageHandlers = new Map([
-            ['popupSetVisibleOverride',              {async: false, handler: this._onMessagePopupSetVisibleOverride.bind(this)}],
             ['requestFrontendReadyBroadcast',        {async: false, handler: this._onMessageRequestFrontendReadyBroadcast.bind(this)}]
         ]);
     }
@@ -108,11 +107,13 @@ class Frontend {
         this._textScanner.on('activeModifiersChanged', this._onActiveModifiersChanged.bind(this));
 
         api.crossFrame.registerHandlers([
-            ['getUrl',                 {async: false, handler: this._onApiGetUrl.bind(this)}],
-            ['closePopup',             {async: false, handler: this._onApiClosePopup.bind(this)}],
-            ['copySelection',          {async: false, handler: this._onApiCopySelection.bind(this)}],
-            ['getPopupInfo',           {async: false, handler: this._onApiGetPopupInfo.bind(this)}],
-            ['getDocumentInformation', {async: false, handler: this._onApiGetDocumentInformation.bind(this)}]
+            ['getUrl',                  {async: false, handler: this._onApiGetUrl.bind(this)}],
+            ['closePopup',              {async: false, handler: this._onApiClosePopup.bind(this)}],
+            ['copySelection',           {async: false, handler: this._onApiCopySelection.bind(this)}],
+            ['getPopupInfo',            {async: false, handler: this._onApiGetPopupInfo.bind(this)}],
+            ['getDocumentInformation',  {async: false, handler: this._onApiGetDocumentInformation.bind(this)}],
+            ['setAllVisibleOverride',   {async: true,  handler: this._onApiSetAllVisibleOverride.bind(this)}],
+            ['clearAllVisibleOverride', {async: true,  handler: this._onApiClearAllVisibleOverride.bind(this)}]
         ]);
 
         this._updateContentScale();
@@ -161,11 +162,6 @@ class Frontend {
 
     // Message handlers
 
-    _onMessagePopupSetVisibleOverride({visible}) {
-        if (this._popup === null) { return; }
-        this._popup.setVisibleOverride(visible);
-    }
-
     _onMessageRequestFrontendReadyBroadcast({frameId}) {
         this._signalFrontendReady(frameId);
     }
@@ -196,6 +192,18 @@ class Frontend {
         };
     }
 
+    async _onApiSetAllVisibleOverride({value, priority, awaitFrame}) {
+        const result = await this._popupFactory.setAllVisibleOverride(value, priority);
+        if (awaitFrame) {
+            await promiseAnimationFrame(100);
+        }
+        return result;
+    }
+
+    async _onApiClearAllVisibleOverride({token}) {
+        return await this._popupFactory.clearAllVisibleOverride(token);
+    }
+
     // Private
 
     _onResize() {
diff --git a/ext/fg/js/popup-factory.js b/ext/fg/js/popup-factory.js
index 03ac603b..27bd49cd 100644
--- a/ext/fg/js/popup-factory.js
+++ b/ext/fg/js/popup-factory.js
@@ -27,6 +27,7 @@ class PopupFactory {
         this._frameId = frameId;
         this._frameOffsetForwarder = new FrameOffsetForwarder(frameId);
         this._popups = new Map();
+        this._allPopupVisibilityTokenMap = new Map();
     }
 
     // Public functions
@@ -39,6 +40,7 @@ class PopupFactory {
             ['hide',                 {async: false, handler: this._onApiHide.bind(this)}],
             ['isVisible',            {async: true,  handler: this._onApiIsVisibleAsync.bind(this)}],
             ['setVisibleOverride',   {async: true,  handler: this._onApiSetVisibleOverride.bind(this)}],
+            ['clearVisibleOverride', {async: true,  handler: this._onApiClearVisibleOverride.bind(this)}],
             ['containsPoint',        {async: true,  handler: this._onApiContainsPoint.bind(this)}],
             ['showContent',          {async: true,  handler: this._onApiShowContent.bind(this)}],
             ['setCustomCss',         {async: false, handler: this._onApiSetCustomCss.bind(this)}],
@@ -108,6 +110,40 @@ class PopupFactory {
         }
     }
 
+    async setAllVisibleOverride(value, priority) {
+        const promises = [];
+        const errors = [];
+        for (const popup of this._popups.values()) {
+            const promise = popup.setVisibleOverride(value, priority)
+                .then(
+                    (token) => ({popup, token}),
+                    (error) => { errors.push(error); return null; }
+                );
+            promises.push(promise);
+        }
+
+        const results = await Promise.all(promises);
+
+        if (errors.length === 0) {
+            const token = generateId(16);
+            this._allPopupVisibilityTokenMap.set(token, results);
+            return token;
+        }
+
+        // Revert on error
+        await this._revertPopupVisibilityOverrides(results);
+        throw errors[0];
+    }
+
+    async clearAllVisibleOverride(token) {
+        const results = this._allPopupVisibilityTokenMap.get(token);
+        if (typeof results === 'undefined') { return false; }
+
+        this._allPopupVisibilityTokenMap.delete(token);
+        await this._revertPopupVisibilityOverrides(results);
+        return true;
+    }
+
     // API message handlers
 
     async _onApiGetOrCreatePopup({id, parentPopupId, frameId, ownerFrameId}) {
@@ -134,9 +170,14 @@ class PopupFactory {
         return await popup.isVisible();
     }
 
-    async _onApiSetVisibleOverride({id, visible}) {
+    async _onApiSetVisibleOverride({id, value, priority}) {
+        const popup = this._getPopup(id);
+        return await popup.setVisibleOverride(value, priority);
+    }
+
+    async _onApiClearVisibleOverride({id, token}) {
         const popup = this._getPopup(id);
-        return await popup.setVisibleOverride(visible);
+        return await popup.clearVisibleOverride(token);
     }
 
     async _onApiContainsPoint({id, x, y}) {
@@ -216,4 +257,19 @@ class PopupFactory {
         const parent = popup.parent;
         return parent === null || parent.isVisibleSync();
     }
+
+    async _revertPopupVisibilityOverrides(overrides) {
+        const promises = [];
+        for (const value of overrides) {
+            if (value === null) { continue; }
+            const {popup, token} = value;
+            const promise = popup.clearVisibleOverride(token)
+                .then(
+                    (v) => v,
+                    () => false
+                );
+            promises.push(promise);
+        }
+        return await Promise.all(promises);
+    }
 }
diff --git a/ext/fg/js/popup-proxy.js b/ext/fg/js/popup-proxy.js
index f1b3ab21..efbc72a6 100644
--- a/ext/fg/js/popup-proxy.js
+++ b/ext/fg/js/popup-proxy.js
@@ -86,8 +86,12 @@ class PopupProxy extends EventDispatcher {
         return this._invokeSafe('isVisible', {id: this._id}, false);
     }
 
-    setVisibleOverride(visible) {
-        return this._invokeSafe('setVisibleOverride', {id: this._id, visible});
+    async setVisibleOverride(value, priority) {
+        return this._invokeSafe('setVisibleOverride', {id: this._id, value, priority});
+    }
+
+    async clearVisibleOverride(token) {
+        return this._invokeSafe('clearVisibleOverride', {id: this._id, token});
     }
 
     async containsPoint(x, y) {
diff --git a/ext/fg/js/popup.js b/ext/fg/js/popup.js
index bbf39c4e..f644ee98 100644
--- a/ext/fg/js/popup.js
+++ b/ext/fg/js/popup.js
@@ -34,8 +34,7 @@ class Popup extends EventDispatcher {
         this._childrenSupported = true;
         this._injectPromise = null;
         this._injectPromiseComplete = false;
-        this._visible = false;
-        this._visibleOverride = null;
+        this._visible = new DynamicProperty(false);
         this._options = null;
         this._optionsContext = null;
         this._contentScale = 1.0;
@@ -96,11 +95,12 @@ class Popup extends EventDispatcher {
     // Public functions
 
     prepare() {
-        this._updateVisibility();
         this._frame.addEventListener('mousedown', (e) => e.stopPropagation());
         this._frame.addEventListener('scroll', (e) => e.stopPropagation());
         this._frame.addEventListener('load', this._onFrameLoad.bind(this));
+        this._visible.on('change', this._onVisibleChange.bind(this));
         yomichan.on('extensionUnloaded', this._onExtensionUnloaded.bind(this));
+        this._onVisibleChange({value: this.isVisibleSync()});
     }
 
     async setOptionsContext(optionsContext, source) {
@@ -131,9 +131,12 @@ class Popup extends EventDispatcher {
         return this.isVisibleSync();
     }
 
-    setVisibleOverride(visible) {
-        this._visibleOverride = visible;
-        this._updateVisibility();
+    async setVisibleOverride(value, priority) {
+        return this._visible.setOverride(value, priority);
+    }
+
+    async clearVisibleOverride(token) {
+        return this._visible.clearOverride(token);
     }
 
     async containsPoint(x, y) {
@@ -177,7 +180,7 @@ class Popup extends EventDispatcher {
     }
 
     isVisibleSync() {
-        return (this._visibleOverride !== null ? this._visibleOverride : this._visible);
+        return this._visible.value;
     }
 
     updateTheme() {
@@ -392,12 +395,11 @@ class Popup extends EventDispatcher {
     }
 
     _setVisible(visible) {
-        this._visible = visible;
-        this._updateVisibility();
+        this._visible.defaultValue = visible;
     }
 
-    _updateVisibility() {
-        this._frame.style.setProperty('visibility', this.isVisibleSync() ? 'visible' : 'hidden', 'important');
+    _onVisibleChange({value}) {
+        this._frame.style.setProperty('visibility', value ? 'visible' : 'hidden', 'important');
     }
 
     _focusParent() {
diff --git a/ext/mixed/js/display.js b/ext/mixed/js/display.js
index c3498e85..6e41a190 100644
--- a/ext/mixed/js/display.js
+++ b/ext/mixed/js/display.js
@@ -1145,9 +1145,12 @@ class Display extends EventDispatcher {
     }
 
     async _getScreenshot() {
+        const ownerFrameId = this._ownerFrameId;
+        let token = null;
         try {
-            await this._setPopupVisibleOverride(false);
-            await promiseTimeout(1); // Wait for popup to be hidden.
+            if (ownerFrameId !== null) {
+                token = await api.crossFrame.invoke(ownerFrameId, 'setAllVisibleOverride', {value: false, priority: 0, awaitFrame: true});
+            }
 
             const {format, quality} = this._options.anki.screenshot;
             const dataUrl = await api.screenshotGet({format, quality});
@@ -1155,7 +1158,9 @@ class Display extends EventDispatcher {
 
             return {dataUrl, format};
         } finally {
-            await this._setPopupVisibleOverride(null);
+            if (token !== null) {
+                await api.crossFrame.invoke(ownerFrameId, 'clearAllVisibleOverride', {token});
+            }
         }
     }
 
@@ -1163,10 +1168,6 @@ class Display extends EventDispatcher {
         return this._options.general.resultOutputMode === 'merge' ? 0 : -1;
     }
 
-    _setPopupVisibleOverride(visible) {
-        return api.broadcastTab('popupSetVisibleOverride', {visible});
-    }
-
     _getEntry(index) {
         const entries = this._container.querySelectorAll('.entry');
         return index >= 0 && index < entries.length ? entries[index] : null;