Autocomplete trigger not clearing when adding a chip in paper-chips with autocomplete

[pauln]

Some time between v1.0.0-beta.4 and v1.0.0-beta.7, paper-autocomplete has changed in such a way that the workaround used by paper-chips (with autocomplete) is no longer able to clear the trigger value (search text). To be more precise, it does actually reset - but the autocomplete then immediately switches to populating the trigger with a cached value (_innerText), so the user has to manually remove the search text before they can search for another item.

[pauln]

The change in question is 5f264a2, which doesn't explain why the change was made. @xomaczar, do you recall why the _innerText stuff was added to the text computed property in paper-autocomplete-trigger.js? It looks as though it might be an attempt to prevent a violation of the DDAU principle - but it's making it impossible to clear the searchText from outside the trigger, as text reverts back to the _innerText value if searchText is set to a falsey value (i.e. empty string).

I've confirmed that removing the _innerText stuff completely fixes this issue (and a bit of manual testing doesn't seem to reveal any issues caused by doing so), but if it's there for a good reason (such as preventing DDAU violations) it'd probably be better to fix it instead. That said, I can't see (at a quick glance) anywhere that DDAU would be violated anyway - none of the paper-autocomplete code seems to be modifying it, and it's wrapped in the readonly helper when passed into paper-autocomplete-trigger - but maybe I'm missing something (or _innerText wasn't DDAU-related)?

[pauln]

Further testing shows that _innerText is necessary for the trigger to actually display any text if no searchText parameter is specified on the {{paper-autocomplete}} tag (such as the second and third examples in the docs).

I've had a bit of a play around to try to make it work without _innerText while still allowing searchText and onSearchTextChange to be optional, but haven't had much luck yet. Is anyone who's more familiar with the autocomplete code able to take a look? Or should we just make searchText and onSearchTextChanged both required, so that we can simplify the autocomplete code by removing _innerText?

[xomaczar]

@pauln please confirm these changes address your use cases: https://github.com/xomaczar/ember-paper/tree/fix/make-autocomplete-data-down-action-up

Also, paper-chips component was updated to use this "fixed" autocomplete

See the dummy app

[xomaczar]

@miguelcobain @pauln I would love to remove _innerText craziness, as it violates DDAU principle and causes issues for other components that depend on paper-autocomplete.
If a user only provides searchText, we should assert that onSearchTextChange action was also provided.

Thoughts?

[pauln]

Your make-autocomplete-data-down-action-up branch seems to work exactly as I'd expect/hope, @xomaczar - well done!

I've just submitted a PR to your branch which completely cleans out (unless I missed anything) the resetInput workaround which used to be necessary to reset the autocomplete trigger; feel free to squash it before submitting a PR to the main ember-paper repo. I'm thrilled to see this hacky mess disappear - thanks for working on this 🎉

[pauln]

Does anything else need to be done before your branch is ready to submit a PR to this repo, @xomaczar? If so, is there anything I can do to help move it along?

[xomaczar]

@pauln - sorry I haven’t had a chance to button it up. Here are the things I’d like to add/fix before making a PR.

1. Remove commented _innerText
2. Still thinking about this one. Assert if searchText is provided, onSearchTextChange action must also be supplied by the caller. This is not that critical as we already provide a fallback action which simply sets the searchText. The case where it will not work correctly or not at all actually is when you use readonly helper while binding searchText. (e.g searchText=(readonly searchText)).

[pauln]

Personally, I'd think that use of readonly signals a deliberate intent to prevent the component from updating the searchText in order to allow a custom onSearchTextChange to handle it, so cases where somebody has used readonly but not provided an onSearchTextChange should be minimal (and those cases which do exist may well be mistakes).

Requiring (via an assert) an onSearchTextChange would also presumably render the fallback action unnecessary, so I guess the question is really whether we want to require onSearchTextChange or have a fallback which doesn't support searchText being readonly. It's a pity that it's not possible (as far as I'm aware) to detect whether the param is readonly, as if it was possible to do so, we could require onSearchTextChange only when searchText is readonly...

Any thoughts on all of this, @miguelcobain?

[xomaczar]

readonly helper is kind of advanced and in general discourage by ember core team (as glimmer components are/will be readonly/one-way by default). I think what we have should just work as is.

[xomaczar]

@pauln can we close this

[pauln]

Yes, this is now fixed thanks to your great work in #956, @xomaczar - thanks again!