It would be nice to have contextual links activated on media/{mid} pages. Its also nice to use quickedit on these pages.

CommentFileSizeAuthor
#64 contextual_links.png108.2 KBxjm
#56 interdiff-51-56.txt2.23 KBseanB
#56 2775131-56.patch2.47 KBseanB
#51 interdiff-43-50.txt1.96 KBmarcoscano
#51 2775131-50.patch3.56 KBmarcoscano
#43 2775131-43.patch2.39 KBmarcoscano
#42 2885131-42-this-patch-only-do-not-test.patch2.39 KBmarcoscano
#42 2775131-42-with-dependencies.patch12.75 KBmarcoscano
#32 interdiff-2775131-28-32.txt1.31 KBchr.fritsch
#32 2775131-32.patch2.98 KBchr.fritsch
#28 interdiff-2775131-27-28.txt780 byteschr.fritsch
#28 2775131-28.patch3.06 KBchr.fritsch
#27 interdiff-2775131-21-27.txt1.82 KBchr.fritsch
#27 2775131-27.patch3.94 KBchr.fritsch
#21 interdiff-2775131-18-21.txt1.19 KBchr.fritsch
#21 media_entities_should-2775131-21.patch5.52 KBchr.fritsch
#21 media_entities_should-2775131-21-FAIL.patch1.36 KBchr.fritsch
#18 media_entities_should-2775131-18.patch3.93 KBchr.fritsch
#15 media_entities_should-2775131-15.patch3.9 KBkatzilla
#12 media_entities_should-2775131-12.patch3.75 KBkatzilla
#5 add_contextual_links-2775131-2.mp42.36 MBgippy
#2 add_contextual_links-2775131-2.patch899 byteschr.fritsch
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

chr.fritsch created an issue. See original summary.

chr.fritsch’s picture

Status: Active » Needs review
FileSize
899 bytes

Here is a first patch. We already have a dependency to the entity module, so we could use their EntityViewBuilder which supports contextual link building.

slashrsm’s picture

Issue tags: +D8Media
+++ b/templates/media.html.twig
@@ -13,6 +13,8 @@
+  {{ title_suffix.contextual_links }}
+

Can we print entire title_suffix here?

When was this class added to the Entity module? Do we need to raise minimum version of the dependency?

chr.fritsch’s picture

title_suffix.contextual_links will be set from the contextual module if $element['#contextual_links'] is set. This is done by Drupal\entity\EntityViewBuilder.

So no new dependencies here

gippy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.36 MB

I tested this with simplytest.me. The patch worked exactly as described.

phenaproxima’s picture

Project: Media entity » Drupal core
Version: 8.x-1.x-dev » 8.4.x-dev
Component: Code » other
Status: Reviewed & tested by the community » Postponed
Issue tags: +Needs tests

Ah, video proof! Love it. :) Thank you, @gippy and @chr.fristch!

So, this seems like an issue that is quite possibly affecting the core Media system as well. We should definitely fix it there, since we've pretty much halted development on Media Entity 1.x in favour of core Media. We'll certainly need automated tests for this, but I see no reason why we shouldn't address this once #2831274: Bring Media entity module to core as Media module lands.

I'll be adding this to the list of issues that comprise the core Media MVP: #2825215: Media initiative: Essentials - first round of improvements in core

phenaproxima’s picture

Title: Add contextual links support » Media entities should support contextual links
phenaproxima’s picture

This issue is now part of the core Media Initiative. Postponed on #2831274: Bring Media entity module to core as Media module.

phenaproxima’s picture

Status: Postponed » Needs review
dawehner’s picture

Status: Needs review » Needs work
+++ b/src/Entity/Media.php
@@ -20,7 +20,7 @@ use Drupal\user\UserInterface;
- *     "view_builder" = "Drupal\Core\Entity\EntityViewBuilder",
+ *     "view_builder" = "Drupal\entity\EntityViewBuilder",

relying on the entity module certainly doesn't fly :) Maybe we could bring this particular class into Drupal core?

phenaproxima’s picture

Oooh, nice catch. It seems like bringing the view builder from Entity API to core would be a giant pain in the ass in relation to the scope of this issue, though...surely we can do it some other way that doesn't involve moving parts of Entity API into core?

katzilla’s picture

Status: Needs work » Needs review
FileSize
3.75 KB

Added a new class: MediaViewBuilder and adapted code from NodeViewBuilder to add contextual links. Also added the title_suffix to classy and stable media.html.twig, so this should be tested in those themes.

katzilla’s picture

