Meta issue: #1980004: [meta] Creating Dream Markup
Issue based on: #1939102: Convert theme_indentation() to Twig

The motivation is to get rid of empty div's being used for indenting (which clearly is not good markup). Instead, the space should be applied to the table elements, originally proposed in the tr tags but after a semantical-technical discussion at Barcelona, the target was shifted to the first td of the row, containing the title / label content.

However, implementation of the data-indent attribute in the td of only draggable tables, it will be necessary to adjust the template of the whole table. Hence in this issue, there is a patch provided to reduce the redundant markup to only one div (a better-than-nothing approach), and a proposal to consider a new issue towards creating the "table-draggable" template.

Also, the patch removes theme_indentation() (which only inserted markup via PHP) in favour of equivalent Twig template.

Target markup proposal:



1
2
3
4
5
6
7
8
9
td[data-indent="1"] { padding-left: 1em; } td[data-indent="2"] { padding-left: 2em; } td[data-indent="3"] { padding-left: 3em; } td[data-indent="4"] { padding-left: 4em; } td[data-indent="5"] { padding-left: 5em; } td[data-indent="6"] { padding-left: 6em; } td[data-indent="7"] { padding-left: 7em; } td[data-indent="8"] { padding-left: 8em; } td[data-indent="9"] { padding-left: 9em; }

Comments

oresh’s picture

Project: » Drupal core
Version: » 8.x-dev
Component: Twig templates conversion (front-end branch) » markup
Category: feature » task

moving issue to core.

ry5n’s picture

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.

LewisNyman’s picture

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.
LewisNyman’s picture

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?

ry5n’s picture

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).

joelpittet’s picture

Title: Markup for: theme_indentation() » Replace theme_indentation() to data attributes
Issue summary: View changes

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

ry5n’s picture

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.

joelpittet’s picture

Issue tags: +CSS, +JavaScript

@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?

joelpittet’s picture

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.

ry5n’s picture

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.

LewisNyman’s picture

Issue tags: +frontend
mortendk’s picture

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

Cottser’s picture

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 :)

davidhernandez’s picture

Cottser’s picture

Still relevant I think, just related.

Adrian Richardson’s picture

Started working on this issue in mentored sprints, DrupalCon Barcelona

holist’s picture

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
Adrian Richardson’s picture

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

dark-o’s picture

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

kiwimind’s picture

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.

holist’s picture

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.

kiwimind’s picture

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

JS to follow.

kiwimind’s picture

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

rachel_norfolk’s picture

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!!

holist’s picture

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

holist’s picture

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.

Adrian Richardson’s picture

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

kiwimind’s picture

To-do:

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

akalata’s picture

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

Adrian Richardson’s picture

Status: Active » Needs review
FileSize
19.31 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,699 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 30: 1982208-replace-theme-indentation-30.patch, failed testing.

The last submitted patch, 30: 1982208-replace-theme-indentation-30.patch, failed testing.

nod_’s picture

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?

Adrian Richardson’s picture

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.

holist’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

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

Adrian Richardson’s picture

Status: Needs work » Needs review
FileSize
27.8 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,045 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 36: 1982208-replace-theme-indentation-36.patch, failed testing.

nod_’s picture

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.

nod_’s picture

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.

Cottser’s picture

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.

Cottser’s picture

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

rachel_norfolk’s picture

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!)

Cottser’s picture

@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.

davidhernandez’s picture

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

http://yesct.net/

Cottser’s picture

Title: Replace theme_indentation() to data attributes » Replace indentation theme hook/indentation.html.twig with data attributes

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.