ZK-5025: redundant selection highlight on a menupopup

zkdoc/release-note

@@ -65,6 +65,7 @@ ZK 10.0.0
   ZK-5540: zk.wpd with browser condition won't work with ZK 10
   ZK-5530: users cannot focus on the icons on the Calendar header
   ZK-5453: XSS in chosenbox
+  ZK-5025: redundant selection highlight on a menupopup
 
 * Upgrade Notes
   + The Java binary-compatible level is Java 11 since ZK 10.

zktest/src/main/webapp/test2/B100-ZK-5025.zul

@@ -0,0 +1,30 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+B100-ZK-5025.zul
+
+        Purpose:
+
+        Description:
+
+        History:
+                Tue Sep 05 11:12:30 CST 2023, Created by jamson
+
+Copyright (C) 2023 Potix Corporation. All Rights Reserved.
+-->
+<zk>
+	<menubar>
+		<menu label="Nested">
+			<menupopup>
+				<menuitem label="F1R1"/>
+				<menuitem label="F1R2"/>
+				<menu label="F1R3">
+					<menupopup>
+						<menuitem label="F2R1"/>
+						<menuitem label="F2R2"/>
+						<menuitem label="F2R3"/>
+					</menupopup>
+				</menu>
+			</menupopup>
+		</menu>
+	</menubar>
+</zk>

zktest/src/main/webapp/test2/config.properties

@@ -3159,6 +3159,7 @@ B90-ZK-4431.zul=A,E,Multislider
 ##zats##B100-ZK-5468-Tree.zul=A,E,Model,Tree
 ##zats##B100-ZK-5235.zul=A,E,Menu,Menupopup,focus
 ##zats##B100-ZK-5453.zul=A,E,Chosenbox,XSS
+##zats##B100-ZK-5025.zul=A,E,Menu,Menupopup,Menuitem,focus,hover
 
 ##
 # Features - 3.0.x version

zktest/src/test/java/org/zkoss/zktest/zats/test2/B100_ZK_5025Test.java

@@ -0,0 +1,64 @@
+/* B100_ZK_5025Test.java
+
+        Purpose:
+
+        Description:
+
+        History:
+                Fri Sep 08 14:22:26 CST 2023, Created by jamson
+
+Copyright (C) 2023 Potix Corporation. All Rights Reserved.
+*/
+package org.zkoss.zktest.zats.test2;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.openqa.selenium.Keys;
+import org.openqa.selenium.interactions.Actions;
+
+import org.zkoss.test.webdriver.WebDriverTestCase;
+import org.zkoss.test.webdriver.ztl.JQuery;
+
+
+public class B100_ZK_5025Test extends WebDriverTestCase {
+	@Test
+	public void test(){
+		Actions actions = new Actions(connect());
+
+		JQuery menu1 = jq(".z-menu"),
+				menu2 = jq("@menu:eq(1)"),
+				menuitem1 = jq("@menuitem:eq(0)"),
+				menuitem2 = jq("@menuitem:eq(1)");
+
+		// open menu1
+		actions.moveToElement(toElement(menu1));
+		click(menu1);
+		Assertions.assertTrue(menuitem1.attr("class").equals("z-menuitem"));
+		Assertions.assertTrue(menuitem2.attr("class").equals("z-menuitem"));
+		Assertions.assertTrue(menu2.attr("class").equals("z-menu"));
+
+		// hover to menuitem1
+		actions.moveToElement(toElement(menuitem1)).build().perform();
+		Assertions.assertTrue(menuitem1.attr("class").equals("z-menuitem z-menuitem-hover")); // highlight should be here only
+		Assertions.assertTrue(menuitem2.attr("class").equals("z-menuitem"));
+		Assertions.assertTrue(menu2.attr("class").equals("z-menu"));
+
+		// keydomn:DOWN to menuitem2
+		actions.keyDown(Keys.DOWN).perform();
+		Assertions.assertTrue(menuitem1.attr("class").equals("z-menuitem"));
+		Assertions.assertTrue(menuitem2.attr("class").equals("z-menuitem z-menuitem-focus")); // highlight should be here only
+		Assertions.assertTrue(menu2.attr("class").equals("z-menu"));
+
+		// keydomn:DOWN to menu2
+		actions.keyDown(Keys.DOWN).perform();
+		Assertions.assertTrue(menuitem1.attr("class").equals("z-menuitem"));
+		Assertions.assertTrue(menuitem2.attr("class").equals("z-menuitem"));
+		Assertions.assertTrue(menu2.attr("class").equals("z-menu z-menu-focus")); // highlight should be here only
+
+		// hover to menuitem2
+		actions.moveToElement(toElement(menuitem2)).build().perform();
+		Assertions.assertTrue(menuitem1.attr("class").equals("z-menuitem"));
+		Assertions.assertTrue(menuitem2.attr("class").equals("z-menuitem z-menuitem-hover")); // highlight should be here only
+		Assertions.assertTrue(menu2.attr("class").equals("z-menu"));
+	}
+}

