Popup live toggle #439

siikamiika · 2020-04-10T12:58:17Z

Fixes #431

siikamiika · 2020-04-10T13:08:31Z

There should probably be a forced settings update when using the root frame popup after using it from an iframe, but I haven't investigated it yet

toasted-nutbread · 2020-04-10T14:25:27Z

ext/fg/js/popup-nested.js

+let popupNestedInitialized = false;
+
+async function popupNestedInitialize(id, depth, parentFrameId, url) {
+    if (popupNestedInitialized) {


I don't think popupNestedInitialized is necessary anymore. Please remove.

It's called from float.js, but I can't think of cases when it would actually be called multiple times except maybe reloading the extension, and this check wouldn't help with that. Maybe there should be a check similar to the one in search-frontend.js

const node = document.querySelector(`script[src='${src}']`); if (node !== null) { continue; }

Yeah, that prepare should only be called once due to the _prepareInvoked guard, so that should handle it.

toasted-nutbread · 2020-04-10T14:29:16Z

ext/fg/js/frontend-initialize.js

+
+    const isIframe = !proxy && (window !== window.parent);
+
+    const initEventDispatcher = new EventDispatcher();


The use of initEventDispatcher can probably be removed by some simplification.

initEventDispatcher is only used by frontend, which is only instantiated once. Add a setPopup method on it for popupChange, and setDisabledOverride can probably be handled in a similar way, or by setting the popup to null.

44ff4d8

I'd rather not set popup to null because then I'd have to potentially handle nulls all the places where popup is used.

toasted-nutbread · 2020-04-10T14:40:42Z

ext/mixed/js/text-scanner.js

@@ -23,11 +23,13 @@
 */

 class TextScanner {
-    constructor(node, ignoreElements, ignorePoints) {
+    constructor(node, ignoreElements, ignorePoints, canEnable=null) {


Rather than adding canEnable, might be better to add it as an argument to setEnabled. setEnabled is only called from TextScanner.setOptions, which is called from the subclasses of TextScanner (i.e. Frontend).

Meaning you can just pass this.popup.depth <= this.options.scanning.popupNestingMaxDepth && !this._disabledOverride here:

yomichan/ext/fg/js/frontend.js

Line 147 in 791c51f

this.setOptions(await apiOptionsGet(this.getOptionsContext()));

And true here (or default value):

yomichan/ext/bg/js/search-query-parser.js

Line 109 in 791c51f

super.setOptions(options);

This should give better control over when the object is enabled/disabled.

siikamiika · 2020-04-11T13:24:05Z

Seems like something's up with the CI, none of the tests go through

toasted-nutbread · 2020-04-11T14:33:53Z

Seems like something's up with the CI, none of the tests go through

Re-running it manually seems to have worked. ¯\_(ツ)_/¯

siikamiika · 2020-04-11T21:19:33Z

There should probably be a forced settings update when using the root frame popup after using it from an iframe, but I haven't investigated it yet

Actually, the settings should be the same regardless of the iframe proxy because the context is the same there with the current profile conditions. If a refresh is needed, something like this could be done:

diff --git a/ext/fg/js/frontend.js b/ext/fg/js/frontend.js
index 20bfc638..df038e24 100644
--- a/ext/fg/js/frontend.js
+++ b/ext/fg/js/frontend.js
@@ -247,13 +247,20 @@ class Frontend extends TextScanner {
     }
 
     _showPopupContent(textSource, type=null, details=null) {
-        this._lastShowPromise = this.popup.showContent(
-            textSource.getRect(),
-            textSource.getWritingMode(),
-            type,
-            details
+        let promise = this._lastShowPromise;
+        if (!this.popup.isProxy() && this.popup.usedByProxy()) {
+            this.popup.setUsedByProxy(false);
+            promise = promise.then(() => this.popup.setOptions(this.options));
+        }
+
+        this._lastShowPromise = promise.then(
+            () => this.popup.showContent(
+                textSource.getRect(),
+                textSource.getWritingMode(),
+                type,
+                details
+            )
         );
-        return this._lastShowPromise;
     }
 
     _updateContentScale() {
diff --git a/ext/fg/js/popup-proxy-host.js b/ext/fg/js/popup-proxy-host.js
index 958462ff..2aa0e523 100644
--- a/ext/fg/js/popup-proxy-host.js
+++ b/ext/fg/js/popup-proxy-host.js
@@ -158,6 +158,7 @@ class PopupProxyHost {
         if (typeof popup === 'undefined') {
             throw new Error(`Invalid popup ID ${id}`);
         }
+        popup.setUsedByProxy(true);
         return popup;
     }
 
diff --git a/ext/fg/js/popup.js b/ext/fg/js/popup.js
index 42f08afa..4118368d 100644
--- a/ext/fg/js/popup.js
+++ b/ext/fg/js/popup.js
@@ -36,6 +36,7 @@ class Popup {
         this._containerSizeContentScale = null;
         this._targetOrigin = chrome.runtime.getURL('/').replace(/\/$/, '');
         this._messageToken = null;
+        this._usedByProxy = false;
 
         this._container = document.createElement('iframe');
         this._container.className = 'yomichan-float';
@@ -81,6 +82,14 @@ class Popup {
         return false;
     }
 
+    usedByProxy() {
+        return this._usedByProxy;
+    }
+
+    setUsedByProxy(used) {
+        this._usedByProxy = used;
+    }
+
     async setOptions(options) {
         this._options = options;
         this.updateTheme();

siikamiika · 2020-04-12T14:10:25Z

It's a bit ambiguous what should be done with the url of the optionsContext for the content script. When a content script is injected into an iframe, it could be either the URL of the iframe or the root frame. When iframe popups are set to show in the root frame, it could make sense to abstract iframes away from the user. To make things more complicated, the decision whether to show iframe popups in the root frame could be affected by the current URL.

This was broken when merging #417, but I've addressed the issue in b0d94e8. I can change it to window.location.href if that makes more sense. In that case a new change similar to the diff above is needed to keep the popup options in sync.

toasted-nutbread · 2020-04-12T14:55:46Z

ext/fg/js/frontend-initialize.js

-                if (action === 'rootPopupInformation') {
-                    resolve(params);
-                }
+    const {id, depth=0, parentFrameId, url=window.top.location.href, proxy=false, isSearchPage=false} = data;


window.top.location.href can have an exception. I tested manually on https://dic.nicovideo.jp/'s twitter widget and get the following error when trying to read it from its context:

Blocked a frame with origin "https://platform.twitter.com" from accessing a cross-origin frame.

I wonder what's going to happen with the line below:

const isIframe = !proxy && (window !== window.parent);

I tested and that doesn't throw an error

It seems that there's still an issue with DisplayFloat's optionsContext. Fixing that would be quite complicated, and not necessarily something I want to do in this PR. Should I revert b80fd00 and b0d94e8 and create a new commit making the default value window.location.href?

Makes sense to me.

toasted-nutbread · 2020-04-12T15:45:31Z

ext/fg/js/frontend-initialize.js

+}
+
+async function getOrCreatePopup(depth) {
+    const frameOffsetForwarder = new FrameOffsetForwarder();


Two FrameOffsetForwarder instances can exist at the same time this way; one from createIframePopupProxy and one from getOrCreatePopup.

toasted-nutbread · 2020-04-12T17:10:49Z

Noticed an issue:

Open Chrome.
Open test-document2.html in a new tab.
Open a popup by scanning content in test 1.
Open the options tab.
Change the value of Maximum number of additional popups or Show iframe popups in root frame.
test-document2.html hijacks the tab focus from the options tab.

Appears to have something to do with the iframes, since it doesn't occur on documents without iframes.

toasted-nutbread · 2020-04-12T17:17:51Z

Another issue I noticed, which may be a timing issue. Not as critical, but not sure how complex this would be to fix.

Change Maximum number of additional popups to some large value like 10.
Open a document, scan text, open all popups.
Change value of Maximum number of additional popups to 1. All popups will remain open.
Change value of Maximum number of additional popups to 2. Most of the popups will now close.

The options will use the old value of popupNestingMaxDepth.

For testing, on line 290 I added:

console.log('_canEnable', this.options, this.popup.depth, '<=', this.options !== null ? this.options.scanning.popupNestingMaxDepth : null, this._disabledOverride);

Logged this:

// Initially opening all popups
frontend.js:290 _canEnable null 0 <= null false
frontend.js:290 _canEnable null 1 <= null false
frontend.js:290 _canEnable null 2 <= null false
frontend.js:290 _canEnable null 3 <= null false
frontend.js:290 _canEnable null 4 <= null false
frontend.js:290 _canEnable null 5 <= null false
frontend.js:290 _canEnable null 6 <= null false
frontend.js:290 _canEnable null 7 <= null false
frontend.js:290 _canEnable null 8 <= null false
frontend.js:290 _canEnable null 9 <= null false
frontend.js:290 _canEnable null 10 <= null false
// Changed value of popupNestingMaxDepth to 1
frontend.js:290 _canEnable {…} 0 <= 10 false
frontend.js:290 _canEnable {…} 1 <= 10 false
frontend.js:290 _canEnable {…} 7 <= 10 false
frontend.js:290 _canEnable {…} 6 <= 10 false
frontend.js:290 _canEnable {…} 4 <= 10 false
frontend.js:290 _canEnable {…} 2 <= 10 false
frontend.js:290 _canEnable {…} 5 <= 10 false
frontend.js:290 _canEnable {…} 3 <= 10 false
frontend.js:290 _canEnable {…} 10 <= 10 false
frontend.js:290 _canEnable {…} 8 <= 10 false
frontend.js:290 _canEnable {…} 9 <= 10 false
// Changed value of popupNestingMaxDepth to 2
frontend.js:290 _canEnable {…} 0 <= 1 false
frontend.js:290 _canEnable {…} 1 <= 1 false
frontend.js:290 _canEnable {…} 7 <= 1 false
frontend.js:290 _canEnable {…} 6 <= 1 false
frontend.js:290 _canEnable {…} 4 <= 1 false
frontend.js:290 _canEnable {…} 2 <= 1 false
frontend.js:290 _canEnable {…} 5 <= 1 false
frontend.js:290 _canEnable {…} 3 <= 1 false
frontend.js:290 _canEnable {…} 10 <= 1 false
frontend.js:290 _canEnable {…} 8 <= 1 false
frontend.js:290 _canEnable {…} 9 <= 1 false

siikamiika · 2020-04-14T17:35:13Z

Noticed an issue:

1. Open Chrome.

2. Open [test-document2.html](https://github.com/FooSoft/yomichan/blob/master/test/data/html/test-document2.html) in a new tab.

3. Open a popup by scanning content in test 1.

4. Open the options tab.

5. Change the value of **Maximum number of additional popups** or **Show iframe popups in root frame**.

6. test-document2.html hijacks the tab focus from the options tab.

Appears to have something to do with the iframes, since it doesn't occur on documents without iframes.

This happens when https://github.com/FooSoft/yomichan/pull/439/files#diff-c0a35793e5728d9d3e4b1301d23a4c3bR113-R115 is run because Frontend.setPopup calls this.onSearchClear(true);. I can change it to false, but it could lead to hiding a popup so that it's still focused. I guess forcefully focusing the tab is worse. I remember we had a similar issue in the past, but it was fixed somehow.

siikamiika · 2020-04-14T18:01:16Z

Another issue I noticed, which may be a timing issue. Not as critical, but not sure how complex this would be to fix.

1. Change **Maximum number of additional popups** to some large value like 10.

2. Open a document, scan text, open all popups.

3. Change value of **Maximum number of additional popups** to 1. All popups will remain open.

4. Change value of **Maximum number of additional popups**  to 2. Most of the popups will now close.

The options will use the old value of popupNestingMaxDepth.

For testing, on line 290 I added:

console.log('_canEnable', this.options, this.popup.depth, '<=', this.options !== null ? this.options.scanning.popupNestingMaxDepth : null, this._disabledOverride);

Logged this:

// Initially opening all popups
frontend.js:290 _canEnable null 0 <= null false
frontend.js:290 _canEnable null 1 <= null false
frontend.js:290 _canEnable null 2 <= null false
frontend.js:290 _canEnable null 3 <= null false
frontend.js:290 _canEnable null 4 <= null false
frontend.js:290 _canEnable null 5 <= null false
frontend.js:290 _canEnable null 6 <= null false
frontend.js:290 _canEnable null 7 <= null false
frontend.js:290 _canEnable null 8 <= null false
frontend.js:290 _canEnable null 9 <= null false
frontend.js:290 _canEnable null 10 <= null false
// Changed value of popupNestingMaxDepth to 1
frontend.js:290 _canEnable {…} 0 <= 10 false
frontend.js:290 _canEnable {…} 1 <= 10 false
frontend.js:290 _canEnable {…} 7 <= 10 false
frontend.js:290 _canEnable {…} 6 <= 10 false
frontend.js:290 _canEnable {…} 4 <= 10 false
frontend.js:290 _canEnable {…} 2 <= 10 false
frontend.js:290 _canEnable {…} 5 <= 10 false
frontend.js:290 _canEnable {…} 3 <= 10 false
frontend.js:290 _canEnable {…} 10 <= 10 false
frontend.js:290 _canEnable {…} 8 <= 10 false
frontend.js:290 _canEnable {…} 9 <= 10 false
// Changed value of popupNestingMaxDepth to 2
frontend.js:290 _canEnable {…} 0 <= 1 false
frontend.js:290 _canEnable {…} 1 <= 1 false
frontend.js:290 _canEnable {…} 7 <= 1 false
frontend.js:290 _canEnable {…} 6 <= 1 false
frontend.js:290 _canEnable {…} 4 <= 1 false
frontend.js:290 _canEnable {…} 2 <= 1 false
frontend.js:290 _canEnable {…} 5 <= 1 false
frontend.js:290 _canEnable {…} 3 <= 1 false
frontend.js:290 _canEnable {…} 10 <= 1 false
frontend.js:290 _canEnable {…} 8 <= 1 false
frontend.js:290 _canEnable {…} 9 <= 1 false

Caused by calling _canEnable before it got the newest options. Fixed in b6f7f8c

toasted-nutbread reviewed Apr 10, 2020

View reviewed changes

siikamiika added 13 commits April 11, 2020 21:01

listen to optionsUpdated in search-frontend.js

8c9389e

listen to optionsUpdated in popup-nested.js

9adbc80

prevent injecting frontend multiple times

61a96e3

limit popup depth live

1df59d5

allow disabling scan on search page live

92109bb

extract different popup creation functions

7dd2610

cache created popups

b903585

change iframe popup live

565e3a8

fix preview frame error

275f455

use setters instead of EventDispatcher

d93e3e1

simplify frontend disable override

5c3641e

fix ignoreElements in QueryParser

ba97e72

remove redundant call guard

e6078ce

siikamiika force-pushed the popup-live-toggle branch from 98d7e09 to e6078ce Compare April 11, 2020 18:06

toasted-nutbread reviewed Apr 12, 2020

View reviewed changes

fix optionsContext URL for root popup

e627ab2

siikamiika force-pushed the popup-live-toggle branch from b80fd00 to e627ab2 Compare April 12, 2020 16:11

prevent spawning multiple FrameOffsetForwarders

362ac84

siikamiika mentioned this pull request Apr 12, 2020

Use Frontend's options for Popup and DisplayFloat #452

Closed

don't focus tab when changing settings

950f50e

fix _canEnable being used before setting options

b6f7f8c

toasted-nutbread approved these changes Apr 15, 2020

View reviewed changes

siikamiika merged commit e6053ee into FooSoft:master Apr 16, 2020

toasted-nutbread mentioned this pull request Apr 18, 2020

Changelog #376

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Popup live toggle #439

Popup live toggle #439

siikamiika commented Apr 10, 2020

siikamiika commented Apr 10, 2020

toasted-nutbread Apr 10, 2020

siikamiika Apr 11, 2020

toasted-nutbread Apr 11, 2020

siikamiika Apr 11, 2020

toasted-nutbread Apr 10, 2020

siikamiika Apr 11, 2020

toasted-nutbread Apr 10, 2020

siikamiika Apr 11, 2020

siikamiika commented Apr 11, 2020

toasted-nutbread commented Apr 11, 2020

siikamiika commented Apr 11, 2020 •

edited

Loading

siikamiika commented Apr 12, 2020

toasted-nutbread Apr 12, 2020

siikamiika Apr 12, 2020

siikamiika Apr 12, 2020

siikamiika Apr 12, 2020

toasted-nutbread Apr 12, 2020

toasted-nutbread Apr 12, 2020

siikamiika Apr 12, 2020

toasted-nutbread commented Apr 12, 2020 •

edited

Loading

toasted-nutbread commented Apr 12, 2020

siikamiika commented Apr 14, 2020

siikamiika commented Apr 14, 2020


		const isIframe = !proxy && (window !== window.parent);

		const initEventDispatcher = new EventDispatcher();

Popup live toggle #439

Popup live toggle #439

Conversation

siikamiika commented Apr 10, 2020

siikamiika commented Apr 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siikamiika commented Apr 11, 2020

toasted-nutbread commented Apr 11, 2020

siikamiika commented Apr 11, 2020 • edited Loading

siikamiika commented Apr 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toasted-nutbread commented Apr 12, 2020 • edited Loading

toasted-nutbread commented Apr 12, 2020

siikamiika commented Apr 14, 2020

siikamiika commented Apr 14, 2020

siikamiika commented Apr 11, 2020 •

edited

Loading

toasted-nutbread commented Apr 12, 2020 •

edited

Loading