Remove classes from system templates r*.html.twig

Postponing this until we decide how we are moving forward. Keep an eye on #2348543: [meta] Consensus Banana Phase 2, transition templates to the starterkit theme for updates. Thanks.

Un-postponing. The templates have been copied to Classy, so all we need to do is remove classes from the original templates.

lets try this again

The only CSS I see attached to form-radios is margin. I don't think I see anything using 'region' directly but I do see some JS using region-*, do we need to change those? For example, quickedit looks for region-content.

@david sounds like we should do a cleanup on quickedit as a seperate issue, or eventually scopecreep this ?

Well, I don't think we can commit anything here without accounting for it, otherwise we introduce a regression.

I would suggest that we then do a css standards on quicklinks cleanup & then at the same time there fixes any calls to region-content.
- yes more followups i know, but :/

Diggin into this:

    // In the case of a full entity view page, the entity title is rendered
    // outside of "the entity DOM node": it's rendered as the page title. So in
    // this case, we must find the entity in the mandatory "content" region.
    if (entityElement.length === 0) {
      entityElement = $('.region-content').find(entityElementSelector);
    }

Could somebody point to the place that says that drupal have to have a "content" region - which is ok to assume.
But if that region is there then we assume that offcourse we have a wrapper around it thats called .region-contentt around the region
It seems like were requiring themes to have this, if thats the case - Then it should be documented somewhere & the classnames must indicate this as well in this case with a js- prefix - If this is not the case we should clean this up.

Could somebody point to the place that says that drupal have to have a "content" region - which is ok to assume.

https://www.drupal.org/node/171224

In Drupal 7, $content became a full region and is now mandatory in all themes. This new requirement was set up so that when enabling new themes, Drupal knows where to put the main page content by default.

I knew this was true for 7, but have never checked to see if it was a hard requirement for 8.

ok cleaned it up a bit and removed the .region-content selector, as we cant rely on that whatsoever to be a part of a theme (usually one of the first places anybody goes to cleanup - when the div soup is to thick) I changed the selected from region-content to be main instead.

We should use <main> instead of .region-content - Based on the following reasons:
* using css for this is terrible - ill keep the rant down! but .... *grrr*
* Main should always be present on a page, its like having a head / footer so it makes perfect sense.
* Main is an unique tag that there should be only once on a page (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/main) - hurray without using an ID

This should be documented somewhere in the big book of "what goes on in drupals frontend", but maybe in the page.html.twig as well ?

+++ b/core/modules/quickedit/js/quickedit.js
@@ -272,7 +272,7 @@
     // outside of "the entity DOM node": it's rendered as the page title. So in
     // this case, we must find the entity in the mandatory "content" region.
     if (entityElement.length === 0) {
-      entityElement = $('.region-content').find(entityElementSelector);
+      entityElement = $('main').find(entityElementSelector);

Should change that comment about using "content" region, since it is being changed to <main>.

Also, this needs some testing to make sure it is still working right, whatever it does. This entityElement, it is being moved from the region-content div to the main tag which is two levels up in the markup.

Selector changed to be main[role="main"] which is the default wrapper for "content" and the full view of a node
In that way we dont "sneak in" a dependency on .region-content that nobody have an idea about and <main role="main"> is always present.

Fixed a typo in a comment.

Attached a few screenshots:

Radios BEFORE

Radios AFTER

QuickEdit BEFORE

QuickEdit AFTER

CR updated as well

+++ b/core/modules/quickedit/js/quickedit.js
@@ -270,9 +270,11 @@
+      // entityElement = $('main').find(entityElementSelector);

Looks like this can be removed.

new patch

+++ b/core/modules/quickedit/js/quickedit.js
@@ -270,9 +270,10 @@
+       entityElement = $('main[role="main"]').find(entityElementSelector);

Wrong indentation.

Just fixing that indent.

For this change, I'd be real careful about checking all the region stuff. That's what worries me in possibly introducing a regression. Make sure to test with Stark.

@david

+++ b/core/modules/quickedit/js/quickedit.js
@@ -270,9 +270,10 @@
-      entityElement = $('.region-content').find(entityElementSelector);
+      entityElement = $('main[role="main"]').find(entityElementSelector);

We change the selector from using the class .region-content thats only in the region thats named "content" & move that up to <main role=" main"> thats unique.

im kinda thinking that we maybe should add a data-attibute, or adding in a class lik "js-quickedit-selector-foo" to make it more robust + needs comments in the source so its clear that we use this as a selector for quickedit

But yes needs test anyways

I'm no JS wiz, but this seems to maintain the quickedit functionality.

we should really use a js-quickedit class here else nobody have an idea of whats going on

@david think one of the win we could get with removing the quickedit dependency from the region template, we could make it more obvious where it comes from and whats happening in the code. In that light i would suggest that we put it out in the main region and used .js-quickselect to make it clear for the themer whats going on + we dont fill all the region divs with data-- that will probably scare a lot of people.

a reroll + manual testing of all existing themes.

+++ b/core/modules/quickedit/js/quickedit.js
@@ -369,9 +369,11 @@
+      entityElement = $('.js-quickedit').find(entityElementSelector);      ¶

I think this is a good idea. Can we change this class to something more meaningful? This isn't a trigger for all of quickedit, this is a specific selector for the main content container, how about .js-quickedit-main-content? Also there is whitespace at the end of this line.

@lewis good point ive updated the patch

I manually tested this and I'm happy with the changes here. I wave the RTBC wand.

Markup is not frozen yet. Committed 22cd7b8 and pushed to 8.0.x. Thanks!

Wow, it's very strange to have a reference to quickedit in System module's page template.

I can see 2 ways to fix this (IMO) "properly".

1. Decide that finding the main-content is useful for all kinds of JS and call this "js-main-content"
2. Have Quickedit module add that in preprocess (this would require the addition of a variable for that I guess)

2. Have Quickedit module add that in preprocess (this would require the addition of a variable for that I guess)

I guess this makes the most sense. This is why we allow modules to inject classes.

I think we'd need an attribute object for that. 'page' doesn't currently have one for any of the other elements. I wonder if that is just a coincidence or there is a reason.

its even stranger to have the selector hidden inside a regions classes with no references to it ;)

