Fix selection on mobile - selectionchange events are now fired and fo…

[lustoykov]

Fix selection on mobile - selectionchange events are now fired and formatted correctly

@jhchen can you look at this PR and see if we can merge it? The issue encountered was that there is selectionchange event missing, which breaks the functionality on mobile. When user drags the cursor selection position is not being updated.

Reproduce bug:
Select a word. Drag the cursor to the end of the sentence. Try to format - fails. I couldn't think of a way to write test for that, cause the failure is actually that the update() method is not being called on selectionchange. If you have any ideas how to add tests for that, I'll do it.

Also, fixes #1450

I suspect this fixes other mobile issues such as #1423 or #1374, but can't confirm it yet - since I haven't explicitly tested.

provided by @gutefrage :-)

[jhchen]

I have considered removing all the other listeners in favor of selectionchange. However the issue is since it is being attached globally, this will leak memory in single page apps. One solution is to implement an abstraction that utilizes event delegation to only attach a listener once. If you would like to take this on I can provide more details.

I would be opposed to adding a destroy function as I think that is unnecessarily lazy and just passes work onto all users of Quill.

[lustoykov]

@jhchen thanks for the response, I agree on all your points - especially about the memory leak and unnecessary manual destroy() call. In fact, we're using the editor in a SPA context with multiple instances as well.

Can you provide more details how you envision a possible solution and then I'll take on that?

Thanks.

[jhchen]

The main idea is maintaining a WeakMap of handlers and only listen once to and event and delegate to those handlers. WeakMap's properties should handle the memory leak problem for us. One issue to avoid is we cannot use the target node as the key for WeakMap since the target node may be document (as in the case of selectionchange) so we have to use something that would be garbage collected, like the quill instance or quill's emitter instance.

Starting with just handling one event selectionchange to start simple (though this can be generalized to any event with event delegation), one implementation would be:

• Add a static listeners WeakMap that will maintain a mapping of Emitter instances to event handlers.
• Add an event listener on document with a handler that will query all ql-editor dom nodes and go through each of them checking if they have an entry in listeners and if so call the handler.
• Add an API to an emitter instance onSelectionChange(root, handler). When it is called, add a new entry to listeners keyed by the quill.root domNode (also scroll.domNode or selection.root) with handler as the value.

Let me know if this makes sense.

[jhchen]

Can you let me know if you have time to try this @lustoykov ? Otherwise I will have someone else do it.

[lustoykov]

Hey @jhchen sorry for the late response... we're in the middle of a big release (in fact, releasing quill to our users 🎉 ) so couldn't get to this PR yet...

Just to clarify, for the first simple case with only selection change, you're envisioning something along the lines of ...

// create a wrapper to hold a WeakMap of handlers
class EventHandler { 
    private events = new WeakMap();
    // ... 
}

then somewhere delegate to a handler on selectionchange...

const selectionChangeKey = /*some key that will be garbage collected after quill is gone */;
EventHandler.set(selectionChangeKey, selectionChangeHandler);
 // delegation happens here
document.addEventListener('selectionchange', EventHandler.get(selectionChangeKey)); 

This is very oversimplified, but basically the idea is, once quill is gone, EventHandler.get(selectionChangeKey) handler would be garbage collected, therefore nothing will point to this / the quill instance or its properties anymore, thus will be cleaned up from memory, are we on the same page?

In any case we are going to fix this properly in accordance to your suggestion and at some point it will be a very high priority task for us, but can't promise that I'll manage to do it in next couple of weeks. I'll make a PR with proper fix then if there isn't one yet, so in this case I think you can close this one 👍

[jhchen]

I think that should do the trick for selectionchange