Skip to content

Commit e4b2f0a

Browse files
authored
fix: Make MenuItem methods toggle classes immediately (#9570)
* fix: Make `MenuItem` methods toggle classes immediately * chore: Add docstring * fix: Clarify name of `toggleHasCheckbox()` * fix: Ensure menu items are enabled before highlighting them
1 parent 7411675 commit e4b2f0a

File tree

3 files changed

+235
-15
lines changed

3 files changed

+235
-15
lines changed

core/menuitem.ts

Lines changed: 58 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
// Former goog.module ID: Blockly.MenuItem
1313

1414
import * as aria from './utils/aria.js';
15-
import * as dom from './utils/dom.js';
1615
import * as idGenerator from './utils/idgenerator.js';
1716

1817
/**
@@ -74,12 +73,6 @@ export class MenuItem {
7473

7574
const content = document.createElement('div');
7675
content.className = 'blocklyMenuItemContent';
77-
// Add a checkbox for checkable menu items.
78-
if (this.checkable) {
79-
const checkbox = document.createElement('div');
80-
checkbox.className = 'blocklyMenuItemCheckbox ';
81-
content.appendChild(checkbox);
82-
}
8376

8477
let contentDom: Node = this.content as HTMLElement;
8578
if (typeof this.content === 'string') {
@@ -88,6 +81,11 @@ export class MenuItem {
8881
content.appendChild(contentDom);
8982
element.appendChild(content);
9083

84+
// Add a checkbox for checkable menu items.
85+
if (this.checkable) {
86+
this.toggleHasCheckbox(true);
87+
}
88+
9189
// Initialize ARIA role and state.
9290
if (this.roleName) {
9391
aria.setRole(element, this.roleName);
@@ -145,6 +143,7 @@ export class MenuItem {
145143
*/
146144
setRightToLeft(rtl: boolean) {
147145
this.rightToLeft = rtl;
146+
this.getElement()?.classList.toggle('blocklyMenuItemRtl', this.rightToLeft);
148147
}
149148

150149
/**
@@ -166,6 +165,12 @@ export class MenuItem {
166165
*/
167166
setCheckable(checkable: boolean) {
168167
this.checkable = checkable;
168+
169+
if (!this.checkable) {
170+
this.setChecked(false);
171+
}
172+
173+
this.toggleHasCheckbox(checkable);
169174
}
170175

171176
/**
@@ -175,7 +180,14 @@ export class MenuItem {
175180
* @internal
176181
*/
177182
setChecked(checked: boolean) {
183+
if (checked && !this.checkable) return;
184+
178185
this.checked = checked;
186+
const element = this.getElement();
187+
if (element) {
188+
element.classList.toggle('blocklyMenuItemSelected', this.checked);
189+
aria.setState(element, aria.State.SELECTED, this.checked);
190+
}
179191
}
180192

181193
/**
@@ -186,14 +198,11 @@ export class MenuItem {
186198
*/
187199
setHighlighted(highlight: boolean) {
188200
this.highlight = highlight;
189-
const el = this.getElement();
190-
if (el && this.isEnabled()) {
191-
const name = 'blocklyMenuItemHighlight';
192-
if (highlight) {
193-
dom.addClass(el, name);
194-
} else {
195-
dom.removeClass(el, name);
196-
}
201+
if (this.isEnabled()) {
202+
this.getElement()?.classList.toggle(
203+
'blocklyMenuItemHighlight',
204+
this.highlight,
205+
);
197206
}
198207
}
199208

@@ -215,6 +224,11 @@ export class MenuItem {
215224
*/
216225
setEnabled(enabled: boolean) {
217226
this.enabled = enabled;
227+
const element = this.getElement();
228+
if (element) {
229+
element.classList.toggle('blocklyMenuItemDisabled', !this.enabled);
230+
aria.setState(element, aria.State.DISABLED, !this.enabled);
231+
}
218232
}
219233

220234
/**
@@ -243,4 +257,33 @@ export class MenuItem {
243257
onAction(fn: (p1: MenuItem, menuSelectEvent: Event) => void, obj: object) {
244258
this.actionHandler = fn.bind(obj);
245259
}
260+
261+
/**
262+
* Adds or removes the checkmark indicator on this menu item.
263+
* The indicator is present even if this menu item is not checked, as long
264+
* as it is checkable; its visibility is controlled with CSS.
265+
*
266+
* @param add True to add the checkmark indicator, false to remove it.
267+
*/
268+
private toggleHasCheckbox(add: boolean) {
269+
if (add) {
270+
if (
271+
this.getElement()?.querySelector(
272+
'.blocklyMenuItemContent .blocklyMenuItemCheckbox',
273+
)
274+
) {
275+
return;
276+
}
277+
278+
const checkbox = document.createElement('div');
279+
checkbox.className = 'blocklyMenuItemCheckbox ';
280+
this.getElement()
281+
?.querySelector('.blocklyMenuItemContent')
282+
?.prepend(checkbox);
283+
} else {
284+
this.getElement()
285+
?.querySelector('.blocklyMenuItemContent .blocklyMenuItemCheckbox')
286+
?.remove();
287+
}
288+
}
246289
}

tests/mocha/index.html

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,7 @@
224224
import './blocks/lists_test.js';
225225
import './blocks/logic_ternary_test.js';
226226
import './blocks/loops_test.js';
227+
import './menu_item_test.js';
227228
import './metrics_test.js';
228229
import './mutator_test.js';
229230
import './names_test.js';

tests/mocha/menu_item_test.js

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
/**
2+
* @license
3+
* Copyright 2026 Raspberry Pi Foundation
4+
* SPDX-License-Identifier: Apache-2.0
5+
*/
6+
7+
import {assert} from '../../node_modules/chai/index.js';
8+
import {
9+
sharedTestSetup,
10+
sharedTestTeardown,
11+
} from './test_helpers/setup_teardown.js';
12+
13+
suite('Menu items', function () {
14+
setup(function () {
15+
sharedTestSetup.call(this);
16+
this.menuItem = new Blockly.MenuItem('Hello World');
17+
this.menuItem.createDom();
18+
});
19+
20+
teardown(function () {
21+
sharedTestTeardown.call(this);
22+
});
23+
24+
test('can be RTL', function () {
25+
this.menuItem.setRightToLeft(true);
26+
assert.isTrue(
27+
this.menuItem.getElement().classList.contains('blocklyMenuItemRtl'),
28+
);
29+
});
30+
31+
test('can be LTR', function () {
32+
this.menuItem.setRightToLeft(false);
33+
assert.isFalse(
34+
this.menuItem.getElement().classList.contains('blocklyMenuItemRtl'),
35+
);
36+
});
37+
38+
test('can be checked', function () {
39+
this.menuItem.setCheckable(true);
40+
this.menuItem.setChecked(true);
41+
assert.isTrue(
42+
this.menuItem.getElement().classList.contains('blocklyMenuItemSelected'),
43+
);
44+
assert.equal(
45+
this.menuItem.getElement().getAttribute('aria-selected'),
46+
'true',
47+
);
48+
});
49+
50+
test('cannot be checked when designated as uncheckable', function () {
51+
this.menuItem.setCheckable(false);
52+
this.menuItem.setChecked(true);
53+
assert.isFalse(
54+
this.menuItem.getElement().classList.contains('blocklyMenuItemSelected'),
55+
);
56+
assert.equal(
57+
this.menuItem.getElement().getAttribute('aria-selected'),
58+
'false',
59+
);
60+
});
61+
62+
test('can be unchecked', function () {
63+
this.menuItem.setCheckable(true);
64+
this.menuItem.setChecked(false);
65+
assert.isFalse(
66+
this.menuItem.getElement().classList.contains('blocklyMenuItemSelected'),
67+
);
68+
assert.equal(
69+
this.menuItem.getElement().getAttribute('aria-selected'),
70+
'false',
71+
);
72+
});
73+
74+
test('uncheck themselves when designated as non-checkable', function () {
75+
this.menuItem.setChecked(true);
76+
this.menuItem.setCheckable(false);
77+
78+
assert.isFalse(
79+
this.menuItem.getElement().classList.contains('blocklyMenuItemSelected'),
80+
);
81+
assert.equal(
82+
this.menuItem.getElement().getAttribute('aria-selected'),
83+
'false',
84+
);
85+
});
86+
87+
test('do not check themselves when designated as checkable', function () {
88+
this.menuItem.setChecked(false);
89+
this.menuItem.setCheckable(true);
90+
91+
assert.isFalse(
92+
this.menuItem.getElement().classList.contains('blocklyMenuItemSelected'),
93+
);
94+
assert.equal(
95+
this.menuItem.getElement().getAttribute('aria-selected'),
96+
'false',
97+
);
98+
});
99+
100+
test('adds a checkbox when designated as checkable', function () {
101+
assert.isNull(
102+
this.menuItem.getElement().querySelector('.blocklyMenuItemCheckbox'),
103+
);
104+
this.menuItem.setCheckable(true);
105+
assert.isNotNull(
106+
this.menuItem.getElement().querySelector('.blocklyMenuItemCheckbox'),
107+
);
108+
});
109+
110+
test('removes the checkbox when designated as uncheckable', function () {
111+
this.menuItem.setCheckable(true);
112+
assert.isNotNull(
113+
this.menuItem.getElement().querySelector('.blocklyMenuItemCheckbox'),
114+
);
115+
this.menuItem.setCheckable(false);
116+
assert.isNull(
117+
this.menuItem.getElement().querySelector('.blocklyMenuItemCheckbox'),
118+
);
119+
});
120+
121+
test('can be highlighted', function () {
122+
this.menuItem.setHighlighted(true);
123+
assert.isTrue(
124+
this.menuItem.getElement().classList.contains('blocklyMenuItemHighlight'),
125+
);
126+
});
127+
128+
test('can be unhighlighted', function () {
129+
this.menuItem.setHighlighted(false);
130+
assert.isFalse(
131+
this.menuItem.getElement().classList.contains('blocklyMenuItemHighlight'),
132+
);
133+
});
134+
135+
test('cannot be highlighted if not enabled', function () {
136+
this.menuItem.setEnabled(false);
137+
this.menuItem.setHighlighted(true);
138+
assert.isFalse(
139+
this.menuItem.getElement().classList.contains('blocklyMenuItemHighlight'),
140+
);
141+
});
142+
143+
test('can be enabled', function () {
144+
this.menuItem.setEnabled(true);
145+
assert.isTrue(this.menuItem.isEnabled());
146+
assert.isFalse(
147+
this.menuItem.getElement().classList.contains('blocklyMenuItemDisabled'),
148+
);
149+
assert.equal(
150+
this.menuItem.getElement().getAttribute('aria-disabled'),
151+
'false',
152+
);
153+
});
154+
155+
test('can be disabled', function () {
156+
this.menuItem.setEnabled(false);
157+
assert.isFalse(this.menuItem.isEnabled());
158+
assert.isTrue(
159+
this.menuItem.getElement().classList.contains('blocklyMenuItemDisabled'),
160+
);
161+
assert.equal(
162+
this.menuItem.getElement().getAttribute('aria-disabled'),
163+
'true',
164+
);
165+
});
166+
167+
test('invokes its action callback', function () {
168+
let called = false;
169+
const callback = () => {
170+
called = true;
171+
};
172+
this.menuItem.onAction(callback, this);
173+
this.menuItem.performAction(new Event('click'));
174+
assert.isTrue(called);
175+
});
176+
});

0 commit comments

Comments
 (0)