Issue tags: +drupalsprintberlin
webflo’s picture

  1. +++ b/core/modules/media/src/MediaViewBuilder.php
    @@ -0,0 +1,41 @@
    +/**
    + * Class MediaViewBuilder - View builder handler for media.
    + *
    + * @package Drupal\media
    + */
    
    /**
     * View builder handler for media.
     */
    

    Would be sufficient and more in line with other docs in core.

  2. +++ b/core/themes/classy/templates/content/media.html.twig
    @@ -20,8 +20,14 @@
    +  {% if label and view_mode != 'full' %}
    

    template_preprocess_media provides no label variable. Its "name" in this case.

katzilla’s picture

Added a new patch with suggestions from @webflo. Still works ;)

phenaproxima’s picture

  1. +++ b/core/modules/media/src/MediaViewBuilder.php
    @@ -0,0 +1,39 @@
    +    if ($entity->id()) {
    

    As far as I know, the only circumstances under which $entity->id() would be falsy is if the entity is new...in which case we should be using the more generalized if (!$entity->isNew()) {...}

  2. +++ b/core/modules/media/templates/media.html.twig
    @@ -40,12 +40,11 @@
    -  {% if label and view_mode != 'full' %}
    +  {% if name and view_mode != 'full' %}
         <h2{{ title_attributes }}>
    -      {{ label }}
    +      {{ name }}
    

    I don't understand why this was done, or how it works. Where is name being set? I'm sorry if I'm being dense here, I'm just not sure what this change is doing in the context of this patch.

  3. +++ b/core/themes/classy/templates/content/media.html.twig
    @@ -20,8 +20,14 @@
    +  {{ content }}
    

    It looks like we're now echoing content whether or not it is empty, which is a change from the way it was before. Why is that being done? As with my previous comment, I'm not against it but I'm wondering why it was done.

katzilla’s picture

@phenaproxima - thanks for your feedback!
Just a few words for 2 and 3, I'll check 1 later.

2.) The name variable comes from core/modules/media/media.module - there is a preprocess function: template_preprocess_media.

3) If content variable is empty, nothing gets rendered in Twig, so there is no need to check, if it is empty. We will not get an exception. Also this is how it is done in node template for example. No if statement.

chr.fritsch’s picture

I adjusted the code regarding #16-1

chr.fritsch’s picture

Component: other » media system
phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: -D8Media +Media Initiative

Re-tagging. The patch looks good to me, but it still needs tests.

chr.fritsch’s picture

Ok, here with a very simple test, to check that contextual links will be rendered.

The last submitted patch, 21: media_entities_should-2775131-21-FAIL.patch, failed testing. View results

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/media/templates/media.html.twig
    @@ -37,12 +37,11 @@
    diff --git a/core/modules/media/templates/media.html.twig b/core/modules/media/templates/media.html.twig.orig
    
    diff --git a/core/modules/media/templates/media.html.twig b/core/modules/media/templates/media.html.twig.orig
    similarity index 100%
    
    similarity index 100%
    copy from core/modules/media/templates/media.html.twig
    
    copy from core/modules/media/templates/media.html.twig
    copy to core/modules/media/templates/media.html.twig.orig
    

    oops.

  2. +++ b/core/modules/media/tests/src/Functional/MediaContextualLinksTest.php
    @@ -0,0 +1,55 @@
    +    $this->drupalGet('media/' . $media->id());
    +
    +    $this->assertSession()->responseContains('data-contextual-id="media:media=' . $media->id() . ':');
    

    should we check the revision as well? Edit: Ah, see below, looks like there actually are no revision contextual links, not sure if this is added then or not.

I'm fine with adding it just to media in this issue, but I'm wondering if we already have an issue to add it to the parent? Seems fairly save to do and shouldn't really conflict with anything as long as the entity type doesn't define contextual links. If there is no issue yet, can we open one?

Edit: We could also introduce the concept of a contextual links handler, the view builder could call that and we could also let it generate the contextual link definitions automatically?

What I'm wondering about a bit is that we add media_revision contextual link info as well, if I understand this correctly, this works through the contextual links group, but neither node nor media actually have revision contextual links?

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

chr.fritsch’s picture

phenaproxima’s picture

Status: Needs work » Postponed
chr.fritsch’s picture

chr.fritsch’s picture

Status: Postponed » Needs review
FileSize
3.06 KB
780 bytes

Wohoo, #2791571: Automatically supply contextual links for entities landed.

So let's finish this one.

phenaproxima’s picture

