Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
#2407739: Remove classes from system templates r*.html.twig added a js-quickedit-main-content
class. But this was a bad idea. See #2407739-40: Remove classes from system templates r*.html.twig and later.
Proposed resolution
Remove it, by depending on <main>
instead, which is required anyway per #2407739-45: Remove classes from system templates r*.html.twig.
Remaining tasks
Review.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#20 | 2476947-20.patch | 5.32 KB | Wim Leers |
Comments
Comment #2
Wim LeersComment #3
davidhernandezAny time we touch something related to Quick Edit weird things pop up, so ...
Comment #4
RainbowArrayHmm.
There's really nothing stopping a theme from changing that wrapper from a main to a div. I might want to put main inside the node template, for example, because I don't want comments as part of main.
Or I might be using panels, so I'd want the main on one of the panels, not in the page template around the entire panels layout.
To me it feels like a dedicated class is more flexible than requiring main to only be used in the page template.
Comment #5
Wim LeersBump.
It'd be very sad to have
js-quickedit-main-content
being a hardcoded class in all ourpage.html.twig
templates.#4: see #2407739-45: Remove classes from system templates r*.html.twig, which claims
<main>
is required anyway.Comment #6
RainbowArrayI'd like to think there will always be a main somewhere, but no, there is no requirement for that. There will be no error message if somebody removes main. There's nothing to stop somebody from doing so.
Honestly, this class should be passed in via attributes from the module. There's nothing stopping somebody from removing an attributes variable, either, of course, but I think that's a more reliable solution. I agree that this quickedit class shouldn't be hardcoded into the template. In theory somebody could turn off the quickedit module, and if they do, having that class in there makes no sense. This is exactly the sort of situation where you *would* want a module to pass in a class through attributes, so the class can only show up when necessary.
This is a functional class, so if somebody is really on a vendetta to remove all classes regardless of the consequences, sure they can do so via removeClass, but hopefully they won't.
Comment #7
Wim LeersOn which template should the module then add this identifying class (though it then really should be an ID, not a class, since there can only ever be one)?
Many templates forget to specify
{{ attributes }}
, which would also break this.So is @mortendk wrong at #2407739: Remove classes from system templates r*.html.twig, where he claims
<main>
is something every theme'spage.html.twig
should have?Comment #8
RainbowArrayWith all due respect for @mortendk, yes, I believe he is wrong on that. I have definitely moved to other templates besides page. Typically node if not page. But there could be lots of situations where it doesn't make sense to put main in page. Particularly when using Panels on a site.
Comment #9
mortendk CreditAttribution: mortendk as a volunteer commentedim never wrong ;)
Anyways
<main>
should be there just as well as<body>
should be there. If a theme figures out they dont wanna have that ... well what can we then do ?Hiding such a vital class inside of {{ attributes }} ends up making it hard to work with drupal and honestly causes confusion.
This is the reason i advocating for having data thats needed for core functionality to be in plain sight - then you'r fully aware that you doing something bad, when your doing it.
i dont understand what we win by removing the
js-quickedit-main-content
from main in that way its easy for everybody to see where it is & it can be moved to whereever you want it.Comment #10
Wim LeersWe gain:
If
<main>
needs to be there already anyway, then there's no point in also having that class.Comment #11
mortendk CreditAttribution: mortendk as a volunteer commented@wim check imho a page should always have
<main>
- i think we agree :)Comment #12
RainbowArray"should" always have
<main>
and "does" always have<main
are not at all the same thing. If the entire functionality of QuickEdit is relying upon the fact that some themer doesn't use a div instead of a main because whatever reason (or because HTML evolves in the next ten years), then that's pretty fragile.Should is even debatable here. Yes, the W3C says there should be one and only main element in a page. But guess what? WHATWG says you can have as many main elements as you want, the more the merrier. For the record, I think WHATWG is very wrong on this point. However, this shows that assuming that everybody in the world knows there should be just one main element is a fragile assumption at best.
Just to be clear, at least in D7, if you're using Panels, then the $content variable in the page template is where your layouts typically go. So if you have multiple layouts going on inside $content, then very very often it does not make sense to put main inside of the page template. It may make more sense in the template for a node. Or for a particular panel pane. And when moving things around like that, it's very possible to miss putting a main element in one of the templates somewhere on the page.
Mind you, I don't know if putting this class in attributes helps in this case. I have no idea how QuickEdit is supposed to know what is the actual main content that can be quick-edited in a Panels-like scenario where the main content may well not be defined within the page template.
Comment #13
Wim LeersThe conclusion is: we don't need any of this anymore :) Because we can now reliably add a class to the page title, we can also reliably find the lowest common ancestor in JS, and so Quick Edit is now able to figure things out on its own!
So, totally unintentionally, #2476947: Convert "title" page element into a block brought the solution, by simply consistently applying patterns we've developed over the years: every thing that is a semantical thing of its own should have its own template, and therefore its attributes can be extended.
Comment #14
Wim LeersActually, I think this almost more belongs in
quickedit.module
at this point.Comment #15
mortendk CreditAttribution: mortendk as a volunteer commentedhurray FIX ALL THE THINGS!
... if u use
.page-title
i might get a bit cranky ;)another thing we should not assume anything based on how drupal7 build stuff, cause well ... drupal8 is 100% the opposite way, as we not have templates to fix all the things & not markup-class-configuration-nightmare that d7 turned out to be ;)
Comment #17
Wim LeersHelpTest
got in the way because it's stupid.Comment #18
RainbowArrayManually tested this. While the main content is quick editable with this patch, the page title is not. Tested this on HEAD as well, and the page title is quick editable there. You need to quick edit the main content, then you can quick edit the title and the main content. With this patch, you can only quick edit the main content.
In an ideal world, the contextual links for the page title would allow for quick edit of the page title separate from the quick edit of the main content, but that's a larger issue.
Comment #19
RainbowArrayComment #20
Wim LeersComment #21
RainbowArrayManually tested this, quickedit works on both the title and the body from the body contextual quick edit link now. Nice work!
Comment #22
Wim LeersPer #21.
Comment #23
RainbowArrayThis is a nice improvement. Manually tested, works well, code looks good to me. Let's get this in!
Comment #27
RainbowArrayLooks like aws fail.
Comment #28
Wim LeersI'm pretty sure this is what @mdrummond meant to do in #27, per #23 :)
Comment #29
webchickWell, that's definitely much simpler. I believe this is still on the table given we don't freeze markup until RC1.
Committed and pushed to 8.0.x. Thanks!
Comment #31
mortendk CreditAttribution: mortendk as a volunteer commentedBAM! all the little awesomethings congrats wim :)
Comment #32
Wim LeersOne less DrupalWTF for front-end people! :)