gooact icon indicating copy to clipboard operation
gooact copied to clipboard

Improve loss-of-state, avoid focus work-around

Open mindplay-dk opened this issue 6 years ago • 2 comments

You invited me to open issues (on Medium) so here's the first one :-)

This is regarding the loss-of-focus work-around in the patch function:

        const active = document.activeElement;
        // ...
        [].concat(...vdom.children).map((child, index) => {
            const key = child.props && child.props.key || `__index_${index}`;
            dom.appendChild(pool[key] ? patch(pool[key], child) : render(child, dom));
            delete pool[key];
        });
        // ...
        active.focus();

Here, every child-node is getting repositioned with a call to appendChild - whether the node in question has actually moved or not.

Changing the position of a node in the document causes loss of state, which you can partially work around with activeElement and focus() to restore input focus after losing it - however, there is also loss of selection state and cursor position etc. to consider.

Here's an example to illustrate the problem:

https://codesandbox.io/s/rwjr4xo47p

After typing into the input, try selecting the text you typed in, which is displayed below - you will end up selecting the entire contents of the document, because all the nodes are changing places every second.

Possible approach to fixing this: avoid appendChild and conditionally use insertBefore instead - for example, you could track the previous node by assigning it to a variable, e.g. last_dom = dom, then call dom.insertBefore(new_dom, last_dom.nextSibling) only after checking if (new_dom.previousSibling !== last_dom) ... in plain english, check if the patched/rendered DOM node's previous sibling is already the DOM node from the last iteration, and only position it after the last DOM node if it isn't.

What do you think?

mindplay-dk avatar Jul 19 '18 16:07 mindplay-dk

You're right, but I am not sure how should I handle key reordering properly in this case. Anyway, it is a good thing to think about, I am going to investigate this one.

sweetpalma avatar Jul 21 '18 15:07 sweetpalma

I think I would suggest doing this in two iterations.

During the first iteration, only create (don't position) and remove nodes, a build a list of ordered keys.

Then iterate over the ordered keys and check if each node's previousSibling is what it should be - if not, reposition it. Because garbage nodes have been removed already, they won't get in the way.

What do you think, would that work?

mindplay-dk avatar Oct 29 '18 16:10 mindplay-dk