From 773e28aa3c23c1cb93d5b19b10c49fcec19487a9 Mon Sep 17 00:00:00 2001 From: toasted-nutbread Date: Sun, 23 Aug 2020 15:18:41 -0400 Subject: 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 --- ext/fg/js/frontend.js | 30 ++++++++++++++--------- ext/fg/js/popup-factory.js | 60 ++++++++++++++++++++++++++++++++++++++++++++-- ext/fg/js/popup-proxy.js | 8 +++++-- ext/fg/js/popup.js | 24 ++++++++++--------- ext/mixed/js/display.js | 15 ++++++------ 5 files changed, 104 insertions(+), 33 deletions(-) (limited to 'ext') 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; -- cgit v1.2.3