Only emit Quick Edit data- attributes when actually necessary

First: This is non-sensitive data.

Second: This is by design. It allows us to serve the same render cache items to both anonymous and authenticated users. So it allows us to have more effective caching.

Third: Fields get wrapped with yet another layer of divitis for anonymous users when quickedit is activated. is wrong — if you look at the screenshot you provided, you see that it's merely adding data- attributes to the already-existing field wrapper. (As you can see in quickedit_entity_view_alter().)

Fourth: as a nice side effect, it ensures things don't break when using Quick Edit as an authenticated user, because the CSS and markup is the same for all users. So it helps prevent the "themers only tested this as the anonymous user, i.e. without Quick Edit's metadata, and now it breaks with Quick Edit"-problem.

So I wonder: is it really a problem, given that it does not add more divitis?

Ugh. This is miserable. Absolutely miserable.

I can get behind the argument about caching. But I am not convinced that the performance benefits outweigh the "clean markup" benefit. It simply feels wrong to expose markup intended for editors to end-users. I admit that this is a lesser issue if we really are *only* adding data- attributes (I have so far found data-quickedit-entity-id and data-quickedit-field-id).

The theming aspect is also relevant, since I have had quite a few discussions with frontend (non-Drupal) developers now about how they should manage the stuff that quickedit "does" to the frontend for authenticated users. So far, we have decided to accept that the we have the same markup for both.

Mainly though, we had a situation where the addition of the field wrapper around a responsive <picture> element led the frontend developer to believe that Quickedit was causing images to not be shown (it was obscured via that wrapper, due to how they manage images in CSS), and so they had to add height to that wrapper. They did this by targeting the quickedit-field class which is added to the wrapper from quickedit.js, which is obviously only loaded for users with correct permissions. So now we have a situation where the markup is *not* the same after all, and where the Quickedit markup behaviour has been mangled into place to account for a custom theming approach.

I imagine this topic will come up again (and again), so I am reopening for more discussion (sorry!).

They did this by targeting the quickedit-field class which is added to the wrapper

Well, that's very misguided. One should always first understand why classes are present before using them in CSS selectors.

#7 + #8: I've already given arguments in #5. If you have a solid change proposal, I'm all ears.

At this point I'm with @Wim Leers I think we need more info here, the arguments in #5 seem pretty sound to me. A data attribute is the correct thing to use here, IMO it would be worse if it was yet another class (even if it were js- prefixed).

Would it ease the pain or misery at all if we could figure out a way to get the class attribute to come before the quickedit data attribute(s)? :)

I could care less what attributes quick edit module wants to use. Data attributes are a perfectly valid use case for this.

