Skip to content
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

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

Open
mindplay-dk opened this issue Jul 19, 2018 · 2 comments
Open

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

mindplay-dk opened this issue Jul 19, 2018 · 2 comments

Comments

@mindplay-dk
Copy link

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?

@sweetpalma
Copy link
Owner

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.

@mindplay-dk
Copy link
Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants