If you have a contextual links region inside another contextual links region, you will see an interesting effect: all inner ones are active if you hover the outer region, see screenshot

Based on this it's not possible to hover.... but just enable the first found contextual-links-trigger.
This might be possible with css3 but

a) this would perhaps work not always
b) will definitive not work in ie6/ie7

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
1.25 KB
37.36 KB

Here is a patch.

The patch adds "contextual-links-trigger-active" to the trigger, and change the css which handles the showing.
It works as expected see screenshot

xandeadx’s picture

subscr

marcvangend’s picture

Status: Needs review » Needs work
+++ b/modules/contextual/contextual.js
@@ -25,6 +25,10 @@ Drupal.behaviors.contextualLinks = {
+      $region.hover(
+        function() { $trigger.addClass('contextual-links-trigger-active'); },
+        function() { $trigger.removeClass('contextual-links-trigger-active'); }
+      );

I'm not really familiar with this pattern of using multiple anonymous functions. Just curious: what is the difference compared to this?

$region.hover(function() {
    $trigger.addClass('contextual-links-trigger-active');
    $trigger.removeClass('contextual-links-trigger-active');
});
dawehner’s picture

The first one is executed when you hover, the second when you unhover
Your code does something else ... this pattern is used in contextual.js already.

Update status, because the part was cleared.

marcvangend’s picture

Status: Needs work » Needs review

Thanks, I understand. In fact, it's right there in the jQuery documentation...

Sorry for inadvertently changing the status.

marcvangend’s picture

Patch works as advertised. I'm not marking it RTBC yet because I only tested with Firefox 4 on a linux machine.

dawehner’s picture

Version: 7.x-dev » 8.x-dev

Even this is a bug report it should be against 8.x-dev

Niklas Fiekas’s picture

Trivial D8 reroll.

I can also confirm that the patch solves the issue on Firefox and Chromium.

Niklas Fiekas’s picture

Issue tags: +Needs backport to D7

Tagging.

Bojhan’s picture

sounds fine, but needs further testing I assume

fenda’s picture

If this is further tested and completed, will a bug fix be applied to a future version of d7? Not sure how these things work.

Niklas Fiekas’s picture

Yes, I believe this is backportable.

chrisarusso’s picture

Version: 8.x-dev » 7.x-dev
FileSize
1.25 KB

D7 Patch attached, which means I'm switching version to 7.x-dev. Should I be doing this another way?

fenda’s picture

Status: Needs review » Reviewed & tested by the community

I tested #13 on d7 dev and it fixed the issue for me.

marcvangend’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review

illmasterc, drupaljoe: thanks for the patch and for testing it.

I'm setting this issue back to 8.x & 'needs review' because the core development work flow requires a commit to D8 before this can be backported to D7. I see that both patches are nearly identical, so this shouldn't be a problem.

I think we need someone to test the patch from #8 with the 8.x branch on IE8 and higher (and perhaps some other major browsers) so we can get the D8 patch committed asap.

Niklas Fiekas’s picture

Issue tags: +Needs manual testing

Tagging for manual testing.

chrisarusso’s picture

D8 patch from #8 no longer applies. This one should.

Contextual js is easy enough to port from patch in #8, but the old css has changed significantly. I believe this patch should do the trick, but looking into it further.

What scenario are folks using to test the nested contextual regions in D8? Basically, what's the easiest way to test this in D8?

fenda’s picture

Hmm, hook_node_view and add a node_view() call to its $node->content?
Needs to be 2 separates node I think.

chrisarusso’s picture

Looks like this'll do it. On D8 with attached patch applied.

Tested with custom module in it's entirety below:

<?php

/**
 * Implements hook_test().
 */
function test_node_view($node,$two = 2, $three = 3, $four = 4) {
  if($node->nid != 2){
    $node->content['node2'] = node_view(node_load(2));
  }
}

Then just create two nodes, the second node will be embedded in the body of the first (and so would any other node that's not nid == 2). Visiting the default homepage should give the needed view.

fenda’s picture

Status: Needs review » Reviewed & tested by the community

Looking good. Can we get this into the D7 update coming next week?

chrisarusso’s picture

Somewhat unimportant, but the above code snippet should read:

Implements hook_node_view() not
Implements hook_test().

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -Needs manual testing

Looks fine to me. Committed/pushed to 8.x, moving to 7.x for backport. Tagging as novice for the backport since this should be a straight re-roll.

Niklas Fiekas’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.25 KB

Reuploading #13.

fenda’s picture

I can confirm #23 fixed the bug for me in D7.

markhalliwell’s picture

#23 also fixed it for me.

webchick’s picture

Can you please verify what browsers you're testing in? We can't get this into D7 unless someone confirms in IE 6/7.

peterpoe’s picture

FileSize
164.61 KB
163.59 KB

Works on IE7 (using IETester). Could not test IE6 because some script was dying on the page (not contextual.js though, it died even before applying the patch).

alex.skrypnyk’s picture

Tested in IE 7/8/9 - works perfectly.
IE6 failed for me, but this may be related to my setup.

Someone else, please confirm IE6 and lets move it into core ASAP. Very useful patch.

markhalliwell’s picture

Status: Needs review » Needs work

This issue should probably be re-rolled against D7 dev again, patch no longer applies cleanly.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.25 KB

No real rerole needed, as it have been just some hunks.

Warning: I don't had time to retest the patch manually.

markhalliwell’s picture

Status: Needs review » Reviewed & tested by the community

I can verify that the patch in #30 applies cleanly and fixes the nesting issue in:

The latest Chrome for both Mac and PC, no issues.
The latest FireFox for both Mac and PC, no issues.

Per @webchick in #26:
IE 6 via IETester. Had JS issues too, but was able to get it to work by switching and testing both Garland and Bartick, even though they both still have stylistic issues with the themes themselves (not sure why IE6 is still being considered, but whatever), no issues.
IE 7/8/9 via IETester, no issues.
IE 10 on Win8, no issues.

Can we please get this committed finally?

markhalliwell’s picture

Would like to see this get added to the queue for 7.23.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.23 release notes

Nice improvement, and thanks for all the testing too.

Committed to 7.x! http://drupalcode.org/project/drupal.git/commit/c2a5709

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