Skip to content

Commit

Permalink
ZK-5025: redundant selection highlight on a menupopup
Browse files Browse the repository at this point in the history
  • Loading branch information
JamsonChan committed Sep 11, 2023
1 parent 1d12dfb commit 940b82b
Show file tree
Hide file tree
Showing 8 changed files with 155 additions and 31 deletions.
1 change: 1 addition & 0 deletions zkdoc/release-note
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ ZK 10.0.0
ZK-5539: $init() will call twice on Keikai component
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-5025: redundant selection highlight on a menupopup

* Upgrade Notes
+ The Java binary-compatible level is Java 11 since ZK 10.
Expand Down
30 changes: 30 additions & 0 deletions zktest/src/main/webapp/test2/B100-ZK-5025.zul
Original file line number Diff line number Diff line change
@@ -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>
1 change: 1 addition & 0 deletions zktest/src/main/webapp/test2/config.properties
Original file line number Diff line number Diff line change
Expand Up @@ -3158,6 +3158,7 @@ B90-ZK-4431.zul=A,E,Multislider
##zats##B100-ZK-5468-Tabbox.zul=A,E,Model,Tabbox
##zats##B100-ZK-5468-Tree.zul=A,E,Model,Tree
##zats##B100-ZK-5235.zul=A,E,Menu,Menupopup,focus
##zats##B100-ZK-5025.zul=A,E,Menu,Menupopup,Menuitem,focus,hover

##
# Features - 3.0.x version
Expand Down
Original file line number Diff line number Diff line change
@@ -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"));
}
}
21 changes: 10 additions & 11 deletions zul/src/main/resources/web/js/zul/menu/Menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 */
Expand Down Expand Up @@ -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();
Expand All @@ -627,7 +627,7 @@ export class Menu extends zul.LabelImageWidget implements zul.LabelImageWidgetWi
if (parentMenupopup)
parentMenupopup.addActive_(wgt);
if (parentMenupopup.parent instanceof zul.menu.Menu)
this._addActive(parentMenupopup.parent);
this._addActive(parentMenupopup.parent, type);
}
}

Expand All @@ -636,9 +636,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);
}

Expand Down
26 changes: 21 additions & 5 deletions zul/src/main/resources/web/js/zul/menu/Menuitem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,23 @@ export class Menuitem extends zul.LabelImageWidget implements zul.LabelImageWidg
if (!isTopmost && !this._disabled) {
if (this.parent)
this.parent.removeActive_();
(this.$class as typeof Menuitem)._addActive(this);
// remove all active
for (var mi = this.parent?.firstChild; mi != undefined; mi = mi.nextSibling) {
if (mi instanceof Menuitem) {
(mi.$class as typeof Menuitem)._rmActive(mi);
}
if (mi instanceof zul.menu.Menu) {
(mi.$class as typeof zul.menu.Menu)._rmActive(mi);
var pp = mi.menupopup;
if (pp?.isOpen()) {
jq(mi.$n_()).removeClass(mi.$s('hover')).removeClass(mi.$s('selected'));
pp.close();
}
}
}
// add new active
(this.$class as typeof Menuitem)._addActive(this, 'hover');
this.focus();
}
}

Expand Down Expand Up @@ -582,9 +598,9 @@ export class Menuitem extends zul.LabelImageWidget implements zul.LabelImageWidg
}

/** @internal */
static _addActive(wgt: zul.menu.Menuitem): void {
static _addActive(wgt: 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) {
// FIXME: See `zul.menu.Menu.prototype._addActive`, but there is an
// additional point here.
Expand All @@ -594,13 +610,13 @@ export class Menuitem extends zul.LabelImageWidget implements zul.LabelImageWidg
if (parentMenupopup)
parentMenupopup.addActive_(wgt);
if (parentMenupopup!.parent instanceof zul.menu.Menu)
this._addActive(parentMenupopup!.parent as unknown as zul.menu.Menuitem);
this._addActive(parentMenupopup!.parent as unknown as zul.menu.Menuitem, 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 {
Expand Down
32 changes: 25 additions & 7 deletions zul/src/main/resources/web/js/zul/menu/Menupopup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -367,8 +367,26 @@ export class Menupopup extends zul.wgt.Popup {
// 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);
// remove all active
for (var mi = this?.firstChild; mi != undefined; mi = mi.nextSibling) {
if (mi instanceof zul.menu.Menuitem) {
(mi.$class as typeof zul.menu.Menuitem)._rmActive(mi);
}
if (mi instanceof zul.menu.Menu) {
(mi.$class as typeof zul.menu.Menu)._rmActive(mi);
var pp = mi.menupopup;
if (pp?.isOpen()) {
jq(mi.$n_()).removeClass(mi.$s('hover')).removeClass(mi.$s('selected'));
pp.close();
}
}
}
// add new active
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
Expand All @@ -379,7 +397,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();
Expand Down Expand Up @@ -424,7 +442,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();
Expand All @@ -446,7 +464,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();
Expand Down Expand Up @@ -542,7 +560,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;
Expand Down
11 changes: 3 additions & 8 deletions zul/src/main/resources/web/js/zul/menu/less/menu.less
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@
white-space: nowrap;
min-height: @menuContentMinHeight;
z-index: 20; // the 20 is greater than menupopup-separator's z-index

&:hover {
.contentStyle(@menuItemHoverColor, @menuItemHoverBackground);
}
Expand Down Expand Up @@ -216,12 +215,6 @@
.z-menuitem-content {
color: @menuPopupItemColor;
background: @menuPopupItemBackground;
&:hover {
.contentStyle(@menuPopupItemHoverColor, @menuPopupItemHoverBackground);
}
&:focus {
.contentStyle(@menuPopupItemHoverColor, @menuPopupItemHoverBackground);
}
&:active {
.contentStyle(@menuPopupItemActiveColor, @menuPopupItemActiveBackground);
}
Expand Down Expand Up @@ -263,7 +256,9 @@
line-height: normal;
}
.z-menu-hover > .z-menu-content,
.z-menuitem-hover > .z-menuitem-content {
.z-menu-focus > .z-menu-content,
.z-menuitem-hover > .z-menuitem-content,
.z-menuitem-focus > .z-menuitem-content {
.contentStyle(@menuPopupItemHoverColor, @menuPopupItemHoverBackground);
}
}
Expand Down

0 comments on commit 940b82b

Please sign in to comment.