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

Add support for new elements in `event-valuechange` #1184

Merged
merged 6 commits into from Sep 16, 2013

Conversation

Projects
None yet
4 participants
@clarle
Copy link
Contributor

clarle commented Sep 11, 2013

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 their value attribute for any change. There are some performance cheats involved in the polling loop to improve performance on IE6 without having to resort to using node.get('value'), and have been fully tested.

The [contenteditable="true"] (or [contenteditable=""]) elements act differently, and fires a change when their innerHTML property changes. For now, there isn't support for elements that inherit contenteditable from 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 valuechange event to efficiently detect form element changes when implementing two-way data-binding later on.

Clarence Leung added some commits Sep 11, 2013

Clarence Leung
Add tests for new `event-valuechange` elements
* Tests for subscriber and delegation on <select> elements
* Tests for subscriber and delegation on [contenteditable="true"]
  elements.
Clarence Leung
Implement new element support in event-valuechange
* 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).
Clarence Leung
Add new element manual tests for event-valuechange
* 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)) {

This comment has been minimized.

@rgrove

rgrove Sep 11, 2013

Contributor

It shouldn't be necessary to check _isEditable() each time you poll. If you just cache the check in _startPolling(), that should be sufficient.

} else {
// Back-compatibility with IE6 <select> element values.
// Huge performance cheat to get past node.get('value').
newVal = domNode.value || domNode.options[domNode.selectedIndex].value

This comment has been minimized.

@rgrove

rgrove Sep 11, 2013

Contributor

Seems like this fallback will fail if an <input> has an empty value. It would probably be best to check the node type when polling begins, then cache it and use that to determine the polling behavior.

var vcData,
isEditable = VC._isEditable(node);

if (!node.test('input,textarea,select') && !isEditable) {

This comment has been minimized.

@rgrove

rgrove Sep 11, 2013

Contributor

This logic could be improved to skip the _isEditable() call if the node matches the "input,textarea,select" selector.


if (!vcData) {
vcData = {prevVal: node.get(VALUE)};
vcData = { prevVal: (isEditable ? node.getHTML() : node.get(VALUE)) };

This comment has been minimized.

@rgrove

rgrove Sep 11, 2013

Contributor

Seems like you should probably use innerHTML both here and in _poll(), since the values will be compared to determine whether something changed. I don't recall off the top of my head whether getHTML() does any fixups that might cause it to return something different from innerHTML, but better safe than sorry.


if (!vcData) {
vcData = {};
node.setData(DATA_KEY, vcData);
}

vcData.prevVal = node.get(VALUE);
vcData.prevVal = (VC._isEditable(node) ? node.getHTML() : node.get(VALUE));

This comment has been minimized.

@rgrove

rgrove Sep 11, 2013

Contributor

Same advice here re. innerHTML vs. getHTML().

if (!child.getData(DATA_KEY)) {
child.setData(DATA_KEY, {prevVal: child.get(VALUE)});
child.setData(DATA_KEY, { prevVal: (isEditable ? child.getHTML() : child.get(VALUE)) });

This comment has been minimized.

@rgrove

rgrove Sep 11, 2013

Contributor

...and here. :)

return;
}

if (!node.getData(DATA_KEY)) {
node.setData(DATA_KEY, {prevVal: node.get(VALUE)});
node.setData(DATA_KEY, { prevVal: (isEditable ? node.getHTML() : node.get(VALUE)) });

This comment has been minimized.

@rgrove

rgrove Sep 11, 2013

Contributor

...and here!

@@ -399,28 +435,32 @@ VC = {
// Add a function to the notifier that we can use to find all
// nodes that pass the delegate filter.
_valuechange.getNodes = function () {
return node.all('input,textarea').filter(filter);
inputNodes = node.all('input,textarea,select').filter(filter);
editableNodes = node.all('[contenteditable="true"],[contenteditable=""]').filter(filter);

This comment has been minimized.

@rgrove

rgrove Sep 11, 2013

Contributor

Hmm. What happens if a non-editable node becomes editable after a delegated subscription is attached? Or an editable node becomes non-editable? This will only catch nodes that are editable at the time of the subscription, and it assumes they'll always be editable.

This comment has been minimized.

@clarle

clarle Sep 12, 2013

Contributor

I'm not sure how to resolve this problem other than possibly polling the entire page at some interval.

The event won't fire, at least, if a [contenteditable="true"] node becomes [contenteditable="false"] somewhere along the line. For the case where a non-editable node becomes editable...that's probably a problem, but that would require at least polling/setting a event-handler on each node for contenteditable changes, which isn't great.

This comment has been minimized.

@rgrove

rgrove Sep 12, 2013

Contributor

The current version only works on input and textarea fields, which can't become something else in the same way that an element can have the contenteditable attribute applied or removed. It's true that the current version won't notice when an element is added to or removed from the page though, so maybe this caveat is similar.

This comment has been minimized.

@ericf

ericf Sep 12, 2013

Member

The delegated focus event listener should be able to handle this since contentedtiable="true" elements fire the focus event: http://jsbin.com/awOVUNi/1/edit

This comment has been minimized.

@clarle

clarle Sep 16, 2013

Contributor

@ericf was right after testing it, shouldn't have spoken so soon. Like he said, it works because of the delegated focus event listener.

Clarence Leung
Add caching and replace `getHTML` with `innerHTML`
* Caches `tagName` and `isEditable` inside of the node
* Uses `tagName` to determine how to extract the `newVal`
* Replaces all `getHTML` instances with `innerHTML`
@clarle

This comment has been minimized.

Copy link
Contributor

clarle commented Sep 12, 2013

@rgrove I made the caching improvements that you suggested by adding tagName and isEditable into vcData, and replaced all instance of node.getHTML() with just innerHTML.

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;

This comment has been minimized.

@rgrove

rgrove Sep 12, 2013

Contributor

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.

This comment has been minimized.

@clarle

clarle Sep 12, 2013

Contributor

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.

This comment has been minimized.

@rgrove

rgrove Sep 12, 2013

Contributor

Cool, sounds good then!

@rgrove

This comment has been minimized.

Copy link
Contributor

rgrove commented Sep 12, 2013

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,

This comment has been minimized.

@ezequiel

ezequiel Sep 12, 2013

Contributor

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.

vcData = node._data && node._data[DATA_KEY], // another perf cheat
tagName = vcData.tagName,
stopped = 0,
facade, prevVal, newVal, selectedOption, stopElement;

if (!domNode || !vcData) {

This comment has been minimized.

@ezequiel

ezequiel Sep 12, 2013

Contributor
!(domNode && vcData)

Clarence Leung added some commits Sep 16, 2013

Clarence Leung
Made changes based on @ezequiel's comments
* Used standards-based `nodeName` instead of `tagName`
* Used better boolean statement in `_poll`
@ericf

This comment has been minimized.

Copy link
Member

ericf commented Sep 25, 2013

Sweet! I'm glad this landed. @apipkin can you take advantage of the contenteditable support now?

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