Seriously lets not have preprocess add in all kinds of stuff, just so we can hide it, its been an epic pita for years, that so much have been hidden.
yes we allow module to add in classes, but really they should not do that, its a fallback for when its not possible otherwise, its keeping us in the "preprocess for everything" thinking of drupal7 - lets get it out in the open so we can see & look at it, yes this is TX to the max.

Adding in {{ attributes }} ... hmmm yes but we have em all over and nobody knows what they are used for, they are just there, so unless theres a really good reason for that, lets not confuse ourself with this just to create anther box where we maybe can dump stuff into.

+++ b/core/modules/system/templates/page.html.twig
@@ -96,7 +96,7 @@
-  <main role="main">
+  <main role="main" class="js-quickedit-main-content">

I just discovered this by accident, I was also very surprised by this.

+++ b/core/modules/quickedit/js/quickedit.js
@@ -369,9 +369,10 @@
+    // this case, we find the main element of the page and use it as a full
+    // entity ancestor.

What does "full entity ancestor" even mean? I wish you'd have pinged a Quick Edit maintainer for review.

yes we allow module to add in classes, but really they should not do that

Huh? By that reasoning, certain modules would never be able to function. They need to inject classes/markup in order to provide certain functionality.

The problem is that we can rely neither on:

• a selector for the "main content" region (that dependency was removed in this patch)
• <main> being present (not every theme may have this)
• [role=main] being present (not every theme may have this)
• .page-title being present (some themes may just use <h1>)

Quick Edit just needs to know the DOM node in which the other fields of the entity besides the title live. Perhaps there's another way possible now, compared to 3 years ago?

Huh? By that reasoning, certain modules would never be able to function. They need to inject classes/markup in order to provide certain functionality.

Yes certain modules may need to do this, but we should work hard to have all classes inside of the theme instead of hidden a few away in a module.
It makes it hard to find for a themer & makes it a pita to track where stuff comes from.
It should always be the exception to add a class from a module.

main ftw
actually we can rely on themes having the main tag, the same way as we can rely on them having a body or footer.
<main> should always be present in a page - not having a role="main" would also go against the accesability
https://developer.mozilla.org/en/docs/Web/HTML/Element/main

.page-title yes please let us remove that, its one of these.

actually we can rely on themes having the main tag, the same way as we can rely on them having a body or footer.

Perfect! In that case, Quick Edit can just rely on that! Much simpler. None of the ugliness introduced here is necessary anymore then.

Patch at: #2568099: Follow-up for #2407739: Remove the js-quickedit-main-content class that was added in favor of relying on <main>. Let's clean this up :)