Screenshot after logging in as admin user after putting in this patch

Arvinder - I think you're saying that this patch doesn't work for you? Can you post your menu-config.xml, a screenshot before the patch and steps to reproduce your problem. A sample-data.xml might be helpful too so I don't have to manually add new users/roles to the database.

In case it helps, I think I found this problem initially using the icab browser (which has built in html validtion reporting) http://www.icab.de/. It is possible the patch is no good, but I'd guess the attached image instead illustrates broken css (that may have previously been masked by unbalanced (x)html.

Arvinder--can you confirm whether or not the lists are balanced before and/or after the patch?

Matt, I have attached the the before and after (applying the patch) screen shots for two users; admin (abrar/admin) and a normal user (agill/user)
There are no detailed steps involved in recreating this. All you need to do is log in as these two users before and after applying the patch. I have attached menu-config.xml and sample-data.xml.
In nutshell, Menu Organization has two Items as children - Customers and Carriers. User agill has access to only customers and admin, obviously can see both.
If you need anything else from me, please let me know.
Dale, I will take a detailed look at it but on surface the it looks like the lists might not be balanced.

Quite possible the problem introduced was because there was a bug in my patch.
Instead of not closing the list, I didn't close the list item containing the list.
I think the attached patch should produce balanced html. (It just changes a "!=" to an "==".)

It must be Monday.
Ignore my last comment (and new attachment). It doesn't fix anything. It appears that the real source of the problem is that $velocityCount is only valid within a foreach, but not within a macro called from within a foreach. Matt has already submitted an issue at velocity: http://issues.apache.org/jira/browse/VELOCITY-532

Matt--is the simple, short-term answer just downgrading to velocity 1.4?

Dale - if you've upgraded to Velocity 1.5, downgrading might be the solution.

Arvinder - have you upgraded to Velocity 1.5 as well? If not, and you're experiencing this with 1.4, I'll grab your attachment (APF840.zip) and try to reproduce tonight. I plan on fixing this issue or moving to release 2.0.1 tonight.

Indeed, going back to velocity 1.4 makes the original logic (pre-patch) generate balanced html. I'd still suggest the whitespace tweaks from the patch be applied.

Fixed with the following patch:

Index: web/common/src/main/resources/cssHorizontalMenu.vm
===================================================================
--- web/common/src/main/resources/cssHorizontalMenu.vm (revision 2839)
+++ web/common/src/main/resources/cssHorizontalMenu.vm (working copy)
@@ -2,12 +2,11 @@
   #if ($displayer.isAllowed($menu))
     ## set menu title
     #set ($title = $displayer.getMessage($menu.title))
-
     #if (!$menu.url) #set ($url="javascript:void(0)") #else #set ($url=$menu.url) #end
 
     ## create a single menu item
     #if ($menu.components.size() == 0)
- #if ($velocityCount == $menu.parent.components.size())
+ #if ($velocityCount == $renderedChildren)
           <li class="last">
         #else
           <li>
@@ -20,9 +19,11 @@
     #else ## create multiple menu items in a menu
         #if ($menu.components.size() > 0)
             #set ($hasViewableChildren = false)
+ #set ($renderedChildren = 0)
             #foreach ($menuIt in $menu.components)
                 #if ($displayer.isAllowed($menuIt))
                     #set($hasViewableChildren = true)
+ #set($renderedChildren = $renderedChildren + 1)
                 #end
             #end
         #end
@@ -40,10 +41,13 @@
             #displayCssMenu($menuIt)
         #end
         
- #if ($hasViewableChildren && ($velocityCount == $menu.parent.components.size()))
+ #if ($hasViewableChildren && ($velocityCount == $renderedChildren))
+ </li>
+ #else
           </ul>
- #else
+ #if ($velocityCount > $renderedChildren)
           </li>
+ #end
         #end
     #else
       </li>