zktest/src/test/java/org/zkoss/zktest/zats/test2/B96_ZK_4884Test.java

@@ -22,18 +22,20 @@
 public class B96_ZK_4884Test extends WebDriverTestCase {
 	@Test
 	public void test() {
+		// The highlight class names were used to be all `-hover`, after ZK-5025, it's divided into `-hover` (mouse) and `-focus` (keydown)
+
 		connect();
 
 		click(jq("$menu1"));
 		waitResponse();
 		click(jq("$menuitem1"));
 		waitResponse();
-		Assertions.assertEquals(1, jq(".z-menu-hover").length());
+		Assertions.assertEquals(1, jq(".z-menu-focus").length());
 
 		click(jq("$menu2"));
 		waitResponse();
 		click(jq("$menuitem2"));
 		waitResponse();
-		Assertions.assertEquals(1, jq(".z-menu-hover").length());
+		Assertions.assertEquals(1, jq(".z-menu-focus").length());
 	}
 }

zktest/src/test/java/org/zkoss/zktest/zats/test2/B96_ZK_5026Test.java

@@ -27,6 +27,8 @@
 public class B96_ZK_5026Test extends WebDriverTestCase {
 	@Test
 	public void test() {
+		// The highlight class names were used to be all `-hover`, after ZK-5025, it's divided into `-hover` (mouse) and `-focus` (keydown)
+
 		Actions act = new Actions(connect());
 		act.moveToElement(toElement(jq("$menu"))).perform();
 		waitResponse();
@@ -39,10 +41,10 @@ public void test() {
 		assertTrue(jq(" .z-menuitem.z-menuitem-hover").exists());
 
 		act.sendKeys(Keys.DOWN).perform();
-		assertTrue(jq(".z-menupopup-content > .z-menuitem:nth-child(2)").hasClass("z-menuitem-hover"));
+		assertTrue(jq(".z-menupopup-content > .z-menuitem:nth-child(2)").hasClass("z-menuitem-focus"));
 
 		act.sendKeys(Keys.DOWN).perform();
-		assertTrue(jq(".z-menupopup-content > .z-menuitem:nth-child(3)").hasClass("z-menuitem-hover"));
+		assertTrue(jq(".z-menupopup-content > .z-menuitem:nth-child(3)").hasClass("z-menuitem-focus"));
 
 		// error happen if bug exists
 		act.moveToElement(toElement(
@@ -56,11 +58,8 @@ public void test() {
 				.perform();
 
 		assertTrue(jq(" .z-menuitem.z-menuitem-hover").exists());
-		JQuery childMenupopup = jq(".z-menupopup-content").eq(1);
-		act.sendKeys(Keys.DOWN).perform();
-		assertTrue(childMenupopup.children(".z-menuitem:first-child").hasClass("z-menuitem-hover"));
 
-		act.sendKeys(Keys.DOWN).perform();
-		assertTrue(childMenupopup.children(".z-menuitem:nth-child(2)").hasClass("z-menuitem-hover"));
+		// After ZK-5025, if hover or focus is removed from the menupopup, the menupopup will automatically close.
+		// Since the behavior has changed, it's contrary to this test case, so the previous subsequent operations are removed.
 	}
 }

zktest/src/test/java/org/zkoss/zktest/zats/test2/F55_ZK_443Test.java

@@ -24,6 +24,8 @@
 public class F55_ZK_443Test extends WebDriverTestCase {
 	@Test
 	public void test() {
+		// The highlight class names were used to be all `-hover`, after ZK-5025, it's divided into `-hover` (mouse) and `-focus` (keydown)
+
 		connect();
 
 		click(jq("@menu:first"));
@@ -59,7 +61,7 @@ public void test() {
 
 		click(jq("@button:eq(0)"));
 		waitResponse();
-		Assertions.assertTrue(jq("@menu:contains(Project)").hasClass("z-menu-hover"));
+		Assertions.assertTrue(jq("@menu:contains(Project)").hasClass("z-menu-focus"));
 
 		click(jq("@button:eq(1)"));
 		waitResponse();

zul/src/main/resources/web/js/zul/menu/Menu.ts

@@ -372,7 +372,7 @@ export class Menu extends zul.LabelImageWidget implements zul.LabelImageWidgetWi
 					jq(this.$n_()).removeClass(this.$s('hover')).removeClass(this.$s('selected'));
 					pp.close();
 				}
-				(this.$class as typeof Menu)._addActive(this); // keep the focus
+				(this.$class as typeof Menu)._addActive(this, 'focus'); // keep the focus
 				evt.stop();
 				break;
 			case 40: //DOWN
@@ -411,7 +411,7 @@ export class Menu extends zul.LabelImageWidget implements zul.LabelImageWidgetWi
 			if (menubar && menubar._lastTarget)
 				$menu._rmActive(menubar._lastTarget);
 			if (!this._ignoreActive)
-				$menu._addActive(this);
+				$menu._addActive(this, 'focus');
 		}
 		// delete the variable used for IE
 		delete this._ignoreActive;
@@ -553,7 +553,7 @@ export class Menu extends zul.LabelImageWidget implements zul.LabelImageWidgetWi
 				}
 			}
 		}
-		(this.$class as typeof Menu)._addActive(this);
+		(this.$class as typeof Menu)._addActive(this, 'hover');
 	}
 
 	/** @internal */
@@ -598,20 +598,20 @@ export class Menu extends zul.LabelImageWidget implements zul.LabelImageWidgetWi
 	}
 
 	/** @internal */
-	static _isActive(wgt: zul.menu.Menu): boolean {
+	static _isActive(wgt: zul.menu.Menu, type: string): boolean {
 		var top = wgt.isTopmost(),
 			n = wgt.$n_(),
 			menupopup = wgt.menupopup,
-			cls = top ? menupopup && menupopup.isOpen() ? wgt.$s('selected') : wgt.$s('hover') : wgt.$s('hover');
+			cls = top ? menupopup && menupopup.isOpen() ? wgt.$s('selected') : wgt.$s(type) : wgt.$s(type);
 		return jq(n).hasClass(cls);
 	}
 
 	/** @internal */
-	static _addActive(wgt: zul.menu.Menu): void {
+	static _addActive(wgt: zul.menu.Menu, type: string): void {
 		var top = wgt.isTopmost(),
 			n = wgt.$n_(),
 			menupopup = wgt.menupopup,
-			cls = top ? menupopup && menupopup.isOpen() ? wgt.$s('selected') : wgt.$s('hover') : wgt.$s('hover');
+			cls = top ? menupopup && menupopup.isOpen() ? wgt.$s('selected') : wgt.$s(type) : wgt.$s(type);
 		jq(n).addClass(cls);
 		if (top) {
 			var mb = wgt.getMenubar();
@@ -621,7 +621,7 @@ export class Menu extends zul.LabelImageWidget implements zul.LabelImageWidgetWi
 			const parentMenupopup = wgt.parent;
 			parentMenupopup.addActive_(wgt);
 			if (parentMenupopup.parent instanceof zul.menu.Menu)
-				this._addActive(parentMenupopup.parent);
+				this._addActive(parentMenupopup.parent, type);
 		}
 	}
 
@@ -630,9 +630,8 @@ export class Menu extends zul.LabelImageWidget implements zul.LabelImageWidgetWi
 		var top = wgt.isTopmost(),
 			n = wgt.$n_(),
 			menupopup = wgt.menupopup,
-			cls = top ? (!ignoreSeld && menupopup && menupopup.isOpen()) ? wgt.$s('selected') : wgt.$s('hover') : wgt.$s('hover'),
-			anode = jq(n).removeClass(cls);
-		if (top && !(anode.hasClass(wgt.$s('selected')) || anode.hasClass(wgt.$s('hover'))))
+			anode = (top && !ignoreSeld && menupopup && menupopup.isOpen()) ? jq(n).removeClass(wgt.$s('selected')) : jq(n).removeClass(wgt.$s('hover')).removeClass(wgt.$s('focus'));
+		if (top && !(anode.hasClass(wgt.$s('selected')) || (anode.hasClass(wgt.$s('hover')) || anode.hasClass(wgt.$s('focus')))))
 			_toggleClickableCSS(wgt, true);
 	}
 

zul/src/main/resources/web/js/zul/menu/Menuitem.ts

@@ -533,9 +533,10 @@ export class Menuitem extends zul.LabelImageWidget implements zul.LabelImageWidg
 		if (zul.menu._nOpen || isTopmost)
 			zWatch.fire('onFloatUp', this); //notify all
 		if (!isTopmost && !this._disabled) {
-			if (this.parent)
-				this.parent.removeActive_();
-			(this.$class as typeof Menuitem)._addActive(this);
+			if (this.parent instanceof zul.menu.Menupopup)
+				this.parent.removeAllChildrenActive_();
+			(this.$class as typeof Menuitem)._addActive(this, 'hover');
+			this.focus();
 		}
 	}
 
@@ -582,20 +583,20 @@ export class Menuitem extends zul.LabelImageWidget implements zul.LabelImageWidg
 	}
 
 	/** @internal */
-	static _addActive(wgt: zul.menu.Menu | zul.menu.Menuitem): void {
+	static _addActive(wgt: zul.menu.Menu | zul.menu.Menuitem, type: string): void {
 		var top = wgt.isTopmost();
-		jq(wgt.$n_()).addClass(wgt.$s('hover'));
+		jq(wgt.$n_()).addClass(wgt.$s(type));
 		if (!top && wgt.parent instanceof zul.menu.Menupopup) {
 			const parentMenupopup = wgt.parent;
 			parentMenupopup.addActive_(wgt);
 			if (parentMenupopup.parent instanceof zul.menu.Menu)
-				this._addActive(parentMenupopup.parent);
+				this._addActive(parentMenupopup.parent, type);
 		}
 	}
 
 	/** @internal */
 	static _rmActive(wgt: zk.Widget): JQuery {
-		return jq(wgt.$n_()).removeClass(wgt.$s('hover'));
+		return jq(wgt.$n_()).removeClass(wgt.$s('hover')).removeClass(wgt.$s('focus'));
 	}
 
 	onShow(): void {

zul/src/main/resources/web/js/zul/menu/Menupopup.ts

@@ -81,7 +81,7 @@ function _activateNextMenu(menu: zul.menu.Menu): void {
 			pp.open(undefined, undefined, undefined, {focusFirst: true, sendOnOpen: true, disableMask: true});
 		}
 	}
-	(menu.$class as typeof zul.menu.Menu)._addActive(menu);
+	(menu.$class as typeof zul.menu.Menu)._addActive(menu, 'focus');
 	zWatch.fire('onFloatUp', menu); //notify all
 }
 
@@ -265,7 +265,7 @@ export class Menupopup extends zul.wgt.Popup {
 			return;
 
 		var org = ctl.origin;
-		if (this.parent!.menupopup == this && !this.parent!.isTopmost() && !(this.parent!.$class as typeof zul.menu.Menu)._isActive(this.parent!)) {
+		if (this.parent!.menupopup == this && !this.parent!.isTopmost() && !(this.parent!.$class as typeof zul.menu.Menu)._isActive(this.parent!, 'hover')) {
 			this.close({sendOnOpen: true});
 			return;
 		}
@@ -366,9 +366,12 @@ export class Menupopup extends zul.wgt.Popup {
 		case 40: //DOWN
 			// UP: 1. jump to the previousSibling item
 			// DOWN: 1. jump to the nextSibling item
-			if (w) (w.$class as typeof zul.menu.Menuitem)._rmActive(w);
+			this.removeAllChildrenActive_();
 			w = keyCode == 38 ? _prevChild(this, w) : _nextChild(this, w);
-			if (w) (w.$class as typeof zul.menu.Menuitem)._addActive(w as zul.menu.Menuitem); // FIXME: type of w is inconsistent
+			if (w) {
+				(w.$class as typeof zul.menu.Menuitem)._addActive(w as zul.menu.Menuitem, 'focus'); // FIXME: type of w is inconsistent
+				w.focus();
+			}
 			break;
 		case 37: //LEFT
 			// 1. close the contenthandler (like colorbox), if any
@@ -379,7 +382,7 @@ export class Menupopup extends zul.wgt.Popup {
 				w._contentHandler.onHide();
 			} else if (((menu = _getMenu(this))) && !menu.isTopmost()) {
 				this.close();
-				(menu.$class as typeof zul.menu.Menu)._addActive(menu);
+				(menu.$class as typeof zul.menu.Menu)._addActive(menu, 'focus');
 				const pp = menu.parent as unknown as zul.menu.Menupopup | undefined; // FIXME: type of pp is inconsistent
 				if (pp) {
 					pp.focus();
@@ -424,7 +427,7 @@ export class Menupopup extends zul.wgt.Popup {
 					if (menu.isTopmost()) {
 						menu.focus();
 					} else {
-						(menu.$class as typeof zul.menu.Menu)._addActive(menu);
+						(menu.$class as typeof zul.menu.Menu)._addActive(menu, 'focus');
 						const pp = menu.parent;
 						if (pp) {
 							pp.focus();
@@ -446,7 +449,7 @@ export class Menupopup extends zul.wgt.Popup {
 						content.onHide();
 					} else {
 						this.close();
-						(menu.$class as typeof zul.menu.Menu)._addActive(menu);
+						(menu.$class as typeof zul.menu.Menu)._addActive(menu, 'focus');
 						const pp = menu.parent;
 						if (pp) {
 							pp.focus();
@@ -542,7 +545,7 @@ export class Menupopup extends zul.wgt.Popup {
 
 				this._curIndex = newCurrIndex;
 				var target = this.getChildAt<zul.menu.Menuitem>(childIndex);
-				if (target) (target.$class as typeof zul.menu.Menuitem)._addActive(target);
+				if (target) (target.$class as typeof zul.menu.Menuitem)._addActive(target, 'hover');
 			}
 		}
 		return this;
@@ -576,6 +579,22 @@ export class Menupopup extends zul.wgt.Popup {
 		}
 	}
 
+	/** @internal */
+	removeAllChildrenActive_(): void {
+		for (let child: zk.Widget | undefined = this?.firstChild; child != undefined; child = child.nextSibling) {
+			if (child instanceof zul.menu.Menuitem) {
+				(child.$class as typeof zul.menu.Menuitem)._rmActive(child);
+			} else if (child instanceof zul.menu.Menu) {
+				(child.$class as typeof zul.menu.Menu)._rmActive(child);
+				const pp = child.menupopup;
+				if (pp?.isOpen()) {
+					jq(child.$n_()).removeClass(child.$s('hover')).removeClass(child.$s('selected'));
+					pp.close();
+				}
+			}
+		}
+	}
+
 	/** @internal */
 	_currentChild(): zul.menu.Menuitem | zul.menu.Menu | undefined {
 		const index = this._curIndex,