GetmeUK / ContentTools

A JS library for building WYSIWYG editors for HTML content.
http://getcontenttools.com
MIT License
3.94k stars 393 forks source link

Unfortunate keypresses propagate from dialogs and page-level elements to the toolbox #485

Open cubiclesoft opened 6 years ago

cubiclesoft commented 6 years ago

The scripts/ui/toolbox.coffee file has the following comment when adding event listeners:

# Keyboard events that apply only to non-text elements

And goes on to process enter and delete keydown events for such elements. It also looks like undo and redo key combinations are also handled here for the entire UI. The event handler is on the window object, which has some unfortunate side effects. (On a side note, I don't see nor know of any way around using a global window event handler for handling those particular events as it's a known weakness of handling content editable enabled areas in ways that users are familiar with/expect.)

The first side effect can be seen if an image is selected and then the user opens a modal dialog like the image/video dialog. While the dialog is open, the user presses the Delete key. The image will be removed and focus will no longer be on the dialog. This is happening because keydown events are propagating past the dialog to the window. I somewhat stopped the behavior by adding the following to the _addDOMEventListeners method in scripts/ui/dialogs/dialogs.coffee:

        # Stop propagation of keydown events to the toolbox EXCEPT for the escape key.
        @_domElement.addEventListener 'keydown', (ev) =>
            if ev.keyCode != 27
                ev.stopPropagation()

That lets the escape key to continue to be handled by the document listener but suppresses passing additional keydown events up the hierarchy (even those not handled by toolbox).

Unfortunately, that's not enough to stop the behavior altogether. To finally fix it, I altered scripts/editor.coffee:

    constructor: () ->
...
        # The number of modals being displayed.
        @_modals = 0

...
    toolbox: () ->
        # Return the toolbox component for the editor
        return @_toolbox

    modals: () ->
        # Return the number of modals being displayed.
        return @_modals
...
    addedModal: () ->
        # Increments the number of modals being displayed.
        @_modals++

    removedModal: () ->
        # Decrements the number of modals being displayed.
        @_modals--

    createPlaceholderElement: (region) ->

Then I modified scripts/ui/modal.coffee:

        # Update the editor to let it know that a new modal is mounted.
        app = ContentTools.EditorApp.get()
        app.addedModal()

        # Add interaction handlers
        @_addDOMEventListeners()

    unmount: () ->
        # Unmount the widget from the DOM

        # Update the editor to let it know that the modal is unmounted.
        app = ContentTools.EditorApp.get()
        app.removedModal()

And finally, I modified scripts/ui/toolbox.coffee:

        @_handleKeyDown = (ev) =>

            # Ignore events if a modal is showing.
            app = ContentTools.EditorApp.get()
            if app.modals()
                return

The other side effect can be seen if there are form elements on the page (e.g. an input element) and the form element has focus. Undo/redo from within the form element is applied but the toolbox handler is also called. I stopped the behavior by modifying the toolbox some more:

        @_handleKeyDown = (ev) =>

            # Ignore events if a modal is showing.
            app = ContentTools.EditorApp.get()
            if app.modals()
                return

            # Prevent keydown events from being handled for form elements.
            target = ev.target || ev.srcElement || ev.originalTarget
            nodename = target.nodeName.toLowerCase();
            if nodename is 'input' or nodename is 'textarea' or nodename is 'select' or nodename is 'button'
                return;

            # Keyboard events that apply only to non-text elements

Between the various modifications, I figure that should deal with the most common situations. However, extremely custom widgets, canvas elements, and the like might still be problematic.

anthonyjb commented 6 years ago

Hi @cubiclesoft thanks for the excellent description and information. I will apply a similar approach to resolving the issue on CT master branch shortly.

I'm currently gearing up to start a complete rebuild of ContentTools, as well as moving to es6 (you may well have noticed I have a bunch of new JS libraries up now as part of a project called manhattan) I have a host of changes planned including implementing a much more capable UI (so excellent detailed descriptions like this are is very useful). The current UI was never meant really to be used beyond what was included in CT but over the last couple of years people have used (and struggled to use) the current UI code to extend CT. This includes myself when trying to use the limited UI library to build the ContentFlow UI.