What I loathe here, and find unacceptable is that this markup is littered in the markup when QuickEdit module is NOT available, while giving your entire backend configuration away to the public, exposing it to search engines and anything and everyone else. While some will argue (see #5-1), IMO, it is a potential security issue. Maybe not a serious one, but I find it crazy that we give away the entire configuration away:

data-quickedit-field-id="entity/id/field_name/lang/view_mode"

Better make sure your entity and field machine names don't give away any non-public information.

It's the whole 2 steps forward, 3 steps back. Theming WTF/garbage in the name of performance. I don't know what the actual difference is in terms of caching, but I hope it's a very significant difference, and even then I'm not sure I'd call it worth it.

I could care less what attributes quick edit module wants to use. Data attributes are a perfectly valid use case for this.

Great!

What I loathe here, and find unacceptable […]
Better make sure your entity and field machine names don't give away any non-public information.

First of all, the sensitiveness of that information is highly questionable. But let's assume it is. Would it not be exposed without Quick Edit? Let's look:

Entity

<article data-history-node-id="7" data-quickedit-entity-id="node/7" role="article" class="contextual-region node node--type-article node--promoted node--view-mode-teaser clearfix" about="/en/node/7" typeof="schema:Article" data-quickedit-entity-instance-id="0">

contains:

1. data-quickedit-entity-id="node/7", which indeed exposes the entity type ID
2. class="… node node--type-article node--promoted node--view-mode-teaser …", which also exposes the entity type ID, and actually even more: the bundle, whether it's promoted or not, and even the view mode

Field

<div property="schema:text" data-quickedit-field-id="node/7/body/en/teaser" class="clearfix text-formatted field field--name-body field--type-text-with-summary field--label-hidden field__item quickedit-field">

contains:

1. data-quickedit-field-id="node/7/body/en/teaser", which indeed exposes the field name
2. class="… field--name-body …", which also exposes the field name, and actually even more: the precise field type

In other words: the existing classes already expose far more information.

Finally, note that Quick Edit will only show up on entities & fields that end up being rendered anyway, and they are only rendered if the current user can actually access them.

First of all, the sensitiveness of that information is highly questionable. But let's assume it is. Would it not be exposed without Quick Edit?

I know that people will want to argue it to death. I just disagree. You're also assuming I have RDF enabled in your examples, which I do not, and forgetting that these classes can be changed, if desired. I don't like what's being produced in RDF either, but I CAN disable it. Here I would have to disable QuickEdit entirely, which is a huge part of the editorial experience.

Also, the classes do not expose everything, especially not all together, in the form of paths, that are also sometimes accessible via URL.

/node/100
/node/100/field_headline/en/teaser

These are like SQL queries in markup... Straight up.

If that's needed for someone to actually USE QuickEdit, great! But, anonymous users sure as hell don't need or use it, and therefore this is just garbage in my markup, that also happens to display my backend configuration to the world. There is no denying that. It also sends a great message to contrib that littering your markup is fine as long as you get better caching at the end of the day.

You guys don't want to fix it. Fine.
You don't want to acknowledge that SQL-like queries in markup, required for a module that is not even running, is not ideal, fine.
I don't really want to argue. I just hate this decision with a passion.

PS - This is not a support request. :)

I don't like what's being produced in RDF either, but I CAN disable it.

Before these data- attributes, Quick Edit actually depended on the RDF module, and used the RDFa metadata. Sounds like it's a good thing that's no longer the case, or you'd have hated this even more :)

in the form of paths, that are also sometimes accessible via URL

They're not paths. They're not URLs. They're just slash-separated strings. We could make them colon-separated if you want. We could also have made it into JSON ({entityType:'node',entityId:100,fieldName:'field_headline',langcode:'en',viewMode:'teaser'}) but that'd have made it ridiculously verbose.

But, anonymous users sure as hell don't need or use it, and therefore this is just garbage in my markup

Then you probably also dislike:

1. data-drupal-selector all over the place: https://www.drupal.org/node/2503277
2. Load /node/3 as the anonymous user. View source, and note this is present in your markup:

   <link rel="delete-form" href="/en/node/3/delete" />
   <link rel="edit-form" href="/en/node/3/edit" />
   <link rel="version-history" href="/en/node/3/revisions" />
   <link rel="revision" href="/en/node/3" />
   <link rel="drupal:content-translation-overview" href="/en/node/3/translations" />
   <link rel="drupal:content-translation-add" href="/en/node/3/translations/add" />
   <link rel="drupal:content-translation-edit" href="/en/node/3/translations/edit" />
   <link rel="drupal:content-translation-delete" href="/en/node/3/translations/delete" />
   

I just hate this decision with a passion.

You just really want clean markup that is as clean/simple/empty/minimal as possible, always. I understand that!

You guys don't want to fix it. Fine.

I do. I said this in #9: If you have a solid change proposal, I'm all ears. I mean that.

Overall, the problem is that in many cases, "cleanest markup possible" requires many variations of a particular piece of markup. In cases like Quick Edit's data- attributes and those <link rel="…" /> tags, they're pure declarations that are not at all user-specific. Hence it's beneficial to just always show them. Conditionally showing them means by definition more computations and worse cacheability.

If you see an alternative implementation here, I'd love to make it happen!

They're not paths. They're not URLs. They're just slash-separated strings. We could make them colon-separated if you want. We could also have made it into JSON ({entityType:'node',entityId:100,fieldName:'field_headline',langcode:'en',viewMode:'teaser'}) but that'd have made it ridiculously verbose.

