Skip to content

Conversation

@nakul-py
Copy link
Contributor

@nakul-py nakul-py commented Sep 4, 2025

Previously, clicking anywhere in the chat panel forced focus onto the input (this.node.onclick = () => this.model.input.focus();). This made it difficult to select or copy text from the chat history, since every click redirected focus.

Now we check if the user has selected text input loses focus, click on button, interactive elements, <details>, <summary> and else work like before

Screencast.From.2025-09-09.13-16-45.mp4

Closes #267

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2025

Binder 👈 Launch a Binder on branch nakul-py/jupyter-chat/selection

@jtpio jtpio added the bug Something isn't working label Sep 4, 2025
@jtpio jtpio requested a review from brichet September 8, 2025 07:55
Copy link
Collaborator

@brichet brichet left a comment

Choose a reason for hiding this comment

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

Thanks @nakul-py for working on this.

I have some comment below, let me know if it sound reasonable.

@nakul-py nakul-py requested a review from brichet September 8, 2025 14:41
@nakul-py nakul-py requested a review from jtpio September 9, 2025 07:51
Copy link
Collaborator

@brichet brichet left a comment

Choose a reason for hiding this comment

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

Thanks @nakul-py, this looks good to me.
I only have a tiny comment.

@jtpio, do you think this is enough to handle the click on elements that should not trigger a input focus ?

@jtpio
Copy link
Member

jtpio commented Sep 9, 2025

Wondering if we really need to force the focus to the chat area at all?

As a user, it can be frustrating if the application forces the focus to be somewhere else than where we want it to be.

@brichet
Copy link
Collaborator

brichet commented Sep 9, 2025

Wondering if we really need to force the focus to the chat area at all?

As a user, it can be frustrating if the application forces the focus to be somewhere else than where we want it to be.

This behavior has been added in #146. The idea was to update the focus tracker on click, to make the commands work on the last clicked chat. To update the focus tracker, we need to focus an element of the chat. By default, the input field seemed to be the most relevant element to focus on.

Comment on lines 46 to 50
if (
target.closest('button, a, input, textarea, select, summary, details')
) {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking again about it, a more generic way could be to check if the current active element is on the chat, instead of checking if the target isl focusable element. Something like:

Suggested change
if (
target.closest('button, a, input, textarea, select, summary, details')
) {
return;
}
if (this.node.contains(document.activeElement)) {
return;
}

That way we ensure that the input get focussed (and the tracker updates the current chat) only if the click occurs in a non-focusable element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds Good!🙂 and i have tried it working fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @nakul-py for updating it quickly.

@brichet brichet merged commit e99b0b1 into jupyterlab:main Sep 25, 2025
12 checks passed
@nakul-py nakul-py deleted the selection branch September 26, 2025 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove the onclick handler on ChatWidget?

3 participants