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

[Request] Opt-in/-out for input polling #164

Open
samtsai opened this issue Jun 7, 2017 · 4 comments
Open

[Request] Opt-in/-out for input polling #164

samtsai opened this issue Jun 7, 2017 · 4 comments
Labels
🐛 bug Something isn't working the way it should (including incorrect wording in documentation)

Comments

@samtsai
Copy link
Contributor

samtsai commented Jun 7, 2017

We ran into a performance issue as well as functional issue when rendering hundreds of accessible-autocomplete components. Albeit, not the typical use case, but think Excel like cells with autocomplete feature. We happen to have 100s of autocomplete-enabled cells.

Digging into source we found that the performance hit came from each component creating a polling timeout:
https://github.com/alphagov/accessible-autocomplete/blob/master/src/autocomplete.js#L106

Two things:

  • we actually want to be able to change values of inputs without triggering an input change (i.e. Avoid this chain: getDirectInputChanges => handleInputChange)

screenshot 2017-06-07 15 12 04

  • timeouts are unnecessarily filling up memory

screen shot 2017-06-07 at 3 05 23 pm
screen shot 2017-06-07 at 3 04 32 pm

Proposal

Add an opt-in or opt-out interface to the existing API.

Example doc for opt-out:

#### `handleDirectInputChanges` (default: `true`)

Type: `boolean`

When the `value` of the input is changed from an external source (e.g. Dragon NaturallySpeaking) trigger the autocomplete menu if the value is different than before. Set this to `false` to disable that feature and allow changing of the `value` field without triggering events.
@tvararu
Copy link
Contributor

tvararu commented Jun 8, 2017

Interesting! Thanks for reporting this @samtsai.

I definitely recognise that the solution used here for Dragon is a hack, and wrecks performance. I actually did think of what would happen if someone added hundreds of autocomplete instances to a page, but thought that to be highly unlikely. Wrong! 😉

I am slightly weary of providing options which explicitly make the component less usable with some assistive technologies.

That being said, this is reminiscent of something me and @edwardhorsford have bumped in before, which is that it's not always possible to build a certain experience while making it accessible. autoselect being disabled on mobile due to iOS VoiceOver comes to mind.

I think there is grounds to allowing, if you know your users very well and not by default, to disable or significantly change the behaviour of certain features, in ways that would conceivably erect barriers with certain AT. As long as the defaults stay as accessible as possible.

So yes, we could provide this option.


There might be another solution to this issue: the timeouts don't need to run unless the autocomplete is focused. We could change it from starting it on componentDidMount to on handleInputFocus (and stopping the polling on handleComponentBlur) and it would still work with Dragon.

@samtsai do you think that could work for your use-case?


Also, if you're looking at rendering large amounts of complex widgets in a page, there's a few other things that spring to mind:

  • Using a virtualized rendering library, such as react-virtualized to only render elements that are within view at any one time (NB: I don't know if these libraries provide an accessible experience); this technique is used in products like Google Docs/Sheets or PDF.js;
  • Rendering dumb <input>s which become autocompletes when users focus them, and become dumb again when users blur. Note that we don't provide an easy way to destroy the instance of an accessibleAutocomplete currently, unless it's with the React/Preact versions; (later edit: I can't think of good reasons why the default accessibleAutocomplete can't just behave like this by default)
  • Keeping in mind that lots of DOM nodes will wreck performance on low specced devices regardless of their complexity. Google Lighthouse gives you a pass on this if you have less than <1500 nodes.

@tvararu tvararu added the Question: decision-making User queries product decisions label Jun 8, 2017
@samtsai
Copy link
Contributor Author

samtsai commented Jun 8, 2017

@tvararu this is great feedback! The more controlled polling scenario via handleInputFocus and handleComponentBlur would probably work in our scenario.

As far as the suggestions for rendering large amounts, that's the direction we're moving towards as we iterate on our existing app. Thanks for the list!

@tvararu tvararu added 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) and removed Question: decision-making User queries product decisions labels Jun 8, 2017
@tvararu
Copy link
Contributor

tvararu commented Jun 8, 2017

@samtsai cool, then if that works I'm going to file this as a bug.

pollInputElement should be called on handleInputFocus and cleared on handleComponentBlur.

@tvararu
Copy link
Contributor

tvararu commented May 29, 2018

A potential solution for this could be to replace the polling with Object.defineProperty, as mentioned in this thread on Wordpress Gutenberg about a very similar issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working the way it should (including incorrect wording in documentation)
Projects
None yet
Development

No branches or pull requests

2 participants