Skip to content

Select text input content on trapFocus #2106

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 17, 2022
Merged

Select text input content on trapFocus #2106

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5daa821
Select text input content on trapFocus
FCalabria Nov 7, 2022
bc2068e
PR fix
FCalabria Nov 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions assets/global.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@ function trapFocus(container, elementToFocus = container) {
document.addEventListener('focusin', trapFocusHandlers.focusin);

elementToFocus.focus();
Copy link
Contributor

@sofiamatulis sofiamatulis Nov 8, 2022

Choose a reason for hiding this comment

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

Search only focuses when I close and re-enter the field:https://screenshot.click/08-12-xeqqs-dai70.mp4

That doesnt happen for the other inputs: https://screenshot.click/08-15-5ykcg-12f5p.mp4

Is that expected? 👀

Copy link
Contributor Author

@FCalabria FCalabria Nov 10, 2022

Choose a reason for hiding this comment

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

Interesting, I cannot reproduce that https://screenshot.click/10-59-0u5in-jynpn.mp4
What browser are you using? You are just tabbing around, right?

Copy link
Contributor

@sofiamatulis sofiamatulis Nov 10, 2022

Choose a reason for hiding this comment

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

I am using chrome and just tabbing 🤔 Am I missing something?

https://screenshot.click/10-34-j4hjv-6b43g.mp4

Copy link
Contributor

@ludoboludo ludoboludo Nov 10, 2022

Choose a reason for hiding this comment

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

This is meant to be that way based on how it's added in the code:

since this is in the trapFocus() function this will only run when we're trapping the focus. But won't happen anywhere else on inputs. So for example on the about us page for the os2-demo store, it won't do anything on the contact us form fields.

We currently run trapFocus() on:

  • the cart drawer
  • the cart notification
  • modals using the detais-modal custom HTML element (search, password page modal to enter password)
  • the menu drawer
  • the modal-dialog custom HTML element
  • the pickup availability drawer

In all those cases, I think the search is the only scenario where the element receiving focus due to our trapFocus function is an input element.

Copy link
Contributor

@sofiamatulis sofiamatulis Nov 11, 2022

Choose a reason for hiding this comment

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

Interesting 🤔 Then how come Paqui is able to do it in her video? https://screenshot.click/10-59-0u5in-jynpn.mp4

Copy link
Contributor Author

@FCalabria FCalabria Nov 11, 2022

Choose a reason for hiding this comment

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

That should be standard browser behaviour, it's not due to any change i have done

Copy link
Contributor

@sofiamatulis sofiamatulis Nov 14, 2022

Choose a reason for hiding this comment

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

What I mean is that the text input's content is selected when automatically focusing it on your video but not in mine

Copy link
Contributor

@sofiamatulis sofiamatulis Nov 15, 2022

Choose a reason for hiding this comment

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

We paired and it turns out that you need to use shift tab and it does select it then :)


if (elementToFocus.tagName === 'INPUT' &&
['search', 'text', 'email', 'url'].includes(elementToFocus.type) &&
elementToFocus.value) {
const end = elementToFocus.value.length;
elementToFocus.setSelectionRange(0, end);
}
Copy link
Contributor

@ludoboludo ludoboludo Nov 7, 2022

Choose a reason for hiding this comment

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

since this is in the trapFocus() function this will only run when we're trapping the focus. But won't happen anywhere else on inputs. So for example on the about us page for the os2-demo store, it won't do anything on the contact us form fields.

We currently run trapFocus() on:

  • the cart drawer
  • the cart notification
  • modals using the detais-modal custom HTML element (search, password page modal to enter password)
  • the menu drawer
  • the modal-dialog custom HTML element
  • the pickup availability drawer

In all those cases, I think the search is the only scenario where the element receiving focus due to our trapFocus function is an input element.

I'm not sure if introducing this behaviour only for that one specific part of the theme makes sense when the rest of the input don't get the same behaviour on focus 🤔

But then having a listener for focusin on all inputs of the theme might be a bit much. Not sure.

What do you think ? 🤔

Copy link
Contributor Author

@FCalabria FCalabria Nov 8, 2022

Choose a reason for hiding this comment

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

Adding this on every focusin event will select the value also when the user focuses the input by clicking on it, which I don't think is a good UX since they might be clicking exactly in the point were they want their cursor.

If there were some way to know if a focus has being triggered programatically or not (or by a mouse click) it could be a nice improvement for the theme, but I don't know any reliable way to do that.

Copy link
Contributor

@ludoboludo ludoboludo Nov 8, 2022

Choose a reason for hiding this comment

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

Adding this on every focusin event will select the value also when the user focuses the input by clicking on it, which I don't think is a good UX since they might be clicking exactly in the point were they want their cursor.
💯

I'll approve the PR 👍 Though we should wait a bit before merging (once you get the second approval).

}

// Here run the querySelector to figure out if the browser supports :focus-visible or not and run code based on it.
Expand Down