[IE 11] DOM does not match up with values returned by the view function when changing the options in a select.

Python-Regius commented 5 years ago

Summary

When changing which options are available to the user on IE11 sometimes other selects end up with the wrong option selected.

Steps to Reproduce

Using Internet Explorer 11 run the test code (at the bottom of the issue), click on the top drop down and first select the option labeled "Three". Then click on the first/top drop-down again and select the option labeled "Zero".

I am using version 2.0.0-rc.6 for this test.

Expected Behavior

The expected behavior is that nothing happens to the second drop-down. This is what I get when running it in Chrome or Firefox.

Current Behavior

The second drop-down changes from "Two" to "One". In the test we serialize allSelectMenus to demonstrate that we are returning correct data. As a part of the data returned by the view function "two" should still be selected in the second menu.

Test code:

<head>
    <title>Select bug</title>
    <script src="mithril.min.js"></script>
</head>
<body>
    <script>
    var root = document.body;

    var allSelections = {"0" : "Zero",
        "1": "One",
        "2": "Two",
        "3": "Three"
    };

    var currentSelections = ["1", "2"];

    function selectValue(idx, value) {
        currentSelections[idx] = value;
    }

    var Selection = {
        view: function() {
            var allSelectMenus = [];
            for (var i = 0; i < currentSelections.length; ++i) {
                var selectOptions = [];
                for (var key in allSelections) {
                    if ((currentSelections[i] != key) && (currentSelections.indexOf(key) >= 0))
                        continue;
                    var optionAttributes = {value: key};
                    var isSelected = (i < currentSelections.length) && (currentSelections[i] == key);
                    optionAttributes['selected'] = isSelected;
                    var optionElm = m('option', optionAttributes, allSelections[key]);
                    selectOptions.push(optionElm);
                }
                var onchangeFunction = new Function('selectValue(' + i + ', this.value)');
                var selectElm = m('select', {onchange: onchangeFunction}, selectOptions);
                allSelectMenus.push(selectElm);
                allSelectMenus.push(m('br'));
            }
            return [allSelectMenus, m('pre', JSON.stringify(allSelectMenus, null, 4))];
        }
    }
    m.mount(root, Selection);
    </script>
</body>

osban commented 5 years ago

@Python-Regius I tried to repro the issue using the IE11 emulator in Edge, but it didn't work...don't know how well that emulator works, though.

Having a lot of business logic in the view is usually not recommended, so I'm wondering if something along the lines of this (as a starting point) would solve the issue?

Python-Regius commented 5 years ago

@osban thank you for the example.

Here is what I ended up running. I needed to use babel because some of the syntax in the example does not work in IE11. In addition the value attribute must be set for all the options otherwise it does not work because it only looks for values in the value attribute. If it is not present IE11 assume the value is blank.

var options = ["Zero", "One", "Two", "Three"];
var selections = [1, 2];
var Selection = {
    view: function view() {
        return [m('div', selections.map(function (x, i) {
            return m('select', {
                onchange: function onchange(e) {
                    return selections[i] = options.indexOf(e.target.value);
                },
                value: options[selections[i]]
            }, options.map(function (y) {
                // return m('option', y);
                return m('option', {value: y}, y); // Otherwise it will not work in IE11.
            }));
        }))];
    }
};
m.mount(document.body, Selection);

After making the changes mentioned above it does work in IE11, however we are not removing the selected value from the other selects. After introducing this change it does not work correctly in any tested browser (Chrome, Firefox, IE11). Normally I would suspect that it is a logic error however I verified that we are returning the correct data using JSON.stringify on the returned array. This one is simpler to reproduce. Just click on the second drop-down and select "Zero". The first drop-down will display "Two" even though below you can see:

"attrs": {
    "value": "One"
},

as a part of the first select element.

var options = ["Zero", "One", "Two", "Three"];
var selections = ["One", "Two"];
var Selection = {
    view: function view() {
        var dropDowns = [m('div', selections.map(function (x, i) {
            return m('select', {
                onchange: function onchange(e) {
                    return selections[i] = e.target.value;
                },
                value: selections[i]
            }, options.filter(function (y) {
                return (y == selections[i]) || (selections.indexOf(y) < 0) }).map(function (y) {
                return m('option', {value: y}, y);
            }));
        }))];
        return [dropDowns, m('pre', JSON.stringify(dropDowns, null, 4))];

    }
};
m.mount(document.body, Selection);

osban commented 5 years ago

@Python-Regius yeah, dynamically changing lists is always tricky...there might be better solutions, but I think this should work. Sorry for the ES6...

dead-claudia commented 5 years ago

Does this repro in v1?

osban commented 5 years ago

Slightly more magical version 😉

Python-Regius commented 5 years ago

Adding the key attribute fixed the two examples I posted in IE11 and for the second example in other browsers as well.

