Why is it wrong to modify the DOM from a controller?

Question

It is common knowledge for any Angular user with even a bit of experience that directly modifying the DOM from a controller is bad practice. One reason is that it results in DOM manipulation code which is localized and cannot be reused. From this excellent Toptal Angular tutorial, there are two more reasons not to manipulate the DOM from the controller:

• it violates the purpose of the controller, and
• the transcluded child content has not been added to the DOM

The first reason is obvious, since the controller was intended to be simply that, namely an entity which brokers information exchange between the view and the model. However, I don't understand what is meant by the second point.

This question was actually driven by a real UI problem I am currently facing. A developer in my team (who shall remain nameless) actually added code to one of our controllers which directly manipulates the DOM. Here is a small screen capture of the area of interest:

Clicking the pencil would turn my name into an editable text box. However, on some browsers we see a momentary flickering, where all the above DOM elements seem to jump all over the place. It appears that something very kinky is happening, and I would like to understand what it is.

Here is a boiled down version of the HTML from the above form:

<input class="usrProfileTextBox" id="displayNameTextBox"
       ng-show="editingDisplayName" type="text" ng-model="displayName"
       ng-keyup="$event.keyCode == 13 ? changeDisplayName() : null"
       ng-blur="changeDisplayName()" />
<div class="usrProfileRightCol">
    <label class="usrProfileNameLabel" ng-hide="editingDisplayName">{{displayName}}
    </label>
    <span ng-hide="editingDisplayName" ng-click="showEditDisplayName()"
          class=" pointer-click glyphicon glyphicon-pencil"></span>
</div>

And, for completeness, here is the controller function which fires when a user clicks the edit button for the username box:

$scope.showEditDisplayName=function(){
    $scope.previousDisplayName = $scope.displayName;
    $scope.editingDisplayName = true;
    $timeout(function() {
        document.getElementById("displayNameTextBox").value=$scope.displayName;
        document.getElementById("displayNameTextBox").focus();
    });
}

Can anyone shed light on what is actually happening under the hood when our controller directly manipulates the DOM?

Answer 1

The first issue is the separation of concerns. The roles are distributed according to directive anatomy. A controller defines things on the scope (which is this for controllerAs syntax), link function manipulates DOM and ties things together with required controllers.

The second issue is directive compilation precedence. Children DOM elements may be not available in controller constructor, as well as bindings. Role distribution suggests that these things work best in compile, pre-link and post-link functions.

This is the established community practice for Angular 1.4.x and lower. Things have changed a lot in Angular 1.5 due to introduction of components and the process of convergence between Angular 1 and Angular 2.

Due to the fact that there are no link functions in components, they can be implemented with lifecycle hooks in controller. All code from link functions that doesn't use required controllers goes to hooks - from pre-link to $onInit hook and from post-link to $postLink hook, respectively.

There are no precedence problems in the snippet listed in the question, because DOM modifications run on click and not on initialization. The problem is that ng-controller directive is used instead of a hierarchy of custom directives/components. In well-designed app there may be no ng-controller directives at all.

document.getElementById has code smell because it hurts testability. This code may require no DOM manipulations at all, because the same thing can likely be achieved with data binding and ng-focus directive.

It isn't an end in itself to not let any DOM manipulations in controller/scope methods (if DOM manipulation should be done on ng-click, it should obviously be done in some method). They just can be avoided when the things are done in a way that is idiomatic to Angular.

Answer 2

The view will be recompiled once a $scope param in the depending view has been changed. In your case it's editingDisplayName. We were facing the same problem and we could make that work by:

1. Handling the display state of both items in separat $scope variables.
2. Working with minimal timeouts for delaying.

The origin of your problem is the recompile with editingDisplayName. It's not set in one render step for both elements at the same time. That results in small flickiring due to asynchronous proceed of JavaScript for ng-hide. This could also happen in a directive or native JavaScript. It's not depending on ng-controller.

Checkout the following attemp. It handles the showStateof your elements in two $scopes. Thats how we fixed that flickering. (you may need to imrprove this example due to your blur functionality)

View

<input class="usrProfileTextBox"
       id="displayNameTextBox"
       ng-show="showInputBox"
       type="text"
       ng-model="displayName"
       ng-keyup="$event.keyCode == 13 ? changeDisplayName() : null"
       ng-blur="changeDisplayName()" />

<div class="usrProfileRightCol"
     ng-show="showTextBox">
    <label class="usrProfileNameLabel">
           {{displayName}}
    </label>
    <span ng-click="showEditDisplayName()"
          class="pointer-click glyphicon glyphicon-pencil">
    </span>
</div>

Controller

$scope.showEditDisplayName = function(){

    $scope.previousDisplayName = $scope.displayName;
    $scope.showTextBox = false;

    $timeout(function () {
        document.getElementById("displayNameTextBox").value=$scope.displayName;
        document.getElementById("displayNameTextBox").focus();
        $scope.showInputBox = true;
    }, 100);
};

You maybe going to replace the native JavaScript set value and focus with an AngularJS directive. But thats just a refactoring hint.

Tim's Edit:

The jist of this answer basically fixed the problem, but the function which was actually causing the flickering was changeDisplayName(), which gets called when the user blurs the input box, which should hide that box and display the updated username label. Here is the code which I ended up using in production. Note carefully the use of the timer which separates the hiding of the input box (first) followed by the showing of the username label (second):

$scope.changeDisplayName=function(){
    if ($scope.showInputBox == false) {
        return; // prevent function from being called twice from both keypress
    }           // and blur events

    // hide the input box, pause, then show the username label
    $scope.showInputBox = false;
    $timeout(function() {
        $scope.showDisplayLabel = true;
    }, 100);
}