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 example text popup not updating correctly when scan resolution is set to word #1527

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

jamesmaa
Copy link
Collaborator

@jamesmaa jamesmaa commented Oct 25, 2024

Fixes #1526

The html for the example text upon loading the settings page

<div class="example-text-container">
    <input type="text" class="example-text example-text-input" id="example-text-input" lang="en" hidden>
    <span class="example-text" id="example-text">rea</span>
</div>

In a normal operations, when a user clicks on the example text, the <span> becomes hidden and the <input> becomes visible. However, all search is done through the value set in the <span> class.

We have the class PopupPreviewFrame (plus an obscure css rule) that manages the state between the <input> and the <span> elements.

When search is triggered, Yomitan searches for value inside <span>. Since the span is hidden while editing the example text, the element would normally be skipped to scanning in DOMTextScanner. But since the initial node of the text source range IS the span text, the first node value in DOMTextScanner is processed no matter what

const textNode = this._exampleText.firstChild;
const range = document.createRange();
range.selectNodeContents(textNode);
const source = TextSourceRange.create(range);

In the case of the scanResolution set to word, upon initiating the scan, we try to expand the beginning of the initial range to encompass the entire word by looking backwards to find the word boundary. In the case of example text, we exit the <span> text to find a space to determine we've reached a word boundary. HOWEVER, this causes a search that starts outside the visibility: none <span> element to be skipped due to how DOMTextScanner skips hidden elements.

Why didn't this break in other UX flows?
In the case of a scanResolution = 'character', we don't try to change the initial range startOffset, so the initial starting node for the search is within the hidden <span>
In the case of a scanResolution = 'word' page load, the <input> value is hidden while <span> is visible, so moving the startOffset of the initial range doesn't affect the scanability of the span text.

Solutions considered

So in terms of fixes I had two potential options that I've explored. The first is to fix the behavior of the visibility of <input> and <span> to update correctly BEFORE the search begins since. This might be weird UX though since I'm not sure what would happen when you update the visibility of an input element while still in focus.

The second potential fix is to prevent the DOMTextScanner from leaving the <span> element. I tried implementing this earlier and it does complicate the DOMTextScanner a bit by forcing the scanner to peek at the contents of the previous DOM node before traversing it, which I feel like violates some abstraction boundaries of how DOMTextScanner is currently implemented.

Solution proposed

I figured the easiest solution is to just add an extra option disallowExpandStartOffset that is set to true only in the example text popup frame. This prevents the startOffset from leaving the initial hidden <span>, thereby allowing scan to successfully work. This fixes our problem "cleanly" without changing too much of how we manage the popup-preview or the DOMTextScanner

@jamesmaa jamesmaa changed the title WIP: Fix example text popup not updating correctly when scan resolution is set to word Fix example text popup not updating correctly when scan resolution is set to word Nov 1, 2024
@jamesmaa jamesmaa added the kind/bug The issue or PR is regarding a bug label Nov 1, 2024
@jamesmaa jamesmaa marked this pull request as ready for review November 1, 2024 20:43
@jamesmaa jamesmaa requested a review from a team as a code owner November 1, 2024 20:43
@Kuuuube Kuuuube added this pull request to the merge queue Nov 4, 2024
@Kuuuube Kuuuube added the area/ui-ux The issue or PR is related to UI/UX/Design label Nov 4, 2024
Merged via the queue into yomidevs:master with commit 37ba526 Nov 4, 2024
11 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.

Yomitan popup in settings not updating when entering new string for non-ja languages
2 participants