For the two most recently posted examples note that you also need to see the option value attribute even though you set the key so without this modification it does not work correctly in IE11.

Regarding if v1 has the same problem, yes it does (or at-least 1.1.6 does which is what I tested). In the first and second example I get the same problem.

osban commented 5 years ago

@Python-Regius glad it works...good to know the value attribute is still needed.

Python-Regius commented 5 years ago

Now the question is should Mithril be expected to handle this without the key attribute?

dead-claudia commented 5 years ago

Now the question is should Mithril be expected to handle this without the key attribute?

I'm thinking not: <option>s in <select> already have names which in a sense represent the option's "identity" within that select. And of course, when things have "identities" to track like that, you need to use keys.

I would be open to a docs fix warning users to key dynamic <option>s lists, so people are less likely to make this mistake. More generally, you should key all dynamic lists of stateful fragments, including radio buttons, contenteditable elements, and even animated SVGs. For instance, you don't want the m("input[type=radio][name=bar]") to be cleared and replaced when removing the m("input[type=radio][name=foo]") in the fragment [m("input[type=radio][name=foo]"), m("input[type=radio][name=bar]")], and you don't want a rotating cube to restart its animation just because you removed the cube before it. Basically, for dynamic lists, key by default and only remove them if you know the list is static.

(The problem extends beyond that of <option>s, and it doesn't even always involve component state or even JS state. Even private, mutable DOM state with no dependency on JS is sufficient to require keys on dynamic lists, with exceedingly few exceptions.)

Python-Regius commented 5 years ago

I see what you mean about needing to add a key after all even if I were working with HTML only I would still give each select menu a name so I can tell which value is which when submitting a form.

Even with the key attribute when I add a delete functionality I encounter another issue. When I delete a row everything under it gets cleared (it displays "Select a value"). I thought the issue was that the keys were not globally unique (do they need to be?) so I fixed that but I still have the bug. Also I saw in the documentation that for arrays each element in it should be keyed so I am keying the <br> tag and the button. Even with these changes it does not work.

To reproduce this issue: click any of the drop-down delete buttons except the last one and you will notice that the drop downs that are under the one you deleted now display "Select a value" instead of what it should be according to the data returned by the view function.

var options = ['Select a value', 'Zero', 'One', 'Two', 'Three'];
var selections = ['Zero', 'One', 'Two', 'Three'];
var Selection = {
    view: function view() {
        var dropDowns = m('div', selections.map(function (x, i) {
            return [m('button', {key: 'remove' + i, onclick: function() {
                    selections.splice(i, 1);
                }}, 'delete'),
                    m('select', {
                onchange: function onchange(e) {
                    return selections[i] = e.target.value;
                },
                value: x, key: i
            }, options.filter(function (y) {
                return (y == selections[i]) || (selections.indexOf(y) < 0);
            }).map(function (z) {
                return m('option', {
                    key: i + z, value: z
                }, z);
            })), m('br', {key: 'br' + i})];
        }));
        return [dropDowns, m('pre', JSON.stringify(dropDowns, null, 4))];
    }
};
m.mount(document.body, Selection);

Thank you @osban and @isiahmeadows for your help so far.

dead-claudia commented 5 years ago

@Python-Regius

I thought the issue was that the keys were not globally unique (do they need to be?)

Yes, they need to be unique. (HTML wants them to be unique anyways, or you'll have issues there, too.)

As for your code snippet, you add keys to only the dynamic part, the fragment you're returning. To add a key to a fragment, you use m.fragment({key: ...}, [...]) instead of just [...] (which is equivalent to m.fragment([...]). To see this in action and why things need to be the way they are, check out this flems. (I also stripped some of the extra whatever in it for clarity.)

Python-Regius commented 5 years ago

Thank you for the example. Unmodified it works great including in IE11 when transpiling.

To get it to work with dynamic options I had to make one modification otherwise I got the same problem as before (all the selects below the deleted one get reset to the first option).

For the fragment key change: {key: i} to: {key: selections[i]}. This only works because I don't allow the same value to be selected twice within the group of selects. Otherwise the key would not be unique.

I am guessing that the reason why this change makes it work is because it is now obvious which row is deleted because it can be detected by the absence of the key which previously existed.

This time I started by modifying the ES6 version which I am posting so that it is easier to see the two changes I made to @isiahmeadows's example. I also have an ES5 version that works great in IE11.

const options = ['Select a value', 'Zero', 'One', 'Two', 'Three'];
const selections = ['Zero', 'One', 'Two', 'Three'];
const Selection = {
    view() {
        const dropDowns = m('div', selections.map((x, i) => {
            // Key goes here: this is a dynamic, stateful part
            // of the `div`'s children'.
            return m.fragment({key: selections[i]}, [ 
                // No key goes here: it's just a static bit of the
                // inner fragment.
                m('button', {
                    onclick() { selections.splice(i, 1) },
                }, 'delete'),
                // No key goes here: it's just a static bit of the
                // inner fragment. Even though it's stateful, it
                // doesn't change place relative to the fragment
                // itself, hence why I'm calling it "static".
                m('select', {
                    onchange: e => selections[i] = e.target.value,
                    value: x,
                }, options.filter(y => (y == selections[i]) || (!selections.includes(y))).map(v =>
                    // Key goes here: this is a dynamic, stateful part
                    // of the `<select>`, and could change if you
                    // change the `options` list for some reason.
                    m('option', {key: v, value: v}, v)
                )),
                m('br'),
            ])
        }))
        return [
            dropDowns,
            m('pre', JSON.stringify(dropDowns, null, 4))
        ]
    },
}
m.mount(document.body, Selection);

dead-claudia commented 5 years ago

@Python-Regius If it helps, Mithril doesn't look into inner fragments (read: arrays) to determine keys. It just looks at the immediate list of children to figure out if those have keys.

So if you flattened your children array and just keyed all the fragment's children, you'd end up with a similar (working) result.

Of course, I didn't include the select changing the number of allowed options that you presumably need, but that's easy enough to add and it doesn't really add anything to my example.

Python-Regius commented 5 years ago

Thank you again for the example. It does work with dynamic options but I needed to make the same change as mentioned above where I changed `select ${i}` to `select ${selections[i]}` for the select key. Otherwise the options below it will get reset.

Is Mithril supposed to correctly handle this when using `select {i}` as the key?

Here is the modified code based on the example.

function flatMap(list, func) {
    return [].concat(...list.map(func))
}
const options = ['Select a value', 'Zero', 'One', 'Two', 'Three'];
const selections = ['Zero', 'One', 'Two', 'Three'];
const Selection = {
    view() {
        const dropDowns = m('div', flatMap(selections, (x, i) => [
            m('button', {
                key: `button ${i}`,
                onclick() { selections.splice(i, 1) },
            }, 'delete'),
            m('select', {
                key: `select ${selections[i]}`,
                onchange: e => selections[i] = e.target.value,
                value: x,
            }, options.filter(y => (y == selections[i]) || (!selections.includes(y))).map(v =>
                m('option', {key: v, value: v}, v)
            )),
            m('br', {key: `br ${i}`}),
        ]))
        return [
            dropDowns,
            m('pre', JSON.stringify(dropDowns, null, 4))
        ]
    },
}
m.mount(document.body, Selection);

dead-claudia commented 5 years ago

@Python-Regius

Yes. Keep in mind, with my flatMap, what Mithril sees is this for the outer tree:

m('div', [
    m('button', {key: 'button Zero'}, 'delete'),
    m('select', {key: 'select Zero', value: 'Zero'}, [...]),
    m('br', {key: 'br Zero'}),

    m('button', {key: 'button One'}, 'delete'),
    m('select', {key: 'select One', value: 'One'}, [...]),
    m('br', {key: 'br One'}),

    m('button', {key: 'button Two'}, 'delete'),
    m('select', {key: 'select Two', value: 'Two'}, [...]),
    m('br', {key: 'br Two'}),

    m('button', {key: 'button Three'}, 'delete'),
    m('select', {key: 'select Three', value: 'Three'}, [...]),
    m('br', {key: 'br Three'}),
])

And for each tree inside the selects, Mithril sees something like this:

m('select', {...}, [
    m('option', {key: 'Select a value', value: 'Select a value'}, 'Select a value'),
    m('option', {key: 'Zero', value: 'Zero'}, 'Zero'),
])

For my suggestion to use fragments, here's what Mithril sees:

m('div', [
    m.fragment({key: 'Zero'}, [...])
    m.fragment({key: 'One'}, [...])
    m.fragment({key: 'Two'}, [...])
    m.fragment({key: 'Three'}, [...])
])

Within each fragment, Mithril sees just an unkeyed list of children:

m.fragment({...}, [
    m('button', {...}, 'delete'),
    m('select', {value: 'Zero'}, [...]),
    m('br'),
])

And within each select, Mithril sees the same as earlier:

m('select', {...}, [
    m('option', {key: 'Select a value', value: 'Select a value'}, 'Select a value'),
    m('option', {key: 'Zero', value: 'Zero'}, 'Zero'),
])

That's why it works that way.

Oh, and apologies - I missed the bug where I was providing the index rather than the value as the key.

Python-Regius commented 5 years ago

Thank you for the clarification. At this point it seems like we ruled out that any of the behavior that I was seeing was due to a bug within Mithril. Would you like me to close this issue?

dead-claudia commented 5 years ago

Not yet. There's still a docs issue present here, one I'd like to address first.

MithrilJS / docs