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

Preventing default when you return on the input field with autoselect: false #156

Open
davehaigh opened this issue Jun 2, 2017 · 15 comments
Labels
feature request User requests a new feature

Comments

@davehaigh
Copy link

Using the autoselect: false setting prevents a form from submitting when hitting return while the autocomplete list is visible.

@tvararu
Copy link
Contributor

tvararu commented Jun 2, 2017

Note: this is when focus is on the input field.

@tvararu tvararu added the 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) label Jun 2, 2017
@peterjaric
Copy link
Contributor

peterjaric commented Jan 8, 2020

As far as I understand it, the problem is in this code:

  handleEnter (event) {
    if (this.state.menuOpen) {
      event.preventDefault()
      const hasSelectedOption = this.state.selected >= 0
      if (hasSelectedOption) {
        this.handleOptionClick(event, this.state.selected)
      }
    }
  }

When this.state.menuOpen is true ("the autocomplete list is visible") but this.state.selected >= 0 is false (you haven't selected anything), it calls event.preventDefault() which prevents the form from being submitted (which I guess is what the title of this issue implies.)

This is somewhat related to #205: If you do select an option first this.state.selected is set to something, and then if you backspace a couple of times to get a shorter query and then hit return, the form is submitted, but not with the visible query, but instead with the one you selected in the list of options. Maybe that should be a separate issue, I don't know.

All this with autoselect: false.

@kwaves
Copy link

kwaves commented Jan 11, 2020

@peterjaric That was what I found as well. Here's a modified version which allows arbitrary text input:

  handleEnter (event) {
    const hasSelectedOption = this.state.selected >= 0
    if (this.state.menuOpen && hasSelectedOption) {
      event.preventDefault()
      this.handleOptionClick(event, this.state.selected)
    }
  }

Fork here.

I had been under the assumption that disallowing arbitrary text input was the desired behavior for this plugin. But if others are looking for this as well, I assume further mods would be needed to play nice with features I'm not using. Perhaps there could be a check added to ensure that !this.hasAutoselect(), for example, or maybe this behavior would have to be enabled via some new setting (for backwards compatibility) like allowAny.

@peterjaric
Copy link
Contributor

peterjaric commented Jan 13, 2020

Thanks @kwaves! @tvararu, what do you think about @kwaves' suggestion(s), is this something that could be incorporated into the code?

I'll tag in @hannalaakso and @NickColley too, since you have been more active as of late, I hope that's ok?

If you would like this change (or any of the alternatives given above) to be added, I can add any necessary tests, both in code and manual testing with a screen reader.

@NickColley NickColley added awaiting triage Needs triaging by team and removed awaiting triage Needs triaging by team labels Jan 13, 2020
@hannalaakso
Copy link
Member

Hi @kwaves thanks so much for picking this up!

Could you say a little bit more about the user need for making this change? If autoselect: false and we don’t prevent default when user hasn’t selected an option, then whatever user types in the input will be submitted. Here’s a screen grab (this is using your fork @kwaves), as you can see in the query string the user input which wasn't on the list was submitted.

suggested-fix (1)

This could be the desired behaviour depending on use case but the user could also make a mistake and press enter unintentionally. But perhaps we could say it’s the job of the server to look at the user input and return an error if appropriate: for instance if user typed in something that wasn’t on the list, we could either retain what the user submitted and move on, or show user an error message.

But it would be good to understand what the problem is that we’re trying to solve. @kwaves have you seen users needing to submit things that are not on the list?

Also, have you tested your fork with the enhanced select? When I tested this with enhanced select and autoselect: false the page refreshes but it looks like the user input is not submitted. Sorry I didn’t have time to dig into it but it’s worth checking that the enhanced select would work correctly with this change.

@peterjaric
Copy link
Contributor

peterjaric commented Jan 22, 2020

I'm not @kwaves, but I'll chip in with our use case. We're using this component for our search field. The options are fetched from a suggestion endpoint on our search server, but that list is not a complete list of completions as we have tens of thousands of web pages indexed. So the user might type something that gives a lot of suggestions, but not the one she is looking for. In our case we match the prefix of the string, so the input might be "Peter" and the matches for example "Peter Andersson" and "Peter Smith", but none of those is the Peter she is looking for, so she hits Enter - and nothing happens.

If this change breaks another feature, maybe it could be optional, as @kwaves suggested? And include some documentation about it being incompatible with the enhanced select feature?

@kwaves
Copy link

kwaves commented Jan 22, 2020

Hi @hannalaakso!

Like @peterjaric, I'm seeing this for AJAX-powered suggestions within a search field. (So accessible-autocomplete's source option is set to a function, as I assume he's doing as well.)

Unlike @peterjaric (I think), I usually find myself using this approach to return a single, very long list of keywords rather than pages. And in that case, the reason for AJAX is not query efficiency or dynamic responses (the list doesn't change based on search text), but frontend performance: it means I can defer the loading of a potentially huge list of terms until the user has opened the search tool.

Maybe I'm wrong, but I feel like enhanced select would fundamentally incompatible with a search field use-case. Seems like a progressively-enhanced search field would just be a search field with no suggestions. If anything there could be an enhanceSearchElement/Field/Input/Whatever function–the search field is rendered server-side, then gets hidden and re-rendered with JS. (Though admittedly that possibility sounds like it would take a lot more dev time than I'm capable of committing to in the near future.)

@peterjaric
Copy link
Contributor

peterjaric commented Jan 29, 2020

I'm considering using something like this, and will create a PR if I don't get a "no" here. Maybe the option could be called something else.

  handleEnter (event) {
    if (this.state.menuOpen) {
      // If not using autoselect and not using enhanceSelectElement, check if the current
      // value can be submitted without selecting an option from the open menu.
      const allowAnyInput = !this.props.autoselect && !this.props.selectElement && this.props.experimentalAllowAnyInput
      const hasSelectedOption = this.state.selected >= 0

      if (!allowAnyInput || hasSelectedOption) {
        event.preventDefault()
      }
      
      if (hasSelectedOption) {
        this.handleOptionClick(event, this.state.selected)
      }
    }
  }

Docs:

#### `experimentalAllowAnyInput` (default: `false`)

Type: `Boolean`

The autocomplete will submit the entered value even if does not match a value in the list of suggestions when pressing Enter. This option has no effect if `autoselect` is `true` or if `enhanceSelectElement` is used.

@hannalaakso hannalaakso added the awaiting triage Needs triaging by team label Jan 30, 2020
@hannalaakso
Copy link
Member

Hi @peterjaric thanks for offering to pick this up. I've assigned your proposal for our team to discuss at our next triage session, this should be next week. Thanks for your patience!

@hannalaakso hannalaakso added feature request User requests a new feature and removed awaiting triage Needs triaging by team 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) labels Feb 4, 2020
@hannalaakso
Copy link
Member

