Replace indentation theme hook/indentation.html.twig with data attributes

Is there a good reason this is done in markup? Like, a really good reason? Otherwise it seems obvious this should be removed in favor of CSS.

Agreed. We could easily add a indent-x or is-indented-x to the relevant element instead of inserting multiple divs. I guess this was done so we could multiply indentation without writing multiple lines of CSS. I'd much prefer control in CSS. Let's write this function out of core and do a happy dance.

http://api.drupal.org/api/drupal/includes!theme.inc/function/theme_inden...

4 theme calls to theme_indentation()

field_ui_table_pre_render in modules/field_ui/field_ui.admin.inc

Pre-render callback for field_ui_table elements.

theme_book_admin_table in modules/book/book.admin.inc

Returns HTML for a book administration form.

theme_menu_overview_form in modules/menu/menu.admin.inc

Returns HTML for the menu overview form into a table.

theme_taxonomy_overview_terms in modules/taxonomy/taxonomy.admin.inc

Returns HTML for a terms overview form as a sortable list of terms.

Here's a quick demo, for the taxononmy admin page only.

Not sure if this feels right. Maybe we can do something a bit better using data attributes and :before pseudo selectors?

Hmmm… the situation here is such that the current solution isn’t bad, even though it’s pretty gross. Having a bunch of .is-indented--n classses is the only way I can think to do it with just CSS, but I agree is not so hot. I kind of like the idea of using data-* and :before, say like this:

<tr class="is-draggable"><td data-indent="\2003"></td>…</tr>

[data-indent]:before {
  content: attr(data-indent);
  font-family: Arial;
}

That just puts one em space character in front of the table cell content, and makes sure it uses a font that has an actual em space in the character set. PHP or JS just needs to change the data-indent property to however many em spaces, so for 3 indent levels, data-indent="\2003\2003\2003".

Honestly though, it’s not any better than what’s in the summary, except possibly for JS performance (JS has to update the indent value during tabledrag).

Kinda repurposing this issue to get data attributes as @ry5n mentioned in #5

Re-reading this right now, this seems like it should just be set as an inline style. JS can trivially convert an indent level integer into an em value for padding. No complicated attributes or byte-heavy stylesheets needed.

@ry5n one plan that seems to be taking hold strongly at the conference and slightly before is to create data-attributes wherever CSS classes were used to help the distinction between a class that is for style and a class that is for function (the latter being converted to a data attribute). This gives a good distinction and allows for themes to remove classes with less fear they will break functionality.

Ideally we could do:

[data-indent] {
  padding-left: calc(attr(data-indent) * 1em);
}

but of course that doesn't work. Though this is a hard limit for the nesting level at 9, so the extra CSS would be trivial to meet this goal.

Thoughts?

Also the idea got a sporadic applause from the audience in MortenDK's talk last Tuesday so I'm assuming it's going in the right direction with the data-attributes. Implementation details may say other wise...

Also I prefer DRY by all means, but settle for limited duplication in this case.

I’m not up to date on community consensus re: those particulars, but yeah if there’s a hard limit on nesting then classes or data-attributes mapping to styles seems sensible. I suspect that the data-attributes thing is about preventing unrelated CSS and JS from depending on the same class (accidentally introducing coupling). If that’s the case a class may be preferable since it’s applying styles; but again I’m not up to speed on consensus.

consensus (no banana) is that prefixing with .js- is an ok solution, but to make it even "harder" to mess up with css we should really use data- instead

Coming back to this now, any thoughts on @ry5n's idea in #7 to just set this inline with JS? Or would we rather keep the theme function :)

Do we not need this, because of this issue? #2449445: Add "indentation" class back to indentation theme hook, use it for styling

Still relevant I think, just related.

Started working on this issue in mentored sprints, DrupalCon Barcelona

theme_indentation() is called from:

core/modules/taxonomy/src/Form/OverviewTerms.php
core/modules/field_ui/src/Form/EntityDisplayFormBase.php
core/modules/menu_ui/src/MenuForm.php
core/modules/book/src/Form/BookAdminEditForm.php

In addition to the theme_indentation() function, we're currently working on core/misc/tabledrag.js, core/modules/system/css/components/tabledrag.module.css

This will need to be tested manually on all places where it takes effect. Places where this can be tested:

Menu items:
admin/structure/menu/manage/main

Edit content type form
admin/structure/types/manage/article/form-display
admin/structure/types/manage/article/display

List taxonomy terms:
admin/structure/taxonomy/manage/tags/overview