Issue tags: -Needs tests
+++ b/core/modules/media/templates/media.html.twig
@@ -37,12 +37,11 @@
-  {% if label and view_mode != 'full' %}
+  {% if name and view_mode != 'full' %}
     <h2{{ title_attributes }}>
-      {{ label }}
+      {{ name }}

This seems out of scope; why was this change made?

Other than that, this looks pretty good...

chr.fritsch’s picture

Yeah, this is currently an issue in our media.html.twig...

Which we have to fix it to get the contextual links support working.

marcoscano’s picture

I have manually tested and this works as expected. No big remarks on the patch, except a very minor observation:

+++ b/core/modules/media/tests/src/Functional/MediaContextualLinksTest.php
@@ -0,0 +1,55 @@
+    'system',
+    'node',
+    'field_ui',
+    'views_ui',
+    'media',
+    'media_test_source',
+    'contextual',

Probably only 'contextual', is needed here. The base class already declares all others, and they all get added in the end.

I also don't mind the change mentioned in #29, even if unrelated. So for me this is +1 RTBC.

chr.fritsch’s picture

Thanks, @marcoscano for reviewing.

I fixed the remarks from #31 and adjusted the test accordingly to NodeContextualLinksTest.

marcoscano’s picture

Status: Needs review » Reviewed & tested by the community

Assuming the bot doesn't complain, this looks good :)

The last submitted patch, 27: 2775131-27.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Gábor Hojtsy’s picture

Assigned: Unassigned » lauriii
+++ b/core/themes/stable/templates/content/media.html.twig
+++ b/core/themes/stable/templates/content/media.html.twig
  */
 #}
 <article{{ attributes }}>
-  {% if content %}
-    {{ content }}
+  {% if name and view_mode != 'full' %}
+    <h2{{ title_attributes }}>
+      {{ name }}
+    </h2>
   {% endif %}
+  {{ title_suffix }}
+  {{ content }}
 </article>

Hm, stable is meant to be stable, especially for a stable module :) I would refer to @laurii on what exactly is allowed to be changed with stable templates.

phenaproxima’s picture

Tagging for review by front-end experts :)

Berdir’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/themes/classy/templates/content/media.html.twig
@@ -20,8 +20,14 @@
-  {% if content %}
-    {{ content }}
+  {{ title_prefix }}
+  {% if name and view_mode != 'full' %}
+    <h2{{ title_attributes }}>
+      {{ name }}
+    </h2>
   {% endif %}
+  {{ title_suffix }}
+  {{ content }}
 </article>

I'm not sure about this.

I understand we need the title prefix/suffix stuff for contextual links to work, correct?

But this makes more changes, it introduces similar title/name logic that node templates have, including the hardcoded view_mode (in case of node it is the is page flag, but that makes little difference).

What this means is that it basically undoes your own recent change to make the display of the name configurable and it will *always* be there. The thing is that #2912298: Make media name available on manage display did not add any actual test coverage that it works, it merely tests that the API set it to configurable, not that the template actually respects that.

If anything, then we need to show content.name and check for that being visible. thinking about it, I'd even expect this to display the title twice, once in content if configured and then additionally hardcoded.

lauriii’s picture

Assigned: lauriii » Unassigned
Category: Feature request » Bug report
Issue tags: -Needs frontend framework manager review

First of all, I'm changing the category to a bug report as this is a bug where media entities are not compatible with contextual links.

Secondly, I noticed that the templates media module is providing do already support contextual links. Therefore this bug only exists when Stable or Classy are being used.

Looking at the media templates it seems like the templates inside the module have been already been off-sync from Stable and Classy and I'm not sure what is the reason for that. I didn't find any information from #2831274: Bring Media entity module to core as Media module related to that.

According to our BC policy, we are not allowed to make changes to Stable and Classy. However, if we can come up with a non-disruptive way to solve this, the maintainer team could consider making an exception in this case.

I'm not sure if I understand the reason for adding the hardcoded title into the template. Could we remove that since it shouldn't be a requirement for supporting contextual links?

seanB’s picture

Thanks for pointing me here Berdir! This seems to conflict with #2930788: Do not show name by default in media displays. If we use a view mode to render media in entity reference fields by default, this means you will always get the media title. That will also break the change in #2895382: Hide label, thumbnail, etc. for default display of media file and image references.

I don't know enough about how the contextual links work, but hardcoding the H2 is definitely an issue.

