Critical issue with rendering property editors caused by disableTabIndex directive

mayhammf commented 1 year ago

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

10.3.2

Bug summary

When a property editor is in preview mode (either in the content type editor view or when it is locked due culture variation setup), the directive disableTabindex observes all DOM changes to set the tabindex of all tabbable elements to -1. The disableTabindex is preforms an extremely inefficient process which results in the entire backoffice freezing.

Specifics

Due to the complexity of this issue, i have provided a sample project that you can use in order to see the issue firsthand and a recording (see below).

Here is a short explanation first: When a property editor is in preview mode, the directive disableTabindex prevents tabbing into it by setting the tabindex to -1. It does that by listening to all DOM changes and whenever there is a changes, it locates all tabbable elements and sets the tabindex attribute to -1 This issue will effect any property editor that has a reasonably large list of inputs, even build-in properties (for example "Repeatable textstrings" which you can test in the attached project by going to the "Build-in prop editor test" and observing the performance)

Please watch the recoding below and then try to run the attached project (login info in readme.txt in the root)

https://user-images.githubusercontent.com/5937658/206419443-4038ee99-74be-4482-ac5b-e6d4283998f5.mp4

If video isn't playing correctly, download from here: Recording of disabletabindex issue.zip

Sample project here: Umbraco-DisableTabIndex-Issue.zip

Steps to reproduce

Run the attached project.
in the Content section
Click on the "Show list" button in the first property and observe the performance
Click on the "Show list" button in the second property and observe the performance
Now, go to the file App_Plugins/CountryPicker/countryPicker.js and
uncomment out lines 50 => 119 (this will override the built-in directive with a copy of it that we can control)
the uncomment line 72 (thus prevent the disableTabIndex from doing its thing)
Repeat the steps above (make sure to clear browser cache)
Observe that the performance issue is gone.
(you can do the same test for a built-in editor using the page "With built-in properties"

Expected result / actual result

Expected result:

The two properties should render instantly without any issue.

Actual result:

The second property where disableTabIndex is involved, the browser chokes for a considerable amount of time

nikolajlauridsen commented 1 year ago

Hey @mayhammf thanks for taking the time to write a really detailed issue description and even providing an example project, love to see it 😄

I can indeed reproduce, and this does seem like a bit of a nasty one, so I'll take it up with the team to see if we can't get it in a future sprint.

mayhammf commented 1 year ago

Hi @nikolajlauridsen A very quick fix on this one could be to simply keep everything that the disableTabindex directive is doing unchanged, but throttle the frequency of it. Currently, the MutationObserver will invoke domChange every single time angular makes a change to the DOM. We can, as a temporary solution, throttle the execution of domChange by debouncing it. Something like this: (see from // Fix Starts here the rest is copied from the file in 10.3.2

angular.module("umbraco.directives")
    .directive('disableTabindex', function (tabbableService) {

        return {
            restrict: 'A', //Can only be used as an attribute,
            scope: {
                "disableTabindex": "<"
            },
            link: function (scope, element, attrs) {

                if (scope.disableTabindex) {
                    //Select the node that will be observed for mutations (native DOM element not jQLite version)
                    var targetNode = element[0];

                    //Watch for DOM changes - so when the property editor subview loads in
                    //We can be notified its updated the child elements inside the DIV we are watching
                    var observer = new MutationObserver(domChangeHandler);

                    // Options for the observer (which mutations to observe)
                    var config = { attributes: true, childList: true, subtree: true };

                    function domChange(mutationsList, observer) {
                        for (var mutation of mutationsList) {

                            //DOM items have been added or removed
                            if (mutation.type == 'childList') {

                                //Check if any child items in mutation.target contain an input
                                var childInputs = tabbableService.tabbable(mutation.target);

                                //For each item in childInputs - override or set HTML attribute tabindex="-1"
                                Utilities.forEach(childInputs, element => {
                                    $(element).attr('tabindex', '-1');
                                });
                            }
                        }
                    }

                    // Fix starts here
                    var domChangeDeboucne;
                    function domChangeHandler(mutationsList, observer) {
                        if (domChangeDeboucne) {
                            clearTimeout(domChangeDeboucne)
                        }

                        domChangeDeboucne = setTimeout(function () {
                            domChange(mutationsList)
                        }, 1e3); // Deboucne by 1 second
                    }
                    // Fix ends here
                    // Start observing the target node for configured mutations
                    //GO GO GO
                    observer.observe(targetNode, config);
                }

            }
        };
    });

enkelmedia commented 1 year ago

This issue caused me a lot of headaches today, thanks @mayhammf for posting the issue here.

I'm on Umbraco 11.2.1 and I really had to get around this so I had to come up with a hack to disable the directive.

And this is what I call a hack, it will basically remove the directive until a fix is submitted.

//@ts-ignore
var invokes = angular.module("umbraco.directives")._invokeQueue;

let removeDirectiveIndex = -1;
for (let i = 0; i < invokes.length; i++) {
    if (invokes[i][1] === "directive")
    {
        if(invokes[i][2][0] == 'disableTabindex'){
            removeDirectiveIndex = i;
        }
    }
}

if(removeDirectiveIndex > -1){
    //@ts-ignore
    angular.module("umbraco.directives")._invokeQueue.splice(removeDirectiveIndex,1);
}

@nikolajlauridsen are there any plans to address this? I would be open to working with @mayhammf to create a PR?

nikolajlauridsen commented 1 year ago

I remember us talking about this one and agreeing that it should be done, however, it's not in the backlog at the moment, so right now our focus is elsewhere.

It does however seem like you have a good deal of insight between you and @mayhammf, so we'd love to get this in as a community PR, so I'll go ahead and mark this as up for grabs.

github-actions[bot] commented 1 year ago

Hi @mayhammf,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post.

Thanks muchly, from your friendly Umbraco GitHub bot :-)

github-actions[bot] commented 1 year ago

Hi @mayhammf,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post.

Thanks muchly, from your friendly Umbraco GitHub bot :-)

umbraco / Umbraco-CMS