I said "in the form of paths, that are also sometimes accessible via URL" and stand by that. /node/100 is definitely a path, and one that is accessible if you append it in your address bar. If didn't show up for users without permission to use QuickEdit, I wouldn't care.

Before these data- attributes, Quick Edit actually depended on the RDF module, and used the RDFa metadata. Sounds like it's a good thing that's no longer the case, or you'd have hated this even more :)

That would have been an atrocity.

Then you probably also dislike:

I don't care about #1, but #2.... I don't even have the energy for this right now.

I do. I said this in #9: If you have a solid change proposal, I'm all ears. I mean that

I believe you, and I do appreciate that. But, unfortunately for me, I'm SOL. You said:

It allows us to serve the same render cache items to both anonymous and authenticated users. So it allows us to have more effective caching.

All I can tell you, is what I have already:

1. I hate it. "things-that-make-themers-scream" is the most appropriate tag ever.
2. I cannot measure the difference between having 1 vs 2 separate cache versions for anonymous and authenticated users. But, I suspect having 2 versions is not that big of a deal.
3. Being that I cannot even measure the difference, I can hardly give you a solid change proposal.

And ultimately, I should just give up now, because I know how this will end.... It will end with a maintainer refusing to commit a fix because it changes markup that people may or may not be depending on now. So, it's all probably just a waste of time to say anything at all. I'm probably better off putting energy into disabling the QuickEdit module, which is unfortunate.

I probably just need to get out of the core issue queue in general before it starts sucking the life out of me again.

But, I suspect having 2 versions is not that big of a deal.

It's not two versions. It's as many as there are combinations of permissions/roles. So just out of the box that is already 3! = 3*2*1 = 6.

It will end with a maintainer refusing to commit a fix because it changes markup that people may or may not be depending on now.

I guarantee this is not the case, precisely because this is a data- attribute only used and controlled by Quick Edit. Which is why we're free to change it.

I really, really want to un-frustrate you (if that's even a word :)). I want to help you make things better.

That got me thinking.

I just said above that it's not just 2 versions, it's 6. Because it needs to vary by combination of permissions. But… since #2493033: Make 'user.permissions' a required cache context we already do that for every freaking rendered thing anyway! So my entire argument is moot! (In this particular case at least: because it varies by permissions.)

Trivial patch. With updated test coverage.

Jacine++

OMG. You just made my week. I will be testing this first thing tomorrow morning. Cannot thank you enough!!! :D

Okay that's really exciting then! I call dibs on committing this. :)

I'll let Jacine RTBC but tested it and works great. RTBC +1

Also when a Wim comment has a <hr>, you know it'll be a good one.

So much love!

That was an amazingly positive outcome! FWIW, I've tested the patch in #23 and it works as expected -- no more quickedit data attributes! So another RTBC+1 from me.

So .. does this mean the same might be possible for contextual.module (in a follow-up issue)?

    <div data-contextual-id="node:node=5:changed=1457008057&amp;langcode=da"></div>

Related issue in contextual links - see #2650246: Only emit Contextual Links data- attributes when actually necessary

#24: The answer is "yes", as @swentel shows in #25 :)

#25: reviewed!

Updated IS.

Just tested and it works perfectly. TX just got MUCH better.

You guys did not have to wait for me, but I appreciate it! RTBC.

This is not at all how I expected this issue to go. When I commented initially I didn't even bother to set the issue back to active, because I really didn't think there was even a slim chance. Kudos to you guys for allowing this sort of change after release. That's huge. And thank you Wim, for your actual concern and dedication here. It's not always easy to do that, so I appreciate it that much more. Respect! :D

:)

Could you then also review #2650246: Only emit Contextual Links data- attributes when actually necessary and perhaps also RTBC it? :)

Yes, for sure! Will go review that as soon as I can (probably not today).

:D

Committed and pushed fa5efed to 8.2.x and c1e7d05 to 8.1.x. Thanks!

I'm not committing this to 8.0.x because this isn't a bug fix and is technically changing markup as well (#5 in https://www.drupal.org/core/d8-allowed-changes#evaluate). On the bright side 8.1.0 will be out before we know it! :)

Thanks everyone and particularly @Jacine and @Wim Leers, this is a really great outcome that will make people happy!