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.

CommentFileSizeAuthor
#20 interdiff.txt813 bytesWim Leers
#20 2476947-20.patch5.32 KBWim Leers
#17 interdiff.txt1.09 KBWim Leers
#17 2476947-17.patch5.24 KBWim Leers
#13 2476947-13.patch4.19 KBWim Leers
#2 2568099-2.patch3.49 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs review
FileSize
2568099-2.patch3.49 KB
davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented
Issue tags: +Needs manual testing

Any time we touch something related to Quick Edit weird things pop up, so ...

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray commented

Hmm.

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.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Bump.

It'd be very sad to have js-quickedit-main-content being a hardcoded class in all our page.html.twig templates.

#4: see #2407739-45: Remove classes from system templates r*.html.twig, which claims <main> is required anyway.

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray commented

I'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.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

On 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's page.html.twig should have?

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray commented

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

mortendk’s picture

mortendk CreditAttribution: mortendk as a volunteer commented

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

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.

We gain:

  • one less DrupalWTF
  • more robustness, because themers won't even need to know about Quick Edit to be compatible with it.

If <main> needs to be there already anyway, then there's no point in also having that class.

mortendk’s picture

mortendk CreditAttribution: mortendk as a volunteer commented

@wim check imho a page should always have <main> - i think we agree :)

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray commented

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +JavaScript
FileSize
2476947-13.patch4.19 KB 
16:03:06 WimLeers: mdrummond: re-investigating
16:03:22 WimLeers: mdrummond: you know what's ironic?
16:03:34: WimLeers: mdrummond: we started discussing this as soon as the 'title block' patch landed
16:03:44 WimLeers: mdrummond: and that patch means the page title can appear ANYWHERE
16:03:48 WimLeers: mdrummond: which COMPLETELY CHANGES THINGS
16:03:54 WimLeers: mdrummond: :P

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Component: theme system » quickedit.module

Actually, I think this almost more belongs in quickedit.module at this point.

mortendk’s picture

mortendk CreditAttribution: mortendk as a volunteer commented

hurray 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 ;)

Status: Needs review » Needs work

The last submitted patch, 13: 2476947-13.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
FileSize
2476947-17.patch5.24 KB
interdiff.txt1.09 KB

HelpTest got in the way because it's stupid.

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray commented

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

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray commented
Status: Needs review » Needs work
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
FileSize
2476947-20.patch5.32 KB
interdiff.txt813 bytes
RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray commented

Manually tested this, quickedit works on both the title and the body from the body contextual quick edit link now. Nice work!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: -Needs manual testing

Per #21.

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray commented
Status: Needs review » Reviewed & tested by the community

This is a nice improvement. Manually tested, works well, code looks good to me. Let's get this in!

The last submitted patch, 13: 2476947-13.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: 2476947-20.patch, failed testing.

mdrummond queued 20: 2476947-20.patch for re-testing.

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray commented
Status: Needs work » Needs review

Looks like aws fail.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

I'm pretty sure this is what @mdrummond meant to do in #27, per #23 :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

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

  • webchick committed e36c84d on 8.0.x 
    Issue #2568099 by Wim Leers, mdrummond, mortendk: Follow-up for #2407739...
mortendk’s picture

mortendk CreditAttribution: mortendk as a volunteer commented

BAM! all the little awesomethings congrats wim :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

One less DrupalWTF for front-end people! :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.