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

LibWeb: Fix "incorrect behavior on select()" #1458

Closed
wants to merge 2 commits into from

Conversation

An-n-ya
Copy link
Contributor

@An-n-ya An-n-ya commented Sep 20, 2024

This patch resolves the problem outlined in issue #1446. The problem is the cursor_position node could be substituted with any other existing node by the mousedown event. Previously, if this new node was not equal to m_text_node, the function would return prematurely without updating the selection.

With this fix, we ensure that the selection is properly updated in such cases, rather than simply exiting the function.

Also, in order to address the issue in #1339, the logic of document().set_cursor_position has been put behind to the logic of Selection.

Closes #1446

@ladybird-bot
Copy link
Collaborator

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

@An-n-ya An-n-ya force-pushed the fix_1446 branch 2 times, most recently from e52f7c3 to e25160f Compare September 20, 2024 14:18
@circl-lastname
Copy link
Contributor

circl-lastname commented Sep 20, 2024

The Issue number is not needed in the commit name, instead you can put Closes #1446 inside the pull request description, which will also make GitHub automatically close the issue this fixes.

@An-n-ya An-n-ya changed the title LibWeb: fix "incorrect behavior on select()" #1446 LibWeb: Fix "incorrect behavior on select()" Sep 20, 2024
@gmta
Copy link
Collaborator

gmta commented Sep 20, 2024

Paging @Gingeh !

This patch resolves the problem outlined in issue LadybirdBrowser#1446, where the
cursor_position node could be substituted with any other existing
node by the mousedown event. Previously, if this new node was not
equal to m_text_node, the function would return prematurely without
updating the selection.

With this fix, we ensure that the selection is properly updated in
such cases, rather than simply exiting the function.

Also, in order to address the issue in LadybirdBrowser#1339, the logic of
document().set_cursor_position has been put behind to the logic of
Selection.

Closes LadybirdBrowser#1446
@Gingeh
Copy link
Contributor

Gingeh commented Sep 20, 2024

Very good!
Could it be worth adding a test for this?

@An-n-ya
Copy link
Contributor Author

An-n-ya commented Sep 21, 2024

Very good! Could it be worth adding a test for this?

Normally, we can use inputNode === document.activeElement to check if an input is focused, but in Ladybird, activeElement is still not right. So, I can only provide a very basic test like this:

<input type="text" id="input" value="foo" />
<input type="text" id="boo"/>
<script src="../include.js"></script>
<script>
    test(() => {
        // simulate issue #1446
        let input = document.getElementById("input");
        let boo = document.getElementById("input");
        input.select();
        boo.select();
        input.select();

        let sel = document.getSelection();
        if (sel.anchorNode) {
            println("PASS");
        }
    });
</script>

which doesn't correspond to issue in #1446 precisely, so I think it would be better to complete this test case once we have a more robust interface (e.g., document.activeElement).

@ronak69
Copy link
Contributor

ronak69 commented Sep 21, 2024

The test case in Issue-1446 works fine now but I also checked with setInterval(mySelect, 3000) and it does not select.

@shlyakpavel
Copy link
Contributor

In light of recent changes, is this still relevant? If so, @An-n-ya, could you please rebase and resolve the conflicts?

@An-n-ya
Copy link
Contributor Author

An-n-ya commented Nov 3, 2024

The function selection_was_changed is no longer needed, so this patch is irrelevant to original problem.

@An-n-ya An-n-ya closed this Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LibWeb/HTML: incorrect behavior on select()
7 participants