What about updating events that attached to new HTML structure?
jmas opened this issue · 16 comments
I tried to rewrite one pice of my application to using diffhtml.element(rootEl, newEl) and result is awesome. It do what I looking for... Except one major thing: diffhtml do not update early attached DOM events.
For example after first call diffhtml.element(rootEl, newEl) I get a list of records with attached events. Each attached event handler contain information of current record. If I click on the record - I can do some workaround for this record. Then I add new first record to the list. Actually new record just update an old HTML structure of old first record with old event listeners. So when I click on new first record - actually I call old event handler with information of old fist record.
Maybe we can add optional setup parameter that switch diffhtml to mode that do refresh event listeners when we do diffhtml.element(rootEl, newEl)?
Hey @jmas glad to hear diffHTML sort of worked for you :-p. I'm a bit confused as to what you're saying/asking for diffHTML to do differently. I'm not sure what an early attached DOM event is. diffHTML won't diff any DOM events, unless they are DOM attributes.
For instance these events are diffable:
const onClick = (e) => {};
diff.outerHTML(document.body, html`<body onclick=${onClick}>
<span>Click me</span>
</body>`);
window.clicked = (e) => {};
const newBody = document.createElement('body');
newBody.setAttribute('onclick', 'clicked');
diff.element(document.body, newBody);This however is not diffable:
const newBody = document.createElement('body');
newBody.addEventListener('click', (e) => {});
diff.element(document.body, newBody);The primary reason for this is that I'm pretty sure it's impossible to get events from a DOM node. If you can figure out a way, let us know!
Btw, I wouldn't anticipate this being a feature added into the core library. I would definitely explore wiring this up as middleware tho.
Thanks @tbranyen for quick response. Yep... I got what you mean. So event listeners that was added by addEventListener is do not sync.
But I add an event by element.onclick = function() { ... } notation. Here is quick example that I have:
function createFormEl (data) {
var el = document.createElement('DIV');
el.innerHTML = '<form class="js-form"><input type="submit" /></form>';
el.querySelector('.js-form').onsubmit = function (event) {
event.preventDefault();
alert('Submit: ' + data.id);
};
return el;
}
var actualFormEl = createFormEl({ id: 1 });
var newFormEl = createFormEl({ id: 2 });
var rootEl = document.getElementById('root');
diffhtml.element(rootEl, actualFormEl);
diffhtml.element(rootEl, newFormEl);Right that's a property not an attribute. So, I could see middleware to handle DOM property synchronization, but that seems tricky to get right. Maybe we can start with just events tho? Basically the question is how do you separate DOM properties not meant to be sync'd from custom properties attached by you, the user?
Also a hardcoded list of events will surely be out-of-date when something new comes out, and I'd hate to manually track that. Maybe we can extract the list by keying off of on in a feature test?
I found this:
http://stackoverflow.com/questions/9368538/getting-an-array-of-all-dom-events-possible
const names = Object.getOwnPropertyNames(document).concat(Object.getOwnPropertyNames(Object.getPrototypeOf(Object.getPrototypeOf(document))));
names.filter(i => !i.indexOf('on') && (document[i]==null||typeof document[i]=='function'))["onreadystatechange", "onpointerlockchange", "onpointerlockerror", "onbeforecopy", "onbeforecut", "onbeforepaste", "oncopy", "oncut", "onpaste", "onsearch", "onselectionchange", "onselectstart", "onwheel", "onwebkitfullscreenchange", "onwebkitfullscreenerror", "onabort", "onblur", "oncancel", "oncanplay", "oncanplaythrough", "onchange", "onclick", "onclose", "oncontextmenu", "oncuechange", "ondblclick", "ondrag", "ondragend", "ondragenter", "ondragleave", "ondragover", "ondragstart", "ondrop", "ondurationchange", "onemptied", "onended", "onerror", "onfocus", "oninput", "oninvalid", "onkeydown", "onkeypress", "onkeyup", "onload", "onloadeddata", "onloadedmetadata", "onloadstart", "onmousedown", "onmouseenter", "onmouseleave", "onmousemove", "onmouseout", "onmouseover", "onmouseup", "onmousewheel", "onpause", "onplay", "onplaying", "onprogress", "onratechange", "onreset", "onresize", "onscroll", "onseeked", "onseeking", "onselect", "onshow", "onstalled", "onsubmit", "onsuspend", "ontimeupdate", "ontoggle", "onvolumechange", "onwaiting"]
@tbranyen Yep... And you can just do re-attaching all of that attributes that !null. Or it is overkill?
So maybe what we could do is write a middleware like this:
diff.use(({ newHTML, node }) => sync => patch => finish => {
if (!newHTML.ownerDocument) { return; }
const names = Object.getOwnPropertyNames(document).concat(Object.getOwnPropertyNames(Object.getPrototypeOf(Object.getPrototypeOf(document))));
const events = names.filter(i => !i.indexOf('on') && (document[i]==null||typeof document[i]=='function'));
events.forEach((eventName) => {
node[eventName] = newHTML[eventName];
});
})Can you give that a shot and see if it works?
@tbranyen Very wired. I did some modifications:
diffhtml.use(({ newHTML, node }) => sync => patch => finish => {
if (!newHTML.ownerDocument) { return; }
const names = Object.getOwnPropertyNames(document).concat(Object.getOwnPropertyNames(Object.getPrototypeOf(Object.getPrototypeOf(document))));
const events = names.filter(i => !i.indexOf('on') && (document[i]==null||typeof document[i]=='function'));
events.forEach((eventName) => {
if (newHTML[eventName]) {
node[eventName] = newHTML[eventName];
}
});
})
function createForm (data) {
var el = document.createElement('FORM');
el.innerHTML = '<input type="submit" value="Submit '+data.id+'" />';
el.onsubmit = function (event) {
event.preventDefault();
alert('Submit: ' + data.id);
};
return el;
}
var rootEl = document.getElementById('a');
diffhtml.element(rootEl, createForm({ id: 1 }), { inner: true });
setTimeout(() => {
diffhtml.element(rootEl, createForm({ id: 2 }), { inner: true });
}, 500);But currently when I click button it alert twice: Submit: 1 and Submit 2. I always think that element.oclick=handler; or similar just rewrite previous handler, not attach another one.
Well yeah you wrote logic in your if statement to block removing. ;-) Remove your if statement and all will be well.
@tbranyen I got where was a problem. When I add option { inner: true } to avoid lost root element - it pass root element to node, so obsubmit event was attached twice to form and to root container.
So middleware is potentially works. The final code:
diffhtml.use(({ newHTML, node }) => sync => patch => finish => {
if (!newHTML.ownerDocument) { return; }
const names = Object.getOwnPropertyNames(document).concat(Object.getOwnPropertyNames(Object.getPrototypeOf(Object.getPrototypeOf(document))));
const events = names.filter(i => !i.indexOf('on') && (document[i]==null||typeof document[i]=='function'));
events.forEach((eventName) => {
node[eventName] = newHTML[eventName];
});
})
function createForm (data) {
var el = document.createElement('FORM');
el.id = 'form-'+data.id;
el.innerHTML = '<span>'+data.id+'</span><input type="submit" value="Submit '+data.id+'" />';
el.onsubmit = function (event) {
event.preventDefault();
alert('Submit: ' + data.id);
};
return el;
}
var rootEl = document.getElementsByTagName('form')[0];
diffhtml.element(rootEl, createForm({ id: 1 }));
setTimeout(() => {
var rootEl = document.getElementsByTagName('form')[0];
diffhtml.element(rootEl, createForm({ id: 2 }));
}, 500);But it works only for root elements. I need to make it worked for all elements. It is very basic example. There is more complex:
function createPostEl (post, user) {
var el = createEl(config.postTemplate);
el.querySelector('.js-post-username').innerHTML = post.username || config.defaultUsername;
el.querySelector('.js-post-body').innerHTML = escapeHtml(post.body);
el.querySelector('.js-post-user-image').src = post.userId ? config.avatarUrl+post.userId: config.defaultUserImage;
el.querySelector('.js-post-add-button').onclick = function (event) {
event.preventDefault();
var formPlaceholderEl = el.querySelector('.js-post-comment-form');
formPlaceholderEl.innerHTML = post.id;
formPlaceholderEl.appendChild(createCommentFormEl(post, user));
};
if (post.comments) {
var commentsEl = el.querySelector('.js-post-comments');
post.comments.forEach(function (comment) {
commentsEl.appendChild(createCommentEl(comment));
});
}
return el;
}You see that element with className .js-post-add-button is not a root element and middleware do not work for this case.
True, true. I do think you'll want some kind of event delegation here. Alternatively you'll need to recursively crawl the children of each element to ensure you properly account for all the events.
Yes. And it is not to easy compare two DOM trees. I need to walk around first tree and compare elements with another tree. But if differences are too big... Maybe this task is more convenient to do when diffhtml compare two dom trees?
So I figure out how is diffhtml event system is working and how to work with it to avoid attaching duplicate events. Now I doing it in following way:
function createElement () {
return html`
<button onclick=${someEvent}>Click</button>
`;
}Thanks for helping.