+++ b/core/themes/classy/templates/content/media.html.twig
@@ -20,8 +20,14 @@
+  {{ title_prefix }}
+  {% if name and view_mode != 'full' %}
+    <h2{{ title_attributes }}>
+      {{ name }}
+    </h2>
   {% endif %}
+  {{ title_suffix }}
marcoscano’s picture

This might explain why the title is not being shown twice :)
#2931057: Media template should use 'name' instead of 'label'

seanB’s picture

The patch in #32 contains the same change as #2931057: Media template should use 'name' instead of 'label'. Let's fix the other issue first before we continue here.

marcoscano’s picture

Title: Media entities should support contextual links » [PP-1] Media entities should support contextual links
Status: Needs work » Postponed
FileSize
12.75 KB
2.39 KB

This should definitely be solved after #2930788: Do not show name by default in media displays is fixed.

For reference, this is a patch rerolled on the top of #2930788-33: Do not show name by default in media displays.

marcoscano’s picture

Title: [PP-1] Media entities should support contextual links » Media entities should support contextual links
Status: Postponed » Needs review
FileSize
2.39 KB

#2930788: Do not show name by default in media displays landed, so we should be good to go with this now.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, @marcoscano!

Despite the fact that this patch changes Stable, it is a bug fix and therefore probably justifiable from a BC standpoint, as @lauriii pointed out in #38. So I think this is RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 43: 2775131-43.patch, failed testing. View results

marcoscano’s picture

Status: Needs work » Reviewed & tested by the community

Testbot glitch

alexpott’s picture

+++ b/core/modules/media/templates/media.html.twig
@@ -32,5 +32,6 @@
+  {{ title_suffix }}

+++ b/core/themes/classy/templates/content/media.html.twig
@@ -21,6 +21,7 @@
+  {{ title_suffix }}

+++ b/core/themes/stable/templates/content/media.html.twig
@@ -13,6 +13,7 @@
+  {{ title_suffix }}

This looks super odd... just printing the suffix without the rest. I guess this is because contextual links just add themselves to the title_suffix. Should we add a comment to the template header to explain this? Or we could change this to {{ title_suffix.contextual_links }} which would be self-documenting. Not sure.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

Or we could change this to {{ title_suffix.contextual_links }} which would be self-documenting.

I like the idea of that, but there are two problems I can see:

  1. Nothing else in core does that, as far as I know.
  2. Anyone trying to alter the title_suffix on media items, or add things to it, will be thwarted. Which means this will probably resurface later.

Personally, I think we should just add a comment in the template header to explain, and leave it at that.

Berdir’s picture

Comment works for me.

There's also the theoretical option of re-thinking those special title suffix/prefix variables, how they're named and placed. But I'm not surehow to exactly make changes there in a BC compatible way.

Btw, the node template only adds those if there is a title, meaning not on the node page. This now adds them always, does this mean that there are now also contextual links on the media page? Or is contextual module already not adding them on the full page?

alexpott’s picture

Anyone trying to alter the title_suffix on media items, or add things to it, will be thwarted.

Well they'll be thwarted trying to alter the title or title_prefix too? Won't these problems surface later too? I think doing the minimum necessary to support contextual links is preferable to adding something which looks super obscure and begs more questions - ie? Where's the title this is suffixing? Where's the prefix?

marcoscano’s picture

Status: Needs work » Needs review
FileSize
3.56 KB
1.96 KB

Added a comment to the templates.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Well they'll be thwarted trying to alter the title or title_prefix too? Won't these problems surface later too? I think doing the minimum necessary to support contextual links is preferable to adding something which looks super obscure and begs more questions - ie? Where's the title this is suffixing? Where's the prefix?

IMHO, stuffing the contextual links into {{ title_suffix }} is the source of the confusion, but that was a decision made long ago. Contextual links should be their own template variable, precisely to eliminate such implicit weirdness. But that would be outside the scope of this issue. Follow-up, maybe?

I think the template comment @marcoscano added in #51 should be enough to reduce themer confusion in the short term until the title_suffix and title_prefix can be properly untangled, which will obviously take a much longer time. So I'm moving this back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 51: 2775131-50.patch, failed testing. View results

marcoscano’s picture

Status: Needs work » Reviewed & tested by the community

Testbot misbehaving

alexpott’s picture

I woke up this morning thinking about the principle of least astonishment and I think the solution here violates that. Because if you are printing title_suffix you would expect both the title and the title_prefix to be printed as well. I think the solution that just prints the contextual links is the better and least surprising solution. I can imagine things in custom and contrib that add opening tags to title_suffix and closing things to title_prefix and this is going to be broken by this change.

