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

Browser agnostic search page clipboard and selection handling #965

Merged
merged 1 commit into from
May 21, 2024

Conversation

Kuuuube
Copy link
Member

@Kuuuube Kuuuube commented May 18, 2024

Due to a 23 year old Firefox bug (https://bugzilla.mozilla.org/show_bug.cgi?id=85686) window.getSelection() does not behave the same on Firefox as it does on Chromium.

It is not possible to get the current selection using this if a textarea or input is where the selection is located on Firefox. It is possible to achieve this using other methods but none of these methods work for fetching the selection anywhere on the page. So there is no equivalent of window.getSelection() to be used.

To avoid jank I've rewritten the code to check whether the user has the browser page focused instead using document.hasFocus(). If the user has the browser page focused, updating the search from a clipboard copy is blocked.

I searched the blame for this code back to when it was added to check if there were any edge cases this was handling but there doesn't appear to be any specifc reason window.getSelection() was chosen so it should be fine to switch out.

The Yomitan bug this fixes is the search page updating when copying out of the textarea search box on the search page on Firefox.

Tested on Firefox and Chromium.

@Kuuuube Kuuuube added kind/bug The issue or PR is regarding a bug area/ui-ux The issue or PR is related to UI/UX/Design labels May 18, 2024
@Kuuuube Kuuuube requested a review from a team as a code owner May 18, 2024 19:24
Copy link
Collaborator

@jamesmaa jamesmaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I barely know what's going on so just commenting for my own edification

@@ -66,11 +66,11 @@ export class SearchDisplayController {
/** @type {boolean} */
this._clipboardMonitorEnabled = false;
/** @type {import('clipboard-monitor').ClipboardReaderLike} */
const clipboardReader = {
this.clipboardReaderLike = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does Like mean here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes it clear that this isnt an instance of the clipboardReader class and doesn't have all the function of it but has similar base function. Similar to an "arraylike" type where theres an array that might be able to do the array storage stuff but it might not be able to do push() or pop() for example.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other languages you would accomplish this by typing an interface but ig js doesn't have this pattern

@@ -268,10 +268,9 @@ export class SearchDisplayController {
}

/** */
_onCopy() {
async _onCopy() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change this to async? I would've thought a function signature change would require changing the call sites of _onCopy as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_onCopy runs based off of an event listener. Event listener calls are essentially voids.

@jamesmaa jamesmaa added this pull request to the merge queue May 21, 2024
Merged via the queue into themoeway:master with commit 654bb75 May 21, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui-ux The issue or PR is related to UI/UX/Design kind/bug The issue or PR is regarding a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants