Dynamic select menus don't live bind correctly #1762

Closed
dylanrtt opened this Issue Jun 22, 2015 · 20 comments

Comments

Projects
None yet
5 participants
@dylanrtt
Contributor

dylanrtt commented Jun 22, 2015

When creating a dynamic select menu with a stache #each such as in the following example, when the result of the rendered list changes, the selected value of the select menu changes to the first item, which isn't necessarily correct.

<select can-value="{selectedOpt}">
  <option></option>
  {{#each opts}}
    <option name="{{name}}">{{name}}</option>
  {{/each}}
</select>

http://jsbin.com/fedixolaru/1/edit?html,js,output

This can occur when using a computed list or when using stache filtering.

@justinbmeyer justinbmeyer added the bug label Aug 5, 2015

@justinbmeyer justinbmeyer added this to the 2.3.0 milestone Aug 5, 2015

@justinbmeyer justinbmeyer self-assigned this Aug 5, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Aug 5, 2015

Contributor

@dylanrtt Just found this myself. Thanks for reporting.

Contributor

justinbmeyer commented Aug 5, 2015

@dylanrtt Just found this myself. Thanks for reporting.

@akagomez

This comment has been minimized.

Show comment
Hide comment
@akagomez

akagomez Aug 5, 2015

Contributor

Here's a demo of this being an issue with native DOM manipulation too: http://jsbin.com/tupodanihu/1/edit?html,js,output

Contributor

akagomez commented Aug 5, 2015

Here's a demo of this being an issue with native DOM manipulation too: http://jsbin.com/tupodanihu/1/edit?html,js,output

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Aug 5, 2015

Contributor

This is probably less likely to happen in minor because of the diffing.

Contributor

justinbmeyer commented Aug 5, 2015

This is probably less likely to happen in minor because of the diffing.

@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt Nov 3, 2015

Contributor

#1827 fixed the case where the options change and the selected option remains in the list, but if the selected option is removed and added back to the list, the displayed option stays out of sync.

Here is the original JSBin updated to use 2.3.1:
http://jsbin.com/hajupezufa/edit?html,js,output

Contributor

dylanrtt commented Nov 3, 2015

#1827 fixed the case where the options change and the selected option remains in the list, but if the selected option is removed and added back to the list, the displayed option stays out of sync.

Here is the original JSBin updated to use 2.3.1:
http://jsbin.com/hajupezufa/edit?html,js,output

@justinbmeyer justinbmeyer added this to the 2.3.7 milestone Dec 12, 2015

justinbmeyer added a commit that referenced this issue Dec 15, 2015

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Dec 16, 2015

Contributor

Should be closed now via 1401c40

Contributor

daffl commented Dec 16, 2015

Should be closed now via 1401c40

@daffl daffl closed this Dec 16, 2015

@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt Dec 17, 2015

Contributor

I don't think this is fixed. My previous comment is still true using master.

Contributor

dylanrtt commented Dec 17, 2015

I don't think this is fixed. My previous comment is still true using master.

@justinbmeyer justinbmeyer reopened this Dec 20, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 20, 2015

Contributor

@dylanrtt anyway you can simplify the test to something along the lines of: 1401c40#diff-7bca9ce6f0378f28c17f2c02cd1eb9b0R1859 ?

Contributor

justinbmeyer commented Dec 20, 2015

@dylanrtt anyway you can simplify the test to something along the lines of: 1401c40#diff-7bca9ce6f0378f28c17f2c02cd1eb9b0R1859 ?

@justinbmeyer justinbmeyer modified the milestones: 2.3.8, 2.3.7 Dec 20, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 23, 2015

Contributor

@dylanrtt how can I see the error? Thanks.

Contributor

justinbmeyer commented Dec 23, 2015

@dylanrtt how can I see the error? Thanks.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 23, 2015

Contributor

@dylanrtt is this how:

  1. Select "Alexis", "Curtis", "Curtis".
  2. Change the first select from "Alexis" to "Curtis" and then back to "Alexis"

?

Contributor

justinbmeyer commented Dec 23, 2015

@dylanrtt is this how:

  1. Select "Alexis", "Curtis", "Curtis".
  2. Change the first select from "Alexis" to "Curtis" and then back to "Alexis"

?

@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt Dec 23, 2015

Contributor

Yeah, that should work.

http://jsbin.com/hopukovala/1/edit?html,js,output

When the selected value is not in the list of options and then enters that list, the selected value should change but does not.

This happens a lot in my current project where picker data loads async and there are default values either present initially or loaded async as well. My current workaround is to render a dummy select if the list is not loaded, but it would be cool if I didn't have to do that.

Contributor

dylanrtt commented Dec 23, 2015

Yeah, that should work.

http://jsbin.com/hopukovala/1/edit?html,js,output

When the selected value is not in the list of options and then enters that list, the selected value should change but does not.

This happens a lot in my current project where picker data loads async and there are default values either present initially or loaded async as well. My current workaround is to render a dummy select if the list is not loaded, but it would be cool if I didn't have to do that.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 23, 2015

Contributor

@dylanrtt thanks!

Contributor

justinbmeyer commented Dec 23, 2015

@dylanrtt thanks!

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 23, 2015

Contributor

Checking out this problem a bit, it doesn't look like there is a "nice" solution. Also, I'm not sure what the behavior should be.

Two-way binding behavior

When Brian is removed, the <select>'s value becomes "". Because of this, I think the right behavior might be to change the two-way bound value (person) to "" if there is an <option value="">. If there isn't an option with a value of "", then no option will be selected and the two-way bound value will be set to undefined.

One-way binding behavior

When Brian is removed, the <select>'s value becomes "". But this should be ignored. When Brian is eventually put back, that <option> should be selected by setting the <select>'s value.

Making it work

Ideally, the bindings could setup a MutationObserver and know whenever the list of options has changed and either update the two-way bound value, or re-set the value.

As that's currently not possible in all the browsers we support, it means I'll have to create a bridge between can.view.live and can.view.bindings. Somehow, can.view.bindings will have to be alerted when some live-binding activity happens within some <select>.

For performance reasons, I want this bridge to run only when explicitly needed. Also, ideally, this bridge would not be super-special cased. There's a strong possibility we'd want to know when some other content is being updated.

My naive approach would be to have can.view.live.html and can.view.live.each simply check their parent. If it's an <select>, look for any callbacks on that option. can.view.bindings would register a callback on <select>'s with bindings:

can.view.live.registerMutationCallback(selectElement, function(){

})

This solution would be decently fast. can.view.live would maintain a "quick check map" of just the elements that get this special callback check.

There's one problem. <options> can exist in <optgroup>s. This wouldn't work because the parent isn't a <select> element.

Fortunately <optgroup>s can't be nested, so we could just check up 2 .parentNodes. However, this is no longer generalized ... or we have to go slow and start walking up the parent chain a lot.

Currently, I'm leaning to one solution that uses MutationObservers for browsers that support it and another, slower, solution for browsers that don't.

Contributor

justinbmeyer commented Dec 23, 2015

Checking out this problem a bit, it doesn't look like there is a "nice" solution. Also, I'm not sure what the behavior should be.

Two-way binding behavior

When Brian is removed, the <select>'s value becomes "". Because of this, I think the right behavior might be to change the two-way bound value (person) to "" if there is an <option value="">. If there isn't an option with a value of "", then no option will be selected and the two-way bound value will be set to undefined.

One-way binding behavior

When Brian is removed, the <select>'s value becomes "". But this should be ignored. When Brian is eventually put back, that <option> should be selected by setting the <select>'s value.

Making it work

Ideally, the bindings could setup a MutationObserver and know whenever the list of options has changed and either update the two-way bound value, or re-set the value.

As that's currently not possible in all the browsers we support, it means I'll have to create a bridge between can.view.live and can.view.bindings. Somehow, can.view.bindings will have to be alerted when some live-binding activity happens within some <select>.

For performance reasons, I want this bridge to run only when explicitly needed. Also, ideally, this bridge would not be super-special cased. There's a strong possibility we'd want to know when some other content is being updated.

My naive approach would be to have can.view.live.html and can.view.live.each simply check their parent. If it's an <select>, look for any callbacks on that option. can.view.bindings would register a callback on <select>'s with bindings:

can.view.live.registerMutationCallback(selectElement, function(){

})

This solution would be decently fast. can.view.live would maintain a "quick check map" of just the elements that get this special callback check.

There's one problem. <options> can exist in <optgroup>s. This wouldn't work because the parent isn't a <select> element.

Fortunately <optgroup>s can't be nested, so we could just check up 2 .parentNodes. However, this is no longer generalized ... or we have to go slow and start walking up the parent chain a lot.

Currently, I'm leaning to one solution that uses MutationObservers for browsers that support it and another, slower, solution for browsers that don't.

@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt Dec 23, 2015

Contributor

Your suggested 2-way binding behavior might be the best option, but what should happen during initialization if the bound value is not present in the options?

For the use cases I described, it would preferably not change the bound value, but showing no selection (selectedIndex = -1) would also mean a "Loading..." option placeholder would not show either, so maybe a dummy select element is the way to go after all.

What would happen if multiple change events caused the list from which the options are derived to change multiple times very quickly? Might that cause the selected value to change to "" unexpectedly or would batching likely prevent that?

Contributor

dylanrtt commented Dec 23, 2015

Your suggested 2-way binding behavior might be the best option, but what should happen during initialization if the bound value is not present in the options?

For the use cases I described, it would preferably not change the bound value, but showing no selection (selectedIndex = -1) would also mean a "Loading..." option placeholder would not show either, so maybe a dummy select element is the way to go after all.

What would happen if multiple change events caused the list from which the options are derived to change multiple times very quickly? Might that cause the selected value to change to "" unexpectedly or would batching likely prevent that?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 23, 2015

Contributor

Your suggested 2-way binding behavior might be the best option, but what should happen during initialization if the bound value is not present in the options?

In this case, if the parent scope has a value, say "foo", the parent value would remain.

Might that cause the selected value to change to "" unexpectedly or would batching likely prevent that?

It might, but hopefully batching would prevent that.

Contributor

justinbmeyer commented Dec 23, 2015

Your suggested 2-way binding behavior might be the best option, but what should happen during initialization if the bound value is not present in the options?

In this case, if the parent scope has a value, say "foo", the parent value would remain.

Might that cause the selected value to change to "" unexpectedly or would batching likely prevent that?

It might, but hopefully batching would prevent that.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 24, 2015

Contributor

Corrected the two-way behavior to account for when <option value=""> is not present.

Contributor

justinbmeyer commented Dec 24, 2015

Corrected the two-way behavior to account for when <option value=""> is not present.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 24, 2015

Contributor

There's a technical problem with one-way bindings. Essentially, with a one-way binding, we need to constantly make sure that when the "child" changes, we try to overwrite that value with the parent.

With a one-way binding, we don't even bind to the child compute. So there's not a great mechanism to make this happen. I'm also not sure what this is conceptually ... it's sort of a "sticky child value" ... a type of child compute that will attempt to take on a certain value if possible. But if not, it can have another value.

I'm trying to think of similar situations to figure out how to build this correctly. For example, <input {$value}="name"/>. Similar to a <select> if a user changes the <input>'s value, the input should not snap back to the parent value. There's not really an analogous situation to <select>'s having their value changed by removing their <option>'s and then having the "sticky value" added back.

To implement the "sticky value" .... I need to probably pass the parentCompute to the childCompute so the childComptue can read the parentCompute at anytime. The other problem is that with one-way bindings, we don't actually bind (and eventually unbind) on the childCompute, so there's not a natural binding lifecycle we can access. This is important because we only want to listen to mutation events when the select has a binding on it.

One possibility might be to just bind the childCompute in these "sticky" cases, but not use it to update the parent.

Contributor

justinbmeyer commented Dec 24, 2015

There's a technical problem with one-way bindings. Essentially, with a one-way binding, we need to constantly make sure that when the "child" changes, we try to overwrite that value with the parent.

With a one-way binding, we don't even bind to the child compute. So there's not a great mechanism to make this happen. I'm also not sure what this is conceptually ... it's sort of a "sticky child value" ... a type of child compute that will attempt to take on a certain value if possible. But if not, it can have another value.

I'm trying to think of similar situations to figure out how to build this correctly. For example, <input {$value}="name"/>. Similar to a <select> if a user changes the <input>'s value, the input should not snap back to the parent value. There's not really an analogous situation to <select>'s having their value changed by removing their <option>'s and then having the "sticky value" added back.

To implement the "sticky value" .... I need to probably pass the parentCompute to the childCompute so the childComptue can read the parentCompute at anytime. The other problem is that with one-way bindings, we don't actually bind (and eventually unbind) on the childCompute, so there's not a natural binding lifecycle we can access. This is important because we only want to listen to mutation events when the select has a binding on it.

One possibility might be to just bind the childCompute in these "sticky" cases, but not use it to update the parent.

@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt Jan 21, 2016

Contributor

Hmm, I guess there are still some cases where the 2-way binding behavior isn't working as expected.

http://jsbin.com/nojikisobe/edit?html,js,console,output

If the parent/scope has a value that is not present in the options initially, the <select> element displays no selected value, which is correct. However, if an option matching the parent/scope value is added...

  1. The <select> element should display that option as selected - it does not
  2. The parent/scope value is being set to the element's value
Contributor

dylanrtt commented Jan 21, 2016

Hmm, I guess there are still some cases where the 2-way binding behavior isn't working as expected.

http://jsbin.com/nojikisobe/edit?html,js,console,output

If the parent/scope has a value that is not present in the options initially, the <select> element displays no selected value, which is correct. However, if an option matching the parent/scope value is added...

  1. The <select> element should display that option as selected - it does not
  2. The parent/scope value is being set to the element's value

@daffl daffl reopened this Jan 29, 2016

@daffl daffl modified the milestones: 2.3.12, 2.3.8 Jan 29, 2016

@daffl daffl removed the fixed in branch label Jan 29, 2016

@daffl daffl closed this in #2226 Feb 1, 2016

@pYr0x

This comment has been minimized.

Show comment
Hide comment
@pYr0x

pYr0x Apr 20, 2016

@justinbmeyer @dylanrtt i think we should document select binding better in the docs... @dylanrtt do you want to add some docs? you are the most familiar guy with that problem

pYr0x commented Apr 20, 2016

@justinbmeyer @dylanrtt i think we should document select binding better in the docs... @dylanrtt do you want to add some docs? you are the most familiar guy with that problem

@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt Apr 20, 2016

Contributor

@pYr0x Since the bugs were fixed, I think the behavior is pretty intuitive now. What more about select binding do you think needs to be documented?

Contributor

dylanrtt commented Apr 20, 2016

@pYr0x Since the bugs were fixed, I think the behavior is pretty intuitive now. What more about select binding do you think needs to be documented?

@pYr0x

This comment has been minimized.

Show comment
Hide comment
@pYr0x

pYr0x Apr 21, 2016

@dylanrtt i run into the issue where a 1-way-binding with a dynamic select list not work.
also what have to document is the default select behavoir if no value exists in can.Map...

pYr0x commented Apr 21, 2016

@dylanrtt i run into the issue where a 1-way-binding with a dynamic select list not work.
also what have to document is the default select behavoir if no value exists in can.Map...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment