Nested contextual links triggers sometimes unreachable

[bugfolder]

Description of the bug

When contextual links regions are nested, the current behavior of Contextual module is to shift the child links down so that the triggers don't overlap each other. However, if the child region is short (e.g., the breadcrumb), then the trigger gets shifted out of the contextual links region and becomes unreachable.

Also, if the right side of the child isn't lined up with the right side of the parent, the child trigger is pushed down unnecessarily.

Steps To Reproduce

To reproduce the behavior:

Create a node with "Raw HTML" format and paste in the HTML from the attached file. This creates several sets of nested contextual links that demonstrate the problem. The text in each div describes what the appropriate child trigger action should be.

Actual behavior

Child triggers for the short (20px) divs are unreachable. When the child has a right margin, its trigger is pushed down unnecessarily.

Expected behavior

Expected trigger position for each example is given by the text of the example.

Additional information

• Backdrop CMS version: 1.20.
• Mentioned in https://github.com/backdrop/backdrop-issues/issues/2626#issuecomment-931743014

contextual_links_demo.html.txt

[klonos]

Intersting! 🤔

[bugfolder]

@klonos points out that this needs to support RTL. PR is updated. New test HTML containing RTL cases is attached.

contextual_links_demo.html.txt

A quick-and-dirty way to test RTL without having to install an RTL language is in theme.inc, find this line:

  $variables['html_attributes']['dir'] = $GLOBALS['language']->direction ? 'rtl' : 'ltr';

And follow it with

$variables['html_attributes']['dir'] = 'rtl'; // DEBUGGING

[klonos]

Thanks @bugfolder 👍🏼 ...I couldn't fully understand what the issue was until I followed the steps to reproduce on a vanilla demo sandbox and the PR sandbox side-by-side 😅

So yes, the PR improves things, but you can get into situations like this, where both contextual menus are open at the same time:

...or like this, where the cog of the closed contextual menu overlaps the open one:

(separate issue perhaps though?)

[bugfolder]

(separate issue perhaps though?)

Yes, I'd suggest that. Both of those scenarios are ugly, but at least they're still usable. Whereas in the original scenario, the child menu isn't even usable since it's not reachable.

(And don't even get me started on what might happen if they're nested 3-deep. Haven't tried it, haven't seen it. But 2-deep does occur.)

[ghost]

Guess it's my fault: https://github.com/backdrop/backdrop-issues/issues/3615

[indigoxela]

@bugfolder many thanks for pointing me to this issue. I tried to test, but realized that the Tugboat demo is already gone. And also, your PR could need a rebase (it's really behind).

I'm not sure, but I have a suspicion that the fix won't work if the child has several contextual links (like a view from multiple nodes). At least, the previous solution (by @BWPanda) suffers from that - for instance in the new promoted cards view.

[bugfolder]

@indigoxela, thanks for taking a look! I've rebased the PR, so there's a new Tugboat demo. I've also created a test page, "Contextual links test", in the Tugboat demo, which you'll see a link for at the top of the page.

I don't see any problems in the new promoted Cards View on the front page, but if you see a place or scenario where it doesn't work, please describe them and I'll strive to update to handle them.

[indigoxela]

@bugfolder great work! The test page is very convenient.

I tested LTR and RTL. And the unnecessary shift of the first child of nested contextual links is also gone. :+1:

[stpaultim]

@bugfolder or @indigoxela - have either of you seen this issue? (Yes, you have) https://github.com/backdrop/backdrop-issues/issues/5595

It seems that my simple fix for that issue, moving the "dismiss" icon to the left might not work in some of these scenarios. OR, is it possible that the conflict described there would not occur in conjunction with nested contextual links.

Can either of you help me comprehend if that issue is related to this in any way and is there a more elegant solution for that issue other than just moving the dismiss icon even further to the left?

I'm happy to fix this issue first and then deal with that one afterwards.

UPDATE: I didn't see all the conversation on that issue today. Both of you are aware and have commented.

[indigoxela]

@stpaultim yes, I saw that other issue (and also commented / brainstormed). :wink: But I think the other issue tries to solve something different. And, no, the fix here will neither solve, nor interfere with the other issue.

[bugfolder]

@indigoxela, @stpaultim, @jenlampton, @klonos, any chance I could get a code review on this?

[jenlampton]

I've left one minor note on the PR (a typo in a comment) but I'm not thrilled about having code in here to both shift the icon to the left/right, and shift it down. If down always works, and would also work for both RTL/LTR, the code would be a lot simpler if that was all we did.

[bugfolder]

Typo fixed.

If down always works

Yeah, but down doesn't always work if the nested items have a small height.

[quicksketch]

I'm sorry I am about to derail this issue, but I recently was on a project where we used the (relatively recent) browser feature for element intersection. See the MDN docs for a full write-up.

But the general capability of the API is that you can specify what happens when any two elements overlap each other, and the browser does all the work of firing an event if two elements overlap.

Browser support is very broad, but even if a browser didn't support it, the worst case scenario would be going back to overlapping contextual links like we have had in the past.

[quicksketch]

Update: Sorry after some more research it looks like Intersection Observer API is not suitable for collision detection like we want. It only applies to parent/child relationships, not to arbitrary elements that might be overlapping.

I do still have a question in the implementation though. See https://github.com/backdrop/backdrop/pull/4300#discussion_r1315327416

[quicksketch]

Another thing I found in testing the PR: This only works if contextual links are overlapping on page load. It doesn't work if links intersect due to the browser window resizing. If the computation is not terribly expensive, it may be better to do this on window resize, wrapped in a Backdrop.debounce().

[bugfolder]

Here's another test case that needs to be covered (the current PR doesn't), which is likely to arise with Views, where the element that collides varies with the screen size (make this the node content, Basis theme, using a Boxton layout).

<style>
  @media (min-width: 768px) {
    .test-child {
      width: 680px;
    }
  }
  @media (min-width: 992px) {
    .test-child {
      width: 460px;
    }
  }
  @media (min-width: 1200px) {
    .test-child {
      width: 370px;
    }
  }
</style>
<div style="margin-bottom: 30px; background-color: #FFD;">
  <div class="contextual-links-region">
    <div class="contextual-links-wrapper">
      <ul class="contextual-links">
        <li><a href="#">Parent link</a></li>
        <li><span class="info">Sample info</span></li>
      </ul>
    </div>
    <div class="contextual-links-region" style="display: inline-block;">
      <div class="contextual-links-wrapper">
        <ul class="contextual-links">
          <li><a href="#">Child link 1</a></li>
          <li><span class="info">Sample info</span></li>
        </ul>
      </div>
      <div class="test-child" style="font-size: 10px; height: 30px; background-color: #DEE;">Height: 30px, first; </div>
    </div>
    <div class="contextual-links-region" style="display: inline-block;">
      <div class="contextual-links-wrapper">
        <ul class="contextual-links">
          <li><a href="#">Child link 2</a></li>
          <li><span class="info">Sample info</span></li>
        </ul>
      </div>
      <div class="test-child" style="font-size: 10px; height: 30px; background-color: #EDE;">Height: 30px, second; </div>
    </div>
    <div class="contextual-links-region" style="display: inline-block;">
      <div class="contextual-links-wrapper">
        <ul class="contextual-links">
          <li><a href="#">Child link 2</a></li>
          <li><span class="info">Sample info</span></li>
        </ul>
      </div>
      <div class="test-child" style="font-size: 10px; height: 30px; background-color: #EED;">Height: 30px, third; </div>
    </div>
  </div>
</div>

It will also be a test case for handling screen resizing.

[bugfolder]

I have provide a new PR in which the JS code for adjusting trigger positions is completely new. Among the changes:

• It handles window resizing
• It handles arbitrarily deeply nested contextual links regions (at least, until you run out of space to hold triggers).

@quicksketch expressed concern over the potential O(n^2) execution. Unfortunately, some O(n^2) is unavoidable, because any given CL trigger could potentially overlap any other CL trigger, so all pairwise combinations need to be considered. However, in the PR, the only stuff that gets executed every time inside the O(n^2) loop is simple arithmetic and branching logic (as opposed to expensive stuff like DOM searching).

I have new test code, which I've attached. Paste this code into any node body (and use Raw HTML as the filter). As before, a quick test for RTL is to use developer tools to edit the <html> tag.

contextual_links_demo_1.html.txt

[quicksketch]

Thanks @bugfolder! I left a few suggestions in the new PR. https://github.com/backdrop/backdrop/pull/4518#pullrequestreview-1629804298

[quicksketch]

The updated PR looks great, thanks @bugfolder!

In testing, I found that Backdrop.optimizedResize() apparently fires on scroll as well as on resize. That doesn't seem like a good idea in this situation. The on scroll behavior was added in https://github.com/backdrop/backdrop-issues/issues/2517, another issue with thorny positioning problems.

Unlike some other usecases like modal dialogs and the autocomplete elements, I think scrolling is pretty unlikely to affect the location of contextual links. So maybe we should use Backdrop.debounce() instead. I left suggestions in the PR.

[bugfolder]

PR is updated with backdrop.debounce().

[quicksketch]

Tested and approved. This looks great @bugfolder! Thanks for working out this difficult problem!

[quicksketch]

Merged into 1.x and 1.26.x. Great work @bugfolder! Thanks @indigoxela, @jenlampton, and @stpaultim for the feedback and testing on this!