Problem/Motivation

Pretty much like before 8.0.x in #2139921: Contextual links can't handle multiple occurrences of the same contextual links, contextual module does not support multiple contextual blocks with the same parameters, although it manifests a bit differently.

When the same content appears more than once on the same page, that content naturally has the same contextual links (specifically, exactly the same contextual links placeholder, e.g. <div data-contextual-id="foo:"></div>). Each contextual link should work correctly and act independently, of course.

However, that is not what happens. In the current codebase, when clicking one contextual link (by clicking on the pencil icon), it does not open. As observed in the chrome devtools, both links receive the event (flashes in the inspector), but none displays.

Steps to reproduce

  • Create a render array in a controller
  • Add a #contextual_links element, pointing to some section with appropriate parameters
  • Add a child element within the top-level render array
  • Within that element, add a similar #contextual_links element, with the same parameters

Go to the page for that controller. The same contextual link now appears twice: once in the main content area, once in the other element further down. Click either contextual link. No contextual link menus will be opened.

Workaround

Add an unused extra route_parameter, with a different value in each element like ['instance' => 1] in the first one, ['instance' => 2] in the second one. The blocks will now work normally, and the links will only carry the extra unused parameter in the query.

This is pretty much how multiple pagers work.

Proposed resolution

Either:

  • Fix this.
  • Or document the workaround
CommentFileSizeAuthor
#37 2891603-8.9.x-37.patch12.42 KBalexpott
#37 30-37-interdiff.txt1.18 KBalexpott
#36 2891603-36.patch12.36 KBalexpott
#30 interdiff2730.txt4.28 KBeiriksm
#30 2891603.patch12.72 KBeiriksm
#30 2891603-test-only.patch11.17 KBeiriksm
#27 interdiff-19-27.patch13.21 KBeiriksm
#27 2891603-27.patch12.69 KBeiriksm
#27 2891603-test-only.patch11.14 KBeiriksm
#25 2891603.patch7.8 KBeiriksm
#19 2891603-18.patch4.58 KBcharlietoleary
#17 2891603-17.patch4.32 KBcharlietoleary
#16 2891603.16.patch4.24 KBgrayle
#14 2891603-14.patch4.23 KBgrayle
#8 contextual-links_counter_for_unique_id-2891603-8.patch1.54 KBdrclaw
#5 interdiff-5.txt1.29 KBAnonymous (not verified)
#5 2891603-5.patch4.27 KBAnonymous (not verified)
#5 2891603-5-test-only.patch2.99 KBAnonymous (not verified)

Comments

fgm created an issue. See original summary.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Issue summary: View changes
fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Related issues: +#2139921: Contextual links can't handle multiple occurrences of the same contextual links

Added reference to original issue.

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.

Anonymous’s picture

Anonymous (not verified) commented
Status: Active » Needs review
Related issues: +#2834346: Contextual links double trigger
StatusFileSize
new 2891603-5-test-only.patch2.99 KB
new 2891603-5.patch4.27 KB
new interdiff-5.txt1.29 KB

@fgm, thanks for this issue and nice IS. Your workaround also works for me.
We can add key with random (or pseudorandom) value to 'route_parameters' in _contextual_id_to_links(). This will give us the guarantee of the uniqueness of any link. But many tests rely on the ability to predict link value.

So here is another solution. This prevents binding of several javascript object with one html tag. Because the links cease to work due to the fact that when you click on them, several models immediately consider themselves the owners of the pencil icon :)

It seems this reason is also very suitable for #2834346: Contextual links double trigger.

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

Anonymous’s picture

Anonymous (not verified) commented
Related issues: +#2884801: Views block duplicated contextual links

Another related issue with a good description of the problem of binding objects.

drclaw’s picture

drclaw commented
StatusFileSize
new contextual-links_counter_for_unique_id-2891603-8.patch1.54 KB

Could do it server-side as a counter which is a little more predictable for testing? Something like the attached patch? (testing disabled since we know it will break current tests)

Anonymous’s picture

Anonymous (not verified) commented

@drclaw, thanks for the suggestion. But it seems to me that different id for the same contextual links is a good idea for the contrib module. When for some reason we need to implement different behavior to equal links. But for the core this logic is unnecessary complication.

We just need to eliminate the bug with multiple processing of links (js building links from web-cache, where the problem arises).

Anonymous’s picture

Anonymous (not verified) commented

Here is the detailed description:

  1. When the page is loaded for the first time, then JS saves all the contextual links to Session Storage (in the browser).
  2. If there are two identical links on the page, they will both in Session Storage without override.
  3. When the page is loaded a second time, the links are taken from the cache, and applied to the related placeholders.
  4. As a result, if there are two placeholders with identical id on the page, than two links will be applied to both of them.
  5. An 'open' event for first link, it is 'close' event for second link. As a result, nothing to shows.

