Add support for new elements in event-valuechange
#1184
Conversation
* Tests for subscriber and delegation on <select> elements * Tests for subscriber and delegation on [contenteditable="true"] elements.
* Supports <select> elements with different <option> choices * Supports all elements with [contenteditable="true"] Maintains performance by accessing raw DOM properties (that are supported in all browsers).
* Added tests for <select> elements with <option> choices * Added tests for [contenteditable="true"] elements
| @@ -96,6 +97,16 @@ VC = { | |||
|
|
|||
| prevVal = vcData.prevVal; | |||
|
|
|||
| if (VC._isEditable(node)) { | |||
There was a problem hiding this comment.
It shouldn't be necessary to check _isEditable() each time you poll. If you just cache the check in _startPolling(), that should be sufficient.
* Caches `tagName` and `isEditable` inside of the node * Uses `tagName` to determine how to extract the `newVal` * Replaces all `getHTML` instances with `innerHTML`
|
@rgrove I made the caching improvements that you suggested by adding I'm still thinking about what the best way is to handle the last case that you suggested. |
| // Back-compatibility with IE6 <select> element values. | ||
| // Huge performance cheat to get past node.get('value'). | ||
| selectedOption = domNode.options[domNode.selectedIndex]; | ||
| newVal = selectedOption.value || selectedOption.text; |
There was a problem hiding this comment.
What happens here if selectedOption.value is an empty string? Does selectedOption.text return a string on all browsers? If not, this could end up setting newVal to undefined, which wouldn't be good.
There was a problem hiding this comment.
Yeah, if you have no value, and no text inside of an option, like this:
<select>
<option></option>
</select>selectedOption.text will return an empty string ('') on all A-grade browsers. I'm looking for the spec to prove it, but this was manually tested too.
|
Don't forget to update the user guide (and you might want to add a caveat about elements that are added/removed/changed after a delegated listener is attached), but after that: 👍 Thanks @clarle! |
| var domNode = node._node, // performance cheat; getValue() is a big hit when polling | ||
| event = options.e, | ||
| vcData = node._data && node._data[DATA_KEY], // another perf cheat | ||
| tagName = vcData.tagName, |
There was a problem hiding this comment.
Not that it really matters, but I'd probably use nodeName here instead. Why? nodeName is actually part of the DOM Level 3 standard (http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-F68D095), where as tagName isn't.
* Used standards-based `nodeName` instead of `tagName` * Used better boolean statement in `_poll`
|
Sweet! I'm glad this landed. @apipkin can you take advantage of the contenteditable support now? |
As discussed in #1157:
This PR adds support for two groups of new elements for
event-valuechange,<select>elements with<option>choices, and any element with the[contenteditable="true"]attribute.The
<select>elements work nearly the same way as<input>and<textarea>, which basically checks theirvalueattribute for any change. There are some performance cheats involved in the polling loop to improve performance on IE6 without having to resort to usingnode.get('value'), and have been fully tested.The
[contenteditable="true"](or[contenteditable=""]) elements act differently, and fires a change when theirinnerHTMLproperty changes. For now, there isn't support for elements that inheritcontenteditablefrom a parent element. It's something I could add, but I'm worried about the performance hit on having to search up the DOM tree to check whether or not the element has a parent node that has[contenteditable="true"]. If that's something that might be useful, though, let me know, and I'll see if I can find an efficient way to do that.This was tested on all A-grade browsers (including IE6) and passes all unit tests and manual tests for each one. There were no performance hits even after adding the new functionality in.
The goal for this enhancement is mainly so that we can use the
valuechangeevent to efficiently detect form element changes when implementing two-way data-binding later on.