seanB’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.47 KB
2.23 KB

I agree with alexpott. Showing the contextual links only is more explicit for now. Moving the contextual links to it's own variable would be way better, but that is a separate issue.

Updated the patch.

alexpott’s picture

Discussed with @phenaproxima on slack. tldr; we agreed that {{ title_suffix.contextual_links }} was the best solution with the least possible side effects.

Here's the discussion:

phenaproxima: @alexpott So about the contextual links...IMHO the least astonishing way to do it is to do what every other template in core does
phenaproxima: @alexpott Because if we have to change it later, in stable, we will be breaking BC
alexpott: @phenaproxima so then you have to add the title and title_suffix - which you can't do.
phenaproxima: @alexpott Why? We can just add {{ title_prefix }} and {{ title_suffix }} with no title between them.
alexpott: But that is also astonishing
phenaproxima: @alexpott But is it less astonishing than just showing the contextual links?
alexpott: In my opinion yes. We have no way for things to arbitrarily dump stuff into templates - contextual is using title_prefix to do this and it is super weird. But just printing title_suffix.contextual to enable contextual links in media is totally unastonishing given how contextual links works. Also printing just title_suffix.contextual is the smallest way to fix the actually bug with the least amount of possible side effects
phenaproxima: @alexpott Well, I just asked another opinion and they agree with you, so I guess I concede this one
alexpott: Which, I think is also preferred given the imponderables of what people do on real sites
phenaproxima: @alexpott I'd rather just get contextual links working than debate it for very long =P
alexpott: @phenaproxima "just get contextual links working" = do smallest possible change with least possible side effects :slightly_smiling_face:
phenaproxima: @alexpott Okay then :slightly_smiling_face: We agree. We should have a follow-up, maybe, to untangle contextual links from the prefix/suffix variables

There are issues about how contextual uses title_suffix in the core queue, for example: #2933695: Move contextual links out of context maybe we need a more generic one.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs frontend framework manager review

#52 - #57 seems reasonable.

The current patch though is still making changes to Stable and Classy, which @lauriii raised in #38 with the previous approach because it's a BC break. I think we'd need frontend framework manager's signoff if we were to go ahead with changing the stable base templates.

To avoid breaking BC for the stable base themes, we could make the change to the Media template itself and copy it to Bartik and Seven, but not change the base themes. (A followup could do that later if we decide the bug is buggy enough to break BC for the base themes.)

A second opinion might be good but let's start with that for now and maybe file a followup. I'll also ping @lauriii for his thoughts again.

lauriii’s picture

The latest patch seems to have addressed concerns that I brought up in my previous comment. I also think that the current approach is fine - we don't have to solve problems outside the scope of this issue.

I think on this issue it would be worth taking the risk to break BC and provide this fix in Classy and Stable. This change is in the safer end of potential changes that could be done (adding a new child element inside a pre-existing wrapper). Also, not fixing this bug in Classy and Stable would cause confusing UX because of missing contextual links on some sites.

It would be great if we could have a small change record explaining the change in the markup to make sure developers are aware that this might affect their sites.

xjm’s picture

Thanks @lauriii! So then we just need the CR and then this can have its final review.

marcoscano’s picture

Status: Needs work » Needs review

Created CR: https://www.drupal.org/node/2935088

Not sure if the impact on themers should be more explicit, please advise if so.

Thanks!

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Then I think we’re ready.

xjm’s picture

I added a bit more explicit statement about the impacts:
https://www.drupal.org/node/2935088/revisions/view/10782605/10782949

xjm’s picture

Issue summary: View changes
FileSize
108.2 KB

Alright, the approach has signoff from a frontend maintainer, the CR is in place, and I manually tested and confirmed it resolved the issue without any side effects:

Saving issue credit.

  • xjm committed 1da5a5c on 8.5.x
    Issue #2775131 by chr.fritsch, marcoscano, katzilla, seanB, xjm, gippy,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.5.x and published the CR. Thanks!

tstoeckler’s picture

Status: Fixed » Reviewed & tested by the community

I think #51 rather than #56 was committed here by accident.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Whoops, good spot. Fixed, thanks.

  • xjm committed 9f54d00 on 8.5.x
    Revert "Issue #2775131 by chr.fritsch, marcoscano, katzilla, seanB, xjm...
  • xjm committed b8d018e on 8.5.x
    Issue #2775131 by chr.fritsch, marcoscano, katzilla, seanB, xjm, gippy,...

Status: Fixed » Closed (fixed)

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