Book hierarchy
admin/structure/book/4

Our current thinking is to allow a single div to be added with a data attribute attached to it.

Optimally this div would be removed and the data attribute added to its parent td. This may follow.

A partial patch towards the goal outlined by @kiwimind for creating only single div in the theme_indentation() and Twig template indentation.html.twig.

JS and CSS parts still needed.

Further to the patch provided by @holist, this patch now includes the CSS for this update.

JS to follow.

Amended previous patch to use pixel spacing to allow consistent rendering of the png tree.

You guys have done some epic work on this today - make sure you have a plan how you are going to keep up the momentum till completion before you leave.

THANKYOU!!

Removed theme_indentation() in favour of indentation.html.twig. CSS changes by @kiwimind included.

Suggestion for follow-up:

Once we have reduced the div's to only one and removed theme_indentation() (JS solution still pending), the next step should be to include the data-indent attribute in the td instead of an empty div.

A good solution (as discussed with @rteijeiro) would be to create own template called, say, table-draggable.html.twig, and relevant preprocess for that. This would then be triggered from the form builders etc. depending on whether the table is marked draggable. The current implementation makes it very complicated to add attributes to td tags, because only generic table.html.twig is used.

This approach needs to be evaluated and the implementation outlined by someone with the bigger picture in mind.

Here's the updated tabledrag.js file to drive the indentation using data-indent attribute and only one div.

To-do:

- redo CSS and background images for tree-child classes that display on mousedown

