FooSoft / yomichan

Japanese pop-up dictionary extension for Chrome and Firefox.
https://foosoft.net/projects/yomichan
Other
1.06k stars 213 forks source link

Do not change selection if user selected text manually #2186

Closed oakkitten closed 2 years ago

oakkitten commented 2 years ago

While this works for me, please note that I have no idea what I am doing. image Particularly, I am not sure about the setTimeout() bit, and about all the possible edge cases. This also could be slightly optimized by only hooking selectionchange when select text is on, but the extra complexity is probably not worth it.

Closes #2071.

toasted-nutbread commented 2 years ago

This also could be slightly optimized by only hooking selectionchange when select text is on, but the extra complexity is probably not worth it.

We will probably want the extra functionality to run only if _selectText is true, as I may change that setting later on to support both use cases.

oakkitten commented 2 years ago

We will probably want the extra functionality to run only if _selectText is true, as I may change that setting later on to support both use cases.

Not sure I should do anything about it? What I meant was, to only add event listener for selection change if _selectText is true. Given how it can seemingly change while the scanner is active, this will lead to a bit of spaghetti code. I can do that, it's just that optimizations like that tend to have a negative effect later on.

toasted-nutbread commented 2 years ago

Not sure I should do anything about it? What I meant was, to only add event listener for selection change if _selectText is true. Given how it can seemingly change while the scanner is active, this will lead to a bit of spaghetti code. I can do that, it's just that optimizations like that tend to have a negative effect later on.

True, for some reason I thought that this class updated the events listeners each time the settings were updated, but looks like I misremembered. I wouldn't worry about this then.

toasted-nutbread commented 2 years ago

The only other edges case I can think of that might conflict with this and/or potentially confuse the user is if the website has an invisible selection or some external code selects text, which isn't truly a user selection but would be detected as such. I don't think these can be worked around if this behaviour is always active though.

Simple example case, but there are many others:

<span id="select"> </span>
<script>
window.addEventListener('load', () => {
    setTimeout(() => {
        const range = document.createRange();
        range.setStart(document.querySelector('#select'), 0);
        range.setEnd(document.querySelector('#select'), 1);
        const selection = window.getSelection();
        console.log(selection.isCollapsed, {s: selection.toString()});
        selection.removeAllRanges();
        selection.addRange(range);
        console.log(selection.isCollapsed, {s: selection.toString()});
    }, 1000);
});
</script>

Logs:

true {s: ''}
false {s: ''}

Result is that no selection is visible and Yomichan won't update the selection.

oakkitten commented 2 years ago

Even if the user manages to stumble upon such a page, simply clicking in a page should fix that, shouldn't it? Can't say for sure, but I think that I'd click on a page if I saw no selections by Yomichan, if only because I like clicking on things that don't work 😅.

Although, it might be worth checking with a few websites that hijack selections to insert links to their pages. Nasty nasty websites!

toasted-nutbread commented 2 years ago

Even if the user manages to stumble upon such a page, simply clicking in a page should fix that, shouldn't it? Can't say for sure, but I think that I'd click on a page if I saw no selections by Yomichan, if only because I like clicking on things that don't work 😅.

Correct, the user can deselect to fix the issue. It's not something I think should be worried about, just an interesting edge case.

Nasty nasty websites!

Very true

oakkitten commented 2 years ago

Fwiw I managed to find one website that does copy hijacking and everything worked well with it.