Menu li.leaf items get expanded (li.expanded) when active

Not critical. Please read and understand Priority levels of Issues, thanks.

+++ includes/menu.inc
@@ -915,7 +915,7 @@
+    if ($data['below']  && $data['link']['has_children']) {

Duplicate space here.

Also, can we flip the conditions, please? The second is the primary, actually.

Powered by Dreditor.

At first, I wasn't sure whether this really needs to be fixed in menu_tree_output() or not rather in one of the tree builder functions - but changing those would potentially break other use-cases.

To get this in, we want to put a short comment before this condition to clarify that 'below' may contain local tasks, which are not output in regular menu link trees.

Furthermore, I'd also like to see tests to ensure we don't break this logic/functionality again in the future.

+++ includes/menu.inc
@@ -915,7 +915,9 @@
     // Set a class if the link has children.
-    if ($data['below']) {
+    // Since $data['below'] may contain local tasks,
+    // only set 'expanded' class if the link also has children.
+    if ($data['link']['has_children'] && $data['below']) {

1) The new comment wraps too early.

2) We can combine/rephrase the old with the new comment.

+++ modules/simpletest/tests/menu.test
@@ -390,6 +390,35 @@
 
+class MenuCSSTestCase extends DrupalWebTestCase {
...
+
+  function testParentIsLeaf() {

Missing class/function docblocks.

+++ modules/simpletest/tests/menu.test
@@ -390,6 +390,35 @@
+  function get_li($text) {
+    $elements = $this->xpath("//ul[@class='menu']/li[a[text()='$text']]");
+    return $elements ? $elements[0] : NULL;

Do we really need this helper function?

+++ modules/simpletest/tests/menu_test.module
@@ -172,7 +172,19 @@
+  ¶

Trailing white-space here.

+++ modules/simpletest/tests/menu_test.module
@@ -172,7 +172,19 @@
+  $items['menu-test/css/parent'] = array(
+    'title' => 'Test CSS parent #1',
...
+  $items['menu-test/css/parent/child-link-not-shown-because-callback'] = array(
+    'title' => 'Test router item',

1) That second path looks weird.

2) The menu item titles should summarize what's being tested.

+++ modules/simpletest/tests/menu_test.module
@@ -172,7 +172,19 @@
+    'type' => MENU_CALLBACK,

Shouldn't we test local tasks?

107 critical left. Go review some!

Patch still applies with a bit of offset, and works to fix this problem, and the code looks good, so this is rtbc. Nice work!

Committed to CVS HEAD. Thanks.

Commit http://drupal.org/cvs?commit=395108 missed hunks with tests

Also, for some reason, this patch does not appear working for paths like admin/structure/types. Not sure if that's a side effect of not having the whole thing committed or if it's just a fault of the patch.

Confirm that admin/structure/block and admin/structure/types are displayed like holding children

I'd recommend to complete the commit first, because that work is valuable on its own.

I do not think we should commit this test until we figure out why it is not catching the bug.

hm, true, too.

I just ran into this issue and posted here first http://drupal.org/node/955378 (Menu items - "Has children" flag is not reset on menu reordering). Tried that patch and it didn't seem to help. Seems like this isn't the only thread of this issue. Going to try this one as well.

edit: sorry didn't realize this was for D7

Still running into this in 7.7.

do we have the issue in d8?

Do we still need this issue? Seems committed to D8 7 years back.