-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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.
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
Conversation
| if (elementToFocus.tagName === 'INPUT' && | ||
| ['search', 'text', 'email', 'url'].includes(elementToFocus.type) && | ||
| elementToFocus.value) { | ||
| const end = elementToFocus.value.length; | ||
| elementToFocus.setSelectionRange(0, end); | ||
| } |
There was a problem hiding this comment.
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-modalcustom HTML element (search, password page modal to enter password) - the menu drawer
- the
modal-dialogcustom 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 ? 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
sofiamatulis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍 Thanks for making the change. Added some small nitpicks
| document.addEventListener('focusout', trapFocusHandlers.focusout); | ||
| document.addEventListener('focusin', trapFocusHandlers.focusin); | ||
|
|
||
| elementToFocus.focus(); |
There was a problem hiding this comment.
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? 👀
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 theabout uspage for theos2-demostore, 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-modalcustom HTML element (search, password page modal to enter password)- the menu drawer
- the
modal-dialogcustom 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
trapFocusfunction is aninputelement.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
ludoboludo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Select text input content on trapFocus * PR fix
commit dc81806 Author: Ken Meleta <[email protected]> Date: Tue Nov 22 13:32:46 2022 -0700 Add media sizing settings to featured product (Shopify#2114) commit 9875aea Author: Ken Meleta <[email protected]> Date: Tue Nov 22 13:17:55 2022 -0700 Adjust constrain media values for quick add modal (Shopify#2113) * Adjust constrain media values for quickadd * Adjust offset logic * Minor formatting fix commit 2eb8bf6 Author: Ludo <[email protected]> Date: Tue Nov 22 11:50:50 2022 -0500 Add fixed sizes for some images to prevent errors (Shopify#2087) * remove the mention of sizes where unnecessary * remove another instance * add fixed size to the images in structured data * adjust size to 1920 * edit logo size in structured data * remove conditional info * change value to match other logo size values commit 2f9b9f3 Author: Ludo <[email protected]> Date: Mon Nov 21 15:44:24 2022 -0500 Remove alt attribute in image_tag filter where unnecessary (Shopify#2117) * Add fallback to alt attribute where image_tag filter is used * remove unnecessary use of alt attribute in image_tag filter commit 251e5d9 Author: Ludo <[email protected]> Date: Mon Nov 21 12:18:44 2022 -0500 Move share button into a snippet (Shopify#2123) * Move share button into a snippet * change the property name to context and add details in snippet * address review comments * edit props names and description * Address reviewers comment. Remove unused prop and edit existing ID * move script at the top commit 539af27 Author: Eugene Kasimov <[email protected]> Date: Mon Nov 21 09:01:58 2022 -0800 Prettier liquid files snippets. (Shopify#2115) commit 503fdf8 Author: Eugene Kasimov <[email protected]> Date: Thu Nov 17 08:51:21 2022 -0800 Remove noscript css import for product recommendations (Shopify#2101) commit b2b01fd Author: Eugene Kasimov <[email protected]> Date: Thu Nov 17 08:50:33 2022 -0800 Change variables names (Shopify#2055) commit cf05d0d Author: Francisca Calabria Rubio <[email protected]> Date: Thu Nov 17 16:50:55 2022 +0100 Select text input content on trapFocus (Shopify#2106) * Select text input content on trapFocus * PR fix commit b86d1f3 Author: Ken Meleta <[email protected]> Date: Thu Nov 17 08:50:43 2022 -0700 Change thumbnail media fit to "fill" by default (Shopify#2044) * Change thumbnail media fit to cover by default * Simplify and cleanup thumbnail fit approach commit 8ab8e61 Author: Ludo <[email protected]> Date: Thu Nov 17 10:50:28 2022 -0500 Tweak the image_url size to be the max value where necessary (Shopify#2110) * Tweak the image_url size to be the max value where necessary * fix sizing in the header. commit c337301 Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Date: Thu Nov 17 10:49:52 2022 -0500 Update 1 translation file (Shopify#2130) Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> commit bc8f433 Author: Ken Meleta <[email protected]> Date: Thu Nov 17 08:49:10 2022 -0700 Add constrain media and media fit settings to product page (Shopify#2052) * Add constrain height setting * Add constrain scaling functionality * Add media fit setting and functionality * Quick add override and typo fixes * Update setting language and code cosmetics * Update 8 translation files * Update 5 translation files * Update 3 translation files * Update 3 translation files * Update 1 translation file * Add comment Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
* Select text input content on trapFocus * PR fix
* Select text input content on trapFocus * PR fix
PR Summary:
Select text input's content when automatically focusing it
Why are these changes introduced?
This affects the search input on the header, that gets focused automatically when the user clicks on the "search" icon. The behaviour mimics the browser default when tabbing into an input, but I restricted it to text types to avoid causing UX problems or bugs on other kinds of inputs (passwords, dates, etc).
What approach did you take?
Use the native
setSelectionRangefunctionVisual impact on existing themes
Gif preview
Demo links
Checklist