All of the awesome work by sprinters at Barcelona (from comment #16) should be consolidated into an issue summary update!

I've done some work on the background images for the tree hierarchy display when dragging. I've worked around using the CSS background-position edge offsets, since Safari 6.1 and before don't support them. This somewhat complicates the right alignment needed for rtl displays - it has to be handled in tabledrag.js per group row.

Anyway I think this is ready for review now.

I really, really hate to add more code to the tabledrag script. Also the current version has been battle-tested and I don't feel like breaking it a week before RC.

Is it possible to add the divs in the js instead of the theme_function and keep the code as-is?

Unfortunately, tabledrag.js currently needs a div.js-indentation per level to function. So to move to cleaner markup without all the extra divs, we do have to make changes to tabledrag.js.

This patch gets close to the clean markup: theme_indentation and is now removed completely. BookAdminEditForm, EntityDisplayFormBase, MenuForm and OverviewTerms have been changed accordingly.

Still to do: properly test all four of the forms affected - I've been concentrating on MenuForm in development; plus we should only insert div.tree on drag start when needed for displaying the structure of the group being dragged and remove it on drop to keep the markup clean. The test in TaxonomyTermIndentation (and possibly other tests) also needs updating to use the new markup.

I'll be able to look at this further this evening.

I updated the issue summary to reflect our thinking in BCN sprint.

Markup should now match initial proposal. tabledrag.js adds a temporary div to show the tree structure on the child elements being dragged. Taxonomy term indent test also updated for the new markup.

Keyboard can't be used to do everything, and when trying to use the mouse to fix the problem, things go even wronger. I haven't tried touch (for the record this is why i didn't want to change the JS. It's a pain to test and it look like it'll be a pain to fix).

I'd prefer if data-indent was data-drupal-indent to conform to the rest of our attributes and avoid naming collisions. data-indent sounds like it could be used by some scripts out there.

1. +++ b/core/misc/tabledrag.js
   @@ -195,22 +195,47 @@
   +      var testRow1 = $('<tr/>').addClass('draggable').attr('data-indent', 1).appendTo(table);
   +      var testCell1 = $('<td/>').appendTo(testRow1);
   +      var testRow2 = $('<tr/>').addClass('draggable').attr('data-indent', 2).appendTo(table);
   +      var testCell2 = $('<td/>').appendTo(testRow2);
   

   Since you're renaming vars, might as well follow naming standards. If the variable hold a jquery object, it's name need to start with $.

2. +++ b/core/misc/tabledrag.js
   @@ -195,22 +195,47 @@
   +      if (this.rtl == 1) {
   +        this.indentAmount = Math.abs(parseInt(testCell2.css('padding-left')) - parseInt(testCell1.css('padding-left')));
   +      }
   +      else {
   +        this.indentAmount = -Math.abs(parseInt(testCell2.css('padding-right')) - parseInt(testCell1.css('padding-right')));
   +      }
   

   var rtl = this.rtl === 1 ? 1 : -1;
   var paddingSide = rtl ? 'left' : 'right';
   this.indentAmount = rtl * Math.abs…;

   or something like that. we can avoid duplication the whole Math.abs(parseInt…

3. +++ b/core/misc/tabledrag.js
   @@ -195,22 +195,47 @@
   +       *
   +       * @prop {string} y
   +       * @prop {number} dx
   +       * @prop {number} ox
   +       * @prop {bool} rtl
   

   Need descriptions. No idea what ox is. y or dx is fairly clear but might want to be verbose for ox. It's not an existing math convention. (and everything needs description anyway).

4. +++ b/core/misc/tabledrag.js
   @@ -195,22 +195,47 @@
   +          rtl: (this.rtl == 1) ? 0 : true
   

   Seems fishy. is there a reason why we don't have true/false or 1/0 ? we shouldn't be mixing the two.

5. +++ b/core/misc/tabledrag.js
   @@ -1210,37 +1231,33 @@
   +            currentRow.find('td:first-of-type').addClass('tree-child-first').prepend($(menuDisplay));
   ...
   +            currentRow.find('td:first-of-type').addClass('tree-child').prepend($(menuDisplay));
   

   don't need to jquery it. just .prepend(menuDisplay);

6. +++ b/core/misc/tabledrag.js
   @@ -1368,22 +1385,26 @@
   +        el = $(el);
   

   Naming. Should be $el.

7. +++ b/core/misc/tabledrag.js
   @@ -1368,22 +1385,26 @@
   +        rowIndent = el.data('indent');
   +        rowIndent += indentDiff;
   

   Don't see why it's on 2 separate lines.

8. +++ b/core/misc/tabledrag.js
   @@ -1368,22 +1385,26 @@
   +        el.data('indent', rowIndent);
   +        // We need to reflect this in the DOM as well, so the CSS reflects the change.
   +        el.attr('data-indent', rowIndent);
   

   I get why we need to do that but it's a little bit of code smell.

9. +++ b/core/misc/tabledrag.js
   @@ -1467,6 +1488,35 @@
   +    var indentDivs = $(this.table).find('.tree-child-first .tree,.tree-child .tree,.tree-child-last .tree');
   ...
   +      var row = $(this).closest('tr');
   

   Naming.

10. +++ b/core/misc/tabledrag.js
    @@ -1467,6 +1488,35 @@
    +      if (indentPixels.rtl) {
    +        $(this).css({
    +          'margin-right': '-' + backgroundX + 'px',
    +          'width': backgroundX + 'px',
    +          'height': row.height()
    +        });
    +      }
    +      else {
    +        $(this).css({
    +          'margin-left': '-' + backgroundX + 'px',
    +          'width': backgroundX + 'px',
    +          'height': row.height()
    +        });
    +      }
    

    it doesn't need to be that verbose.

    var styles = {width: …, height: …};
    var margin = '-' + backgroundX + 'px';
    if (indentPixels.rtl) {
    styles['margin-right'] =  margin;
    }
    else {
    styles['magin-left'] =margin;
    }
    $(this).css(styles);

    why is it called backgroundX? nowhere a background is involved, just margin, width and all.

    indentPixel is not a good name. It really doesn't help, I forgot what it was and expected a number, not an object. indentInfos or something would be more accurate.

There are eslint errors, needs to be fixed.

umm I guess the keyboard has the same capabilities before and after. It's just that using the mouse on something we moved with the keyboard result in funky stuff where is was working as expected before.

Perhaps it's worth re-profiling to see where we stand if we just removed the theme function at this point. Really the numbers at #1939102-20: Convert theme_indentation() to Twig don't look too terrible. Then we can maybe evaluate further refactoring at a later time.

Going to profile and see what we're looking at.

I would really appreciate if you can describe how you go about that, Cottser. A few people were involved in this issue as first entry to contribution at Drupalcon Barcelona so everything is a good learning experience.

(Okay, I want to know, too!)

@rachel_norfolk there are some slightly outdated but mostly accurate docs at https://www.drupal.org/contributor-tasks/profiling and I'm willing to help in IRC but probably not over the next 7 days or so.

See #2578567: Remove theme_indentation() and use Twig template only for an issue to rip out theme_indentation() because the performance is reasonable. I think this is still worth pursuing (and thank you everyone who jumped in!) but I do tend to agree with @nod_ that it's risky this close to release to refactor the JS. If we can get the Stable base theme into core we can still pursue this after 8.0.0.

YesCT also blogged herself going through the process of setting up profiling recently. It might be more updated and specific.

http://yesct.net/