Thanks so much for offering to contribute this work @peterjaric 🙌

We’re prioritising bug fixes over new feature development for the accessible autocomplete at the moment. Since new features add complexity to the component, they increase the maintenance burden and the number of support requests raised. For this reason we won’t be able to accept and support your contribution at this time.

@peterjaric
Copy link
Contributor

Hi @hannalaakso, I will fork it and if you are interested at a later date, I will make a PR then instead!

Thanks for your work on this great library.

peterjaric added a commit to uppsala-university/accessible-autocomplete that referenced this issue Mar 31, 2020
@peterjaric
Copy link
Contributor

Finally I've had some time for this, and I've added the implementation above to the fork at https://github.com/uppsala-university/accessible-autocomplete. Tests for the new feature are lacking, so it's not ready for a PR, but if and when you're interested in this functionality, just ping me here and I'll see to it, if I haven't fixed it in the meantime.

peterjaric added a commit to uppsala-university/accessible-autocomplete that referenced this issue Apr 28, 2020
peterjaric added a commit to uppsala-university/accessible-autocomplete that referenced this issue Apr 28, 2020
peterjaric added a commit to uppsala-university/accessible-autocomplete that referenced this issue Apr 28, 2020
peterjaric added a commit to uppsala-university/accessible-autocomplete that referenced this issue Apr 28, 2020
@peterjaric
Copy link
Contributor

Hi again @hannalaakso, almost one year later. Are you interested in a PR of this feature at this point in time?

@mikepspc
Copy link

Would like to see that happen! Also encountering this bug over here.

peterjaric added a commit to uppsala-university/accessible-autocomplete that referenced this issue Oct 25, 2021
@vanitabarrett
Copy link
Contributor

Hi @peterjaric ,

Although the GOV.UK Design System team maintains the accessible autocomplete as a standalone component, we’re only able to put in minimal work to support it.

You can read about our plans to maintain this component.

You can also read more about the types of support we can provide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request User requests a new feature
Projects
None yet
Development

No branches or pull requests

8 participants