Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "Hide popup on cursor exit" setting conflicting with "Allow scanning popup content" nested popups (same as #1824) #1830

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 55 additions & 9 deletions ext/js/app/frontend.js
Original file line number Diff line number Diff line change
Expand Up @@ -424,17 +424,17 @@ export class Frontend {
*/
_onPopupFramePointerOver() {
this._isPointerOverPopup = true;
this._stopClearSelectionDelayed();
}

/**
* @returns {void}
*/
_onPopupFramePointerOut() {
this._isPointerOverPopup = false;
const scanningOptions = /** @type {import('settings').ProfileOptions} */ (this._options).scanning;
if (scanningOptions.hidePopupOnCursorExit) {
this._clearSelectionDelayed(scanningOptions.hidePopupOnCursorExitDelay, false, false);
if (!this._options) { return; }
const {scanning: {hidePopupOnCursorExit, hidePopupOnCursorExitDelay}} = this._options;
if (hidePopupOnCursorExit) {
void this._clearSelectionDelayed(hidePopupOnCursorExitDelay, false, false);
}
}

Expand All @@ -456,19 +456,65 @@ export class Frontend {
this._textScanner.clearMousePosition();
}

/**
* Checks if the pointer is over any popup in the hierarchy (parent or child popups).
* @returns {Promise<boolean>}
* @private
*/
async _isPointerOverAnyPopup() {
if (this._isPointerOverPopup) {
return true;
}

let childPopup = this._popup?.child;
while (childPopup !== null && childPopup !== undefined) {
try {
const isOver = await childPopup.isPointerOver();
if (isOver) {
return true;
}
childPopup = childPopup.child;
} catch (e) {
console.warn('Error checking child popup pointer state:', e);
}
}

let parentPopup = this._popup?.parent;
while (parentPopup !== null && parentPopup !== undefined) {
try {
const isOver = await parentPopup.isPointerOver();
if (isOver) {
return true;
}
parentPopup = parentPopup.parent;
} catch (e) {
console.warn('Error checking parent popup pointer state:', e);
}
}

return false;
}

/**
* @param {number} delay
* @param {boolean} restart
* @param {boolean} passive
*/
_clearSelectionDelayed(delay, restart, passive) {
async _clearSelectionDelayed(delay, restart, passive) {
if (!this._textScanner.hasSelection()) { return; }

// Add a small delay to allow mouseover events to be processed
await new Promise(resolve => setTimeout(resolve, 50));

// Always check if pointer is over any popup before clearing
if (await this._isPointerOverAnyPopup()) { return; }

if (delay > 0) {
if (this._clearSelectionTimer !== null && !restart) { return; } // Already running
this._stopClearSelectionDelayed();
this._clearSelectionTimer = setTimeout(() => {
this._clearSelectionTimer = setTimeout(async () => {
this._clearSelectionTimer = null;
if (this._isPointerOverPopup) { return; }
if (await this._isPointerOverAnyPopup()) { return; }
this._clearSelection(passive);
}, delay);
} else {
Expand Down Expand Up @@ -595,8 +641,8 @@ export class Frontend {
this._popupEventListeners.removeAllEventListeners();
this._popup = popup;
if (popup !== null) {
this._popupEventListeners.on(popup, 'framePointerOver', this._onPopupFramePointerOver.bind(this));
this._popupEventListeners.on(popup, 'framePointerOut', this._onPopupFramePointerOut.bind(this));
this._popupEventListeners.on(popup, 'mouseOver', this._onPopupFramePointerOver.bind(this));
this._popupEventListeners.on(popup, 'mouseOut', this._onPopupFramePointerOut.bind(this));
}
this._isPointerOverPopup = false;
}
Expand Down
7 changes: 7 additions & 0 deletions ext/js/app/popup-factory.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export class PopupFactory {
['popupFactorySetCustomOuterCss', this._onApiSetCustomOuterCss.bind(this)],
['popupFactoryGetFrameSize', this._onApiGetFrameSize.bind(this)],
['popupFactorySetFrameSize', this._onApiSetFrameSize.bind(this)],
['popupFactoryIsPointerOver', this._onApiIsPointerOver.bind(this)],
]);
/* eslint-enable @stylistic/no-multi-spaces */
}
Expand Down Expand Up @@ -351,6 +352,12 @@ export class PopupFactory {
return await popup.setFrameSize(width, height);
}

/** @type {import('cross-frame-api').ApiHandler<'popupFactoryIsPointerOver'>} */
async _onApiIsPointerOver({id}) {
const popup = this._getPopup(id);
return popup.isPointerOver();
}

// Private functions

/**
Expand Down
9 changes: 9 additions & 0 deletions ext/js/app/popup-proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,15 @@ export class PopupProxy extends EventDispatcher {
return this._invokeSafe('popupFactorySetFrameSize', {id: this._id, width, height}, false);
}

/**
* Checks if the pointer is over this popup.
* @returns {Promise<boolean>} Whether the pointer is over the popup
*/
isPointerOver() {

return this._invokeSafe('popupFactoryIsPointerOver', {id: this._id}, false);
}

// Private

/**
Expand Down
10 changes: 9 additions & 1 deletion ext/js/app/popup-window.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*/

import {EventDispatcher} from '../core/event-dispatcher.js';
import { EventDispatcher } from '../core/event-dispatcher.js';

/**
* This class represents a popup that is hosted in a new native window.
Expand Down Expand Up @@ -263,6 +263,14 @@ export class PopupWindow extends EventDispatcher {
return false;
}

/**
* @returns {Promise<boolean>}
*/
async isPointerOver() {

return false;
}

// Private

/**
Expand Down
53 changes: 49 additions & 4 deletions ext/js/app/popup.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ export class Popup extends EventDispatcher {
this._frame.style.height = '0';
/** @type {boolean} */
this._frameConnected = false;
/** @type {boolean} */
this._isPointerOverPopup = false;

/** @type {HTMLElement} */
this._container = this._frame;
Expand Down Expand Up @@ -385,8 +387,7 @@ export class Popup extends EventDispatcher {
* @returns {Promise<import('popup').ValidSize>} The size and whether or not it is valid.
*/
async getFrameSize() {
const {width, height} = this._getFrameBoundingClientRect();
return {width, height, valid: true};
return {width: this._frame.offsetWidth, height: this._frame.offsetHeight, valid: true};
}

/**
Expand All @@ -400,20 +401,52 @@ export class Popup extends EventDispatcher {
return true;
}

/**
* Returns whether the pointer is currently over this popup.
* @returns {boolean}
*/
isPointerOver() {

return this._isPointerOverPopup;
}

// Private functions

/**
* @returns {void}
*/
_onFrameMouseOver() {
this.trigger('framePointerOver', {});

this._isPointerOverPopup = true;

this.trigger('mouseOver', {});

// Clear all child popups when parent is moused over
let currentChild = this.child;
while (currentChild !== null) {

currentChild.hide(false);
currentChild = currentChild.child;
}
}

/**
* @returns {void}
*/
_onFrameMouseOut() {
this.trigger('framePointerOut', {});

this._isPointerOverPopup = false;

this.trigger('mouseOut', {});


// Propagate mouseOut event up through the entire hierarchy
let currentParent = this.parent;
while (currentParent !== null) {

currentParent.trigger('mouseOut', {});
currentParent = currentParent.parent;
}
}

/**
Expand Down Expand Up @@ -459,10 +492,12 @@ export class Popup extends EventDispatcher {

const useSecurePopupFrameUrl = this._useSecureFrameUrl;


await this._setUpContainer(this._useShadowDom);

/** @type {import('frame-client').SetupFrameFunction} */
const setupFrame = (frame) => {

frame.removeAttribute('src');
frame.removeAttribute('srcdoc');
this._observeFullscreen(true);
Expand All @@ -486,6 +521,15 @@ export class Popup extends EventDispatcher {
await frameClient.connect(this._frame, this._targetOrigin, this._frameId, setupFrame);
this._frameConnected = true;


// Reattach mouse event listeners after frame injection

const boundMouseOver = this._onFrameMouseOver.bind(this);
const boundMouseOut = this._onFrameMouseOut.bind(this);
this._frame.addEventListener('mouseover', boundMouseOver);
this._frame.addEventListener('mouseout', boundMouseOut);


// Configure
/** @type {import('display').DirectApiParams<'displayConfigure'>} */
const configureParams = {
Expand All @@ -497,6 +541,7 @@ export class Popup extends EventDispatcher {
optionsContext: this._optionsContext,
};
await this._invokeSafe('displayConfigure', configureParams);

}

/**
Expand Down
10 changes: 8 additions & 2 deletions types/ext/cross-frame-api.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export type CrossFrameCommunicationPortDetails = {
otherFrameId: number;
};

type ApiSurface = {
export interface ApiSurface {
displayPopupMessage1: {
params: DisplayDirectApiFrameClientMessageAny;
return: DisplayDirectApiReturnAny;
Expand Down Expand Up @@ -234,7 +234,13 @@ type ApiSurface = {
params: RequestFrameInfoResponseParams;
return: RequestFrameInfoResponseReturn;
};
};
popupFactoryIsPointerOver: {
params: {
id: string;
};
return: boolean;
};
}

export type ApiNames = BaseApiNames<ApiSurface>;

Expand Down
4 changes: 2 additions & 2 deletions types/ext/popup.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,8 @@ export type Events = {
useWebExtensionApi: boolean;
inShadow: boolean;
};
framePointerOver: Record<string, never>;
framePointerOut: Record<string, never>;
mouseOver: Record<string, never>;
mouseOut: Record<string, never>;
offsetNotFound: Record<string, never>;
};

Expand Down
Loading