fix(components): Improve Focus and Blur Experience on Autocomplete v2 #2809
Conversation
This prevents the consumer from running onBlur unnecessarily. In one case this is triggering validation to occur even though the user hasn't selected an option.
|
Published Pre-release for ee803fd with versions: To install the new version(s) for Web run: |
Deploying atlantis with
|
| Latest commit: |
74dd037
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f316a696.atlantis.pages.dev |
| Branch Preview URL: | https://job-141426-fix-autocomplete.atlantis.pages.dev |
|
nice this solves the immediate problem. though I feel that it leads to some additional questions and potential work. are there any actions that should trigger a blur event when they are used? the Modal example comes to mind, and I believe this would prevent firing the say we have a "create client" button inside an interactive element. it launches a Modal to create. the Autocomplete definitely should close, and focus is lost. however, there are other interactive elements that might not lose focus. imagine a "add NewOption" action. choosing that should add your term, and keep focus inside the autocomplete. I'm not sure we'll ever have enough context to say if we should blur or not. it depends so much on the actions. if something has but when it's that means we need to pay attention to what we are clicking on. an inert header, sure - never blur. an interactive element with an interactive element with maybe we do need an additional prop on action like elements? also, this complexity really comes from how much we allow an Autocomplete to contain 😭 |
|
tried something a bit different but couldn't quite get it working. tests are failing and I believe the scenario we want to avoid still happens. overall, what I'd like us to do is to prevent focus events from firing inside the component when we don't want them to. that helps keep blur simple. additionally, we are missing some code to keep focus inside the input when we click on items: actions, headers, footers, and even options. I added it in the commit, and it helps with some aspects of this since that's the desired behavior and if we can prevent focus from leaving then there's less of an issue with blur. accompanying the above were changes preventing the menu from reopening during those cases and finally allowing clicking the already focused input to open the menu again. |
| type: "action", | ||
| label: "Add Service", | ||
| onClick: () => alert("Add Service"), | ||
| onClick: () => console.log("Add Service"), |
There was a problem hiding this comment.
I need to double check if this change is required. with one implementation I had that used setTimeouts - the timing was getting interrupted by the alert which has a different execution flow from "standard" code
There was a problem hiding this comment.
ok yes, using alert is problematic when attempting to verify the onBlur behavior. it has atypical behavior, interrupting the event loop delaying execution and firing focus events.
I've implemented a simple text based feedback system that says what your last action was instead.
|
this works in the current state of the branch, however merging master breaks it and tests will correctly fail. this PR: #2835 will restore the behavior, allow us to pull in master, resolve conflicts, and proceed. |
…void event issues
|
last commit isn't strictly necessary, or even related to this - it was just bugging me and fixing it is a 1 line change. old new the culprit is the debounce. since we have a brief delay to wait for typing - the UI and the state could get out of sync. your value is nothing, so you should see all items but the debounce causes us to still see the previous selection/search term. we debounce when adding words/characters because we can't know if you're done typing. however, reversing that is not equivalent. if I reduce my input down to nothing (empty) I can't keep making it emptier that's a final state, so we should immediately update without a debounce. |
ZakaryH
changed the title
| }), | ||
| }); | ||
| }, | ||
| onMouseDown: onInteractionMouseDown, |
There was a problem hiding this comment.
Curious about the usage of mouse events over pointer events. On mobile browsers, I think this may not fire? 🤔
Is that intentional? Thinking onPointerDown might be better here.
There was a problem hiding this comment.
good question. let me try that out!
There was a problem hiding this comment.
well, that wasn't exactly an intentional decision I made but it's working as expected in both Firefox and Chrome's mobile touch modes
which is really not what I would have expected... I thought the whole point of onPointerDown was to have a device agnostic way to capture both. though I have not kept up with browser implementations around onMouseDown/Up. maybe they get mapped on touch events now?
I'll try this on my actual phone to be sure.
yeah even on my phone it's working as intended 🤔
|
|
||
| const click = useClick(context, { | ||
| enabled: !readOnly, | ||
| toggle: false, // Only open, never close on click |
There was a problem hiding this comment.
playing around with this more, I'm not sure I like this after all. it's nuanced though. if I have text and I click to put my cursor somewhere, I don't want it to close.
if it's empty and I just want to get it out of the way, that's different.
so really, we might want to allow clicking to toggle only when it's empty.
I don't think we need to address this right now though.
jdeichert
left a comment
jdeichert
left a comment
There was a problem hiding this comment.
Looking good, a few comments.
I'm QAing the linked PR next, then will approve.
| return ( | ||
| <div | ||
| className={styles.emptyStateMessage} | ||
| onPointerDown={e => e.preventDefault()} |
There was a problem hiding this comment.
Seeing all of these onPointerDown={e => e.preventDefault()} methods makes me think it could be worth it to make a reusable function like onPointerDown={preventInteraction}. That function could have jsdocs explaining why it's important and how it works with onInteractionPointerDown
Not a blocker at all. I'm happy to get this merged as is 👍
There was a problem hiding this comment.
I like that. here's an attempt. does that align with what you were thinking?
There was a problem hiding this comment.
Exactly what I was thinking! Makes it more clear that these 2 behaviours are working together 👌
| useEffect(() => { | ||
| if (debounceMs === 0) { | ||
| // Skip debounce when clearing input for immediate feedback, preventing flickering of last selected item | ||
| if (debounceMs === 0 || inputValue === "") { |
| <Autocomplete | ||
| version={2} | ||
| placeholder="openOnFocus=false" | ||
| openOnFocus={false} |
There was a problem hiding this comment.
Just something I never noticed before.. this makes me think it shouldn't open when clicking it, but it does. The jsdoc comment says Whether the menu should open when the input gains focus. which also seems to imply that it wouldn't open when clicking on it to focus it.
I guess i'm curious in what cases would openOnFocus=false be useful to consumers? Why would they not want the menu to open when focusing via keyboard?
Not a blocker of course.
There was a problem hiding this comment.
that's fair, it's a little counter intuitive. I added a note in the PR description but that's maybe not the best place since this will likely come up again.
always open the menu on input click (distinct from openOnFocus). focus is a subset of what happens during a click. a click on the input is clear intent to use it so we should show the menu.
so basically we are saying that a click is very high intent. barring misclicks, you are expressing a desire to use this input. as such, if you click on it and it's closed - we open the menu. it's almost a side effect that clicking causes focus.
pure focus that doesn't come from a click such as tabbing through the form elements is more ambiguous as far as intent. did you want to tab to this Autocomplete and use it, or are you actually trying to get to an element that comes after the Autocomplete in the form.
that's why I'd offer a different behavior at all compared to clicking.
as for a real flow or UX this enables: if you try the web story with the openOnFocus={false} using the empty actions, then closing the modal you'll notice that we do not open the Autocomplete again when it regains focus from the closed modal. of course it's not the only way to make that happen, but it's definitely one of the simpler ones rather than say moving focus onto the next sibling element rather than the Autocomplete that initiated the Modal open.
There was a problem hiding this comment.
openOnFocus=true
openOnFocus=false
this is all with keyboard interactions. if I clicked then the behavior difference would be limited to only the Autocomplete not re-opening on Modal close/dismiss
There was a problem hiding this comment.
Gotcha, that makes sense 👍
I guess my point is that maybe the jsdoc comment could more clearly state what conditions it works under.
Again, not a blocker nor the focus of this PR so I'm not saying we need to update that here.
There was a problem hiding this comment.
I agree with that. I'll see if I can either document it better in the JSDoc, or if not there at least in the component docs.
jdeichert
left a comment
jdeichert
left a comment
There was a problem hiding this comment.
Approving, looks good! Thanks again for thoroughly thinking through the implications involved here, I like where you landed.
I can't approve my own PR, so will let you handle that and merging at your convenience 👍
ZakaryH
left a comment
ZakaryH
left a comment
There was a problem hiding this comment.
LGTM (Jacob is actually approving but he's the original PR author)
Motivations
Currently,
props.onBluris being called when clicking within autocomplete v2's floating menu, on a non-option item... a clickable header in this case. This causes the client to run validation on the input due to the blur event, thinking that the user entered a value when they didn't, thus showing a validation error.This PR avoids calling
props.onBlurunnecessarily, when the user is simply clicking inside the Autocomplete v2 floating menu.Changes
Changed
programmatically re-focus the input when the following interactions happen via mouse click:
this means focus never leaves the focus for a user making it easy to use repeatedly. the keyboard experience already behaves like this thanks to the virtual focus we leverage, so now the two experiences are in sync.
paired with the above, made a change to avoid re-opening the menu from that focus after a selection or if an action has shouldClose because without that, it would continuously re-open.
onFocus only called once despite the repeated programmatic focus events. the multiple calls are an implementation detail that a consumer need not know about.
avoid calling onBlur when we interact with anything inside the autocomplete (inert sections, text items, interactive elements)
since text elements are inert, cursor updated to better communicate that
Added
always open the menu on input click (distinct from openOnFocus). focus is a subset of what happens during a click. a click on the input is clear intent to use it so we should show the menu.
the reason for this is since we don't lose focus now, nor do we open the menu after a selection so we need to provide a convenient way for mouse users to quickly re-open the menu. having to blur, and refocus it would be very cumbersome.
Fixed
Prevents unnecessary
onBlurcall when clicking inside Autocomplete V2improved the clearing visual experience, preventing a momentary display of previous matches/selection.
Testing
onFocus should only be called once, when you expect it to
onBlur should not be called while interacting with any elements inside
clearing a selection with select-all + delete should look & feel better
clicking on the input should always open the input, becomes most apparent after clicking to select and wanting to choose something else.
overall, it should still feel good to use.
For QA, please see the linked PR below (anchor link) which has instructions for testing this in the app.
Changes can be tested via Pre-release
In Atlantis we use Github's built in pull request reviews.