When you visit the 'Help' screen (?q=admin/help) you'll notice that the now-active 'Help' menu item, on the 'Management' menu, assumes an 'expanded' CSS class although it has no children menu items.

Ditto when visiting 'Status report' (?q=admin/reports/status).

The patch fixes this.

CommentFileSizeAuthor
#5 menu-css-test.diff2.1 KBmooffie
#4 menu-css-test.diff2.12 KBmooffie
#3 correct-expanded2.diff1.48 KBmooffie
correct-expanded.diff611 bytesmooffie

Comments

mooffie’s picture

mooffie commented

Explanation for the patch: The existence of $data['below'] does not yet mean that there are child menu-items because these "below" items might all be mere callbacks, which aren't visible in a menu (they have their 'hidden' column set to '-1')).

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Needs work

It's a little bit more complex then that. In Drupal 6, the class was set by theme_menu_item() based on the rendering of the menu below it:

$class = ($menu ? 'expanded' : ($has_children ? 'collapsed' : 'leaf'));

Where $menu is menu_tree_output($data['below']).

We probably want to restore that logic. And we certainly want to write some tests for that.

mooffie’s picture

mooffie commented
Status: Needs work » Needs review
StatusFileSize
new correct-expanded2.diff1.48 KB

In Drupal 6, the class was set by theme_menu_item() based on the rendering of the menu below it:
[...]
We probably want to restore that logic.

I agree, this is more robust; because it also answers for the case where there are normal children but all are invisible (because, for example, the user has no access rights to them).

mooffie’s picture

mooffie commented
StatusFileSize
new menu-css-test.diff2.12 KB

Here's a test.

mooffie’s picture

mooffie commented
StatusFileSize
new menu-css-test.diff2.1 KB

That diff wasn't very good. Here's a corrected diff.

mooffie’s picture

mooffie commented

I agree, this is more robust; because it also answers for the case where there are normal children but all are invisible (because, for example, the user has no access rights to them).

(A correction: the patch doesn't handle cases where the parent has only children to whom the user has no access rights; that is, it doesn't show such a parent as "leaf" (but as "collapsed"). I don't consider this a bug: such a parent isn't always in the active trail and when it isn't we aren't fetching its children --so we have no way of knowing if it's going to have visible children. So I don't see _much_ improvement in showing it as a "leaf" when it's in the active trail and as "collapsed" when it isn't (though some would say it _is_ an improvement).)

Status: Needs review » Needs work

The last submitted patch failed testing.

mooffie’s picture

mooffie commented

The last submitted patch failed testing.

(Sure. The testcase (#5) failed because the patch (#3) wasn't applied. In other words, the failed testcase reveals the bug currently in Drupal.)

mooffie’s picture

mooffie commented
Status: Needs work » Needs review

I forgot to switch this to "needs review".

(I explained in #8 why "System Message"'s "failed testing" actually validated the testcase (#5).)

derjochenmeyer’s picture

derjochenmeyer commented

I didn't find this issue when opening that one #767478: Menu li.leaf items get expanded (li.expanded) when active

It reuses tests #5 written by mooffie.

derjochenmeyer’s picture

derjochenmeyer commented
Status: Needs review » Closed (duplicate)

Marking this as duplicate even though it was opened before #767478: Menu li.leaf items get expanded (li.expanded) when active to join forces.

Please test and review the patch over in the other issue.