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

Select box options not matching state on select change event #1916

Closed
robinchew opened this Issue Jul 24, 2017 · 11 comments

Comments

Projects
None yet
4 participants
@robinchew
Copy link
Contributor

robinchew commented Jul 24, 2017

I have a select box with options 0 to 5. Select box onchange event will always set the selected state to 5. On first selection of any option, it does select 5, however not in subsequent selections.

Expected Behavior

On any subsequent selections, the option should remain as 5.

Current Behavior

However, subsequent selections, does not remain 5.

Possible Solution

Hoping i'm not the one to fix it.

Steps to Reproduce (for bugs)

https://jsfiddle.net/7tj9pyet/ with Mithril 1.1.3, tested on Chromium and Vivaldi.

@ArthurClemens

This comment has been minimized.

Copy link
Contributor

ArthurClemens commented Jul 24, 2017

When you replace selected with disabled it does work for that attribute. Somehow selected is removed.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Jul 24, 2017

I found what the issue is: Mithril does have special handling for selected, but it's only if the field is the currently active element (and <option>s are not themselves the active element - it's always the parent <select>). Here's the offending line.

@isiahmeadows isiahmeadows added the bug label Jul 24, 2017

@robinchew

This comment has been minimized.

Copy link
Contributor

robinchew commented Jul 25, 2017

Thanks @isiahmeadows. Is removing && vnode.dom === $doc.activeElement considered a fix?

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Jul 25, 2017

Good catch @robinchew, and @isiahmeadows is correct (search for vnode.dom.parentElement for the fix)

@robinchew

This comment has been minimized.

Copy link
Contributor

robinchew commented Jul 25, 2017

Thanks @pygy. Should I create a PR based on your fix?

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Jul 25, 2017

@robinchew 👍 PR most welcome! I can help with the tests if needed (they should go in render/tests/test-attributes.js)...

I hadn't yet seen your previous message before posting mine. Removing the && vnode.dom === $doc.activeElement would also fix it, I don't know which version would be faster...

@robinchew

This comment has been minimized.

Copy link
Contributor

robinchew commented Jul 26, 2017

@pygy I tried to write the test, but I hit a problem the moment I do:

o("render select options", function() {
    var select = {tag: "select", children: [
        {tag:"option", attrs: {value: "1", selected: ""}},
    ]}
    render(root, select);
})
TypeError: Cannot set property 'selectedIndex' of null
    at Object.set [as selected] (/home/robin/work/mithril.js/test-utils/domMock.js:471:43)
    at setAttr (/home/robin/work/mithril.js/render/render.js:503:17)
    at setAttrs (/home/robin/work/mithril.js/render/render.js:470:4)
    at createElement (/home/robin/work/mithril.js/render/render.js:89:4)
    at createNode (/home/robin/work/mithril.js/render/render.js:39:21)
    at createNodes (/home/robin/work/mithril.js/render/render.js:26:5)
    at createElement (/home/robin/work/mithril.js/render/render.js:104:5)
    at createNode (/home/robin/work/mithril.js/render/render.js:39:21)
    at createNodes (/home/robin/work/mithril.js/render/render.js:26:5)
    at updateNodes (/home/robin/work/mithril.js/render/render.js:149:25)

Sounds like I need to mess with domMock which I think I won't. Can you take over? Also how do you run just a single test? I don't think you need me to remove && vnode.dom === $doc.activeElement for you.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Jul 26, 2017

It's a pretty easy fix to the mock: just add support for parentNode (which is pretty trivial and exactly what you might think). @pygy I'm already on it BTW.

Edit: I clearly missed this...so nope, it doesn't need fixed. 😄

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Jul 26, 2017

@robinchew

This comment has been minimized.

Copy link
Contributor

robinchew commented Jul 27, 2017

The parentNode key is there but it has null value, and the render function wants to get selectedIndex of parentNode, hence the error above. Sorry I'm not sure what you mean by trying vnode.dom.parentNode, instead of what?

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Jul 27, 2017

@robinchew I think he meant instead of parentElement that I use in my flems... thanks for giving it a try :-)

robinchew added a commit to robinchew/mithril.js that referenced this issue Jul 28, 2017

robinchew added a commit to robinchew/mithril.js that referenced this issue Aug 26, 2017

@isiahmeadows isiahmeadows referenced this issue Oct 1, 2017

Open

Tracking bug for DOM mock issues #1978

1 of 2 tasks complete

robinchew added a commit to robinchew/mithril.js that referenced this issue Oct 25, 2017

robinchew added a commit to robinchew/mithril.js that referenced this issue Oct 25, 2017

@pygy pygy closed this in db2a12d Oct 31, 2017

@pygy pygy referenced this issue Dec 8, 2017

Merged

render: fix perf regression introduced by #1918 #2052

4 of 10 tasks complete

robinchew added a commit to robinchew/mithril.js that referenced this issue Aug 4, 2018

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this issue Oct 12, 2018

Trying to fix MithrilJS#1916 (MithrilJS#1918)
* Trying to fix MithrilJS#1916

* Remove test for rendering select options. Add back after resolving issue MithrilJS#1978.

* Add MithrilJS#1916 fix to change log.

* Revert "Remove test for rendering select options. Add back after resolving issue MithrilJS#1978."

This reverts commit d4c1be7.

* Comment on why failing test for MithrilJS#1916 is commented out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment