backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Why are we adding the dummy permission checkboxes via JS instead of PHP? #6272

Open klonos opened 11 months ago

klonos commented 11 months ago

This is not an issue to question whether these dummy checkboxes should be added or not (that's what #5907 is for - see that issue for some further background/context). This issue here is meant to:

So in that file there's these comments currently:

  1. On a site with many roles and permissions, this behavior initially has to perform thousands of DOM manipulations to inject checkboxes and hide them. By detaching the table from the DOM, all operations can be performed without triggering internal layout and re-rendering processes in the browser.

  2. Create dummy checkboxes. We use dummy checkboxes instead of reusing the existing checkboxes here because new checkboxes don't alter the submitted form. If we'd automatically check existing checkboxes, the permission table would be polluted with redundant entries. This is deliberate, but desirable when we automatically check them.

There are already some elaborate problems and workarounds in the admin/config/people/permissions page, and when I am reading these comments I am left with so many questions.

I would like to start with this part of the JS code:

      var $dummy = $('<input type="checkbox" class="form-checkbox dummy-checkbox" disabled="disabled" checked="checked" />')
        .attr('title', Backdrop.t("This permission is inherited from the authenticated user role."))
        .hide();

      $('input[type=checkbox]', this).not('.role-authenticated, .role-anonymous').addClass('real-checkbox').each(function () {
        $dummy.clone().insertAfter(this);
      });

The above basically:

And I wonder: why are we not adding the HTML of those dummy checkboxes in the theme_user_admin_permissions() function in core/modules/user/user.theme.inc instead? Since we know which checkboxes will need a dummy checkbox clone, and since these do not change dynamically on the page, then why not build that page in PHP with those checkboxes already there, and then let JS (or #states) handle only the toggling?

klonos commented 11 months ago

I've filed a PR here: https://github.com/backdrop/backdrop/pull/4554

Not saying that we should do this (that's why I'm not adding the various pr - needs testing/pr - needs code review labels to the issue) - just as a PoC that this can be done via PHP, standard FAPI elements + #states, while at the same time relying as less as possible on custom JS. I personally like that from a consistency point of view, but now we can test and compare various aspects, like accessibility, performance etc as well.

klonos commented 11 months ago

...and here's the documentation I have so far (needs review/wordsmithing):

 * When the checkbox for a specific permission for the "Authenticated" role is
 * checked, the rest of the non-anonymous user roles inherit the same
 * permission. We are indicating that visually by checking and "locking" the
 * checkboxes for the same permission for the other roles in the same row on
 * the permissions table (with the exception of the respective checkbox for the
 * "Anonymous" role).
 *
 * When a permission is inherited that way, instead of checking/unchecking the
 * real checkboxes of the same permission for other user roles, we are visually
 * toggling between real and "dummy" checkboxes. The dummy checkboxes:
 * - always have their 'checked' attribute set.
 * - are always locked via the 'disabled' attribute, so that they cannot be
 *   manually checked/unchecked.
 * - do not have a 'name' attribute set, so their values are not being submitted
 *   with the form.
 * If we'd automatically check the actual/real checkboxes, the respective
 * permissions would be saved in the configuration files of the user roles,
 * which is not desired (inheritance of a permission is done by checking whether
 * that permission has been granted to the "Authenticated" role - not by
 * checking whether the permission has been explicitly granted).
 *
 * Because we are not altering the value of the actual checkboxes during form
 * submission (we are simply visually showing/hiding them on the table), we are
 * not "polluting" configuration files of user roles with any permissions that
 * have not been explicitly granted to them. That way, permissions are only
 * retained in configuration if they have been previously explicitly granted (as
 * opposed to being inherited by the "Authenticated" role).

I have currently placed it in a the docblock in user.permissions.js, but we might want to move it to theme_user_admin_permissions() so that it is shown in our API site (for people that are not looking directly at the code, and so that it is indexed by search engines).