Those, we need just set 1 to 1 relationship between model of contextual links and placeholders (like #5). Or just prevent duplication link ids in Session Storage.

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

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

Nik W’s picture

Nik W commented

Any updates on this?
Still running into this issue...

grayle’s picture

grayle commented
StatusFileSize
new 2891603-14.patch4.23 KB

Reroll for 8.6.1

SriHarshaRavi’s picture

SriHarshaRavi commented

I was having the same issue and The patch in #5 fixed the issue for me.

grayle’s picture

grayle commented
StatusFileSize
new 2891603.16.patch4.24 KB

Reroll for 8.6.2

charlietoleary’s picture

charlietoleary commented
StatusFileSize
new 2891603-17.patch4.32 KB

Hey guys,

I've patched this against 8.7.x tree.

I also changed the checks in the JS to use the eq() jQuery method: as this is better for modern browsers, as per jQuery docs (https://api.jquery.com/eq-selector/):

"Because :eq() is a jQuery extension and not part of the CSS specification, queries using :eq() cannot take advantage of the performance boost provided by the native DOM querySelectorAll() method. For better performance in modern browsers, use $("your-pure-css-selector").eq(index) instead."

Status: Needs review » Needs work

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

charlietoleary’s picture

charlietoleary commented
StatusFileSize
new 2891603-18.patch4.58 KB

My bad, missed a line in the tests.

Another Patch.

charlietoleary’s picture

charlietoleary commented
Priority: Normal » Major
Status: Needs work » Needs review

Elevating to major for visibility and also the original issue #2139921 was also marked major.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

anonym-developer’s picture

anonym-developer commented

Any updates here?

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

kerasai’s picture

kerasai commented

Not sure I fully-grok the what/how/why of this approach. I've updated front-end approach in #2834346: Contextual links double trigger and it seems to be working.

If someone has a good explanation of the backend approach on this issue or has strong opinions about one over the other, I'd love to hear.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
commented
StatusFileSize
new 2891603.patch7.8 KB

Tested the patch in #19, works great. Since we have a test-only patch that fails, and several has tested this solution to be OK, this should probably be marked as RTBC. But it has some coding standard errors in the test class, so here is an updated patch.

Status: Needs review » Needs work

The last submitted patch, 25: 2891603.patch, failed testing. View results

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Status: Needs work » Needs review
StatusFileSize
new 2891603-test-only.patch11.14 KB
new 2891603-27.patch12.69 KB
new interdiff-19-27.patch13.21 KB

Last patch had some leftover diff from another issue so ignore that.

It did however show a new test failure, so here is a new patch adressing that.

The last submitted patch, 27: 2891603-27.patch, failed testing. View results

The last submitted patch, 27: 2891603-test-only.patch, failed testing. View results

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
commented
StatusFileSize
new 2891603-test-only.patch11.17 KB
new 2891603.patch12.72 KB
new interdiff2730.txt4.28 KB

OK, new try. Let's move the new test to a new class, so we dont mess up the existing tests :)

The last submitted patch, 30: 2891603-test-only.patch, failed testing. View results

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

ocastle’s picture

ocastle commented

Patch #30 tested on two sites and solves the issue for me on both. Great Stuff!

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
commented

If you tested and reviewed the patch you might also consider changing the status of the issue to "reviewed and tested by the community"? :)

ocastle’s picture

ocastle commented
Status: Needs review » Reviewed & tested by the community

RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: +Needs reroll
StatusFileSize
new 2891603-36.patch12.36 KB

The patch needed to be rerolled for JS tooling changes in 9.x - I applied #30 without patching core/modules/contextual/js/contextual.js. I then ran:

yarn run lint:core-js-passing --fix
yarn run build:js

As this doesn't change any functionality, and in a better world we wouldn't commit core/modules/contextual/js/contextual.js anyway, leaving at rtbc

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 30-37-interdiff.txt1.18 KB
new 2891603-8.9.x-37.patch12.42 KB

Oh the 8.9.x patch fails the linting rules too...

/Users/alex/dev/drupal/core/modules/contextual/js/contextual.es6.js
  189:23  error  Replace `.find(`[data-contextual-id="${contextualID.id}"]:empty`)` with `⏎················.find(`[data-contextual-id="${contextualID.id}"]:empty`)⏎················`  prettier/prettier

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

So let's fix that too. Same yarn commands run on 8.9.x...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs reroll

Committed and pushed 0c74be7ae2 to 9.1.x and a1239b97e4 to 9.0.x. Thanks!

Committed 796e8dc and pushed to 8.9.x. Thanks!

  • alexpott committed a1239b9 on 9.0.x 
    Issue #2891603 by eiriksm, alexpott, charlietoleary, Grayle, drclaw, fgm...

  • alexpott committed 796e8dc on 8.9.x 
    Issue #2891603 by eiriksm, alexpott, charlietoleary, Grayle, drclaw, fgm...

  • alexpott committed 0c74be7 on 9.1.x 
    Issue #2891603 by eiriksm, alexpott, charlietoleary, Grayle, drclaw, fgm...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue tags: +JavaScript

Status: Fixed » Closed (fixed)

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