Skip to content

Commit 9c50476

Browse files
iOvergaardclaudeCopilot
authored
Tree Navigation: Add visual indicators for items with restricted access (#21365)
* Tree pickers: Implement noAccess property UI handling for user start nodes - Add noAccess observable to document and media tree item contexts - Add visual styling (grayed out, italic) for noAccess items in tree views - Update document and media picker input contexts to prevent selection of noAccess items - Items with noAccess are shown for navigation but cannot be selected in pickers This implements the UI handling for Feature 63060 "Handle Start Nodes" * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Fix noAccess implementation and add E2E tests This commit combines all improvements made to the noAccess property feature: 1. Refactored to use Lit lifecycle methods (updated()) instead of property watchers 2. Added click and keyboard event handlers to prevent navigation 3. Removed disabled attribute that was blocking tree expansion 4. Added comprehensive E2E tests for document and media trees Critical bug fix: Removed disabled attribute that prevented expansion - The disabled attribute was blocking ALL interactions including expanding tree items to show accessible children underneath noAccess ancestors - Now only sets aria-disabled="true" for screen readers and removes href - Click and keyboard event handlers still prevent navigation as intended - Users can now properly navigate through noAccess ancestors to reach their accessible child nodes E2E test coverage: - Display noAccess styling (opacity, italic) - Prevent navigation when clicking noAccess nodes - Allow expansion of noAccess nodes to show children - Picker tests skipped pending infrastructure improvements Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Remove aria-disabled manipulation that interferes with tree expansion The previous implementation set aria-disabled="true" and removed href from the menu-item in #updateMenuItemAccessibility(). This approach caused issues with tree expansion functionality. Removed: - #updateMenuItemAccessibility() method - updated() lifecycle hook that called it - UUIMenuItemElement import (no longer needed) The click and keyboard event handlers already prevent navigation to noAccess nodes, so additional DOM manipulation is not necessary. Test results: ✅ 4 passing: Display styling and prevent navigation work correctly ❌ 2 failing: These appear to be backend issues: 1. Document expansion: Caret button disabled (backend marking noAccess items as not selectable, which disables entire menu-item) 2. Media expansion: Child media folder incorrectly has noAccess attribute (backend data issue - child should be accessible as it's the start node) The UI implementation is sound. The remaining test failures indicate backend API issues that need investigation. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Fix path comparison bug in UserStartNodeEntitiesService (similar to #21162) This fixes the same path comparison bug we fixed in PR #21162 but in C# string comparisons instead of SQL queries. ## Root Cause Path comparisons without trailing commas caused false matches: - Path "-1,1001" incorrectly matched prefix "-1,100" - This marked nodes as ancestors/descendants when they weren't related ## Examples of False Matches - child.Path = "-1,1001", startNodePath = "-1,100" - OLD: "-1,1001".StartsWith("-1,100") = TRUE (bug!) - NEW: "-1,1001,".StartsWith("-1,100,") = FALSE (correct!) - child.Path = "-1,100", startNodePath = "-1,1001" - OLD: "-1,1001".StartsWith("-1,100") = TRUE (bug!) - NEW: "-1,1001,".StartsWith("-1,100,") = FALSE (correct!) ## Fix Applied (Two Locations) 1. Line 146 (ancestor check): Added comma suffix to child.Path 2. Line 226 (IsDescendantOrSelf): Added comma suffix to both paths This matches the pattern already used correctly in lines 92 and 191 of the same file, and mirrors the SQL fix from PR #21162. ## Test Impact This should fix the failing E2E test where child media folders were incorrectly marked as noAccess when they were actually the user's start node. Related: #21162 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Fix remaining merge conflict markers in media-tree-item.element.ts * Remove E2E agent markdown file (moved to personal space) * test: adds mock data for noAccess * feat: moves noAccess subscriber to base class * test: adds mock data for media * feat: moves no-access styling to the base class * fix: media tree items should inherit styling from the base class * feat: observes noAccess from children and reports back to the base class * test: spec file should use undefined instead of null * docs: add comprehensive comments explaining noAccess opt-in pattern - Document why noAccess is not in base interface (breaking change) - Explain opt-in pattern with code examples - Add JSDoc comments to property, event handlers, and CSS - Reference accessibility considerations (keyboard users) - Link child class implementations to base class documentation * test: adds timeout for URL to settle * fix: allow clicks on accessible children of noAccess tree items When a tree item has noAccess, child tree items are rendered in its slot. Previously, the parent's click handler blocked ALL clicks due to event bubbling, preventing users from navigating to accessible descendants. Now checks if click originated from a child tree item element using closest(). If it's a child, allow the click. Only block clicks on the noAccess item itself. Applied to both mouse clicks and keyboard navigation (Enter/Space). This enables users to navigate through noAccess ancestors to reach their accessible start nodes (e.g., Root[noAccess] → Child[noAccess] → Grandchild[accessible]). Fixes tests: - should allow expansion of noAccess ancestor node to show children (documents) - should allow expansion of noAccess ancestor media node to show children (media) * compare with the closest element to see if we are clicking on the element that is blocked or a sub-element that is not * fix: adds forbidden route in case of no variants * test: corrects label locator * test: adds test to check if you can click or deeplink to restricted media * test: removes .only * test: removes duplicated tests * test: adds test for document no-access * test: add unit tests for user start node path comparison logic Adds comprehensive unit tests documenting the path comparison fix that prevents false matches when node IDs are numeric prefixes of other IDs (e.g., 100 vs 1001). The fix uses trailing commas on both paths to ensure accurate comparison: - Without fix: "-1,100".StartsWith("-1,10") = true ❌ (incorrect) - With fix: "-1,100,".StartsWith("-1,10,") = false ✅ (correct) Tests cover: - Numeric prefix edge cases (1 vs 10, 10 vs 100, 100 vs 1001) - Self comparison (start node itself) - Descendant relationships - Deep path hierarchies - Demonstrates the bug without the fix for documentation 19 test cases total, all passing. * test: removes .only * fix: do not overwrite forbidden route * docs: fixes line number in comment * test: fixes comment * feat: uses isSelectableContext to disable and scrub 'href' from base element --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 7dca122 commit 9c50476

File tree

10 files changed

+560
-147
lines changed

10 files changed

+560
-147
lines changed

src/Umbraco.Cms.Api.Management/Services/Entities/UserStartNodeEntitiesService.cs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,8 @@ public IEnumerable<UserAccessEntity> ChildUserAccessEntities(IEnumerable<IEntity
142142
}
143143

144144
// is ancestor of a start node?
145-
if (userStartNodePaths.Any(path => path.StartsWith(child.Path)))
145+
// Note: Add trailing comma to prevent false matches (e.g., path "-1,100" should not match "-1,1001")
146+
if (userStartNodePaths.Any(path => path.StartsWith($"{child.Path},")))
146147
{
147148
return new UserAccessEntity(child, false);
148149
}
@@ -220,5 +221,7 @@ public IEnumerable<UserAccessEntity> UserAccessEntities(IEnumerable<IEntitySlim>
220221
=> entities.Select(entity => new UserAccessEntity(entity, IsDescendantOrSelf(entity, userStartNodePaths))).ToArray();
221222

222223
private static bool IsDescendantOrSelf(IEntitySlim child, string[] userStartNodePaths)
223-
=> userStartNodePaths.Any(path => child.Path.StartsWith(path));
224+
// Note: Add trailing commas to both paths to prevent false matches (e.g., path "-1,100" should not match "-1,1001")
225+
// This matches the pattern used in lines 92 and 192 of this file
226+
=> userStartNodePaths.Any(path => $"{child.Path},".StartsWith($"{path},"));
224227
}

src/Umbraco.Web.UI.Client/src/mocks/data/document/data/permissions-test.data.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,34 @@ const permissionsTestDocument = {
4040

4141
export const data: Array<UmbMockDocumentModel> = [
4242
permissionsTestDocument,
43+
{
44+
...permissionsTestDocument,
45+
ancestors: [{ id: 'permissions-document-id' }],
46+
hasChildren: false,
47+
id: 'permissions-0-document-id',
48+
parent: { id: 'permissions-document-id' },
49+
variants: permissionsTestDocument.variants.map((variant) => ({
50+
...variant,
51+
name: 'Permissions 0 - No Access',
52+
id: 'permissions-0',
53+
})),
54+
flags: [],
55+
noAccess: true,
56+
},
57+
{
58+
...permissionsTestDocument,
59+
ancestors: [{ id: 'permissions-0-document-id' }],
60+
hasChildren: false,
61+
id: 'permissions-0-1-document-id',
62+
parent: { id: 'permissions-0-document-id' },
63+
variants: permissionsTestDocument.variants.map((variant) => ({
64+
...variant,
65+
name: 'Permissions 0.1 - Has access',
66+
id: 'permissions-0-1',
67+
})),
68+
flags: [],
69+
noAccess: false,
70+
},
4371
{
4472
...permissionsTestDocument,
4573
ancestors: [{ id: 'permissions-document-id' }],

src/Umbraco.Web.UI.Client/src/mocks/data/media/media.data.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,43 @@ export const data: Array<UmbMockMediaModel> = [
4444
],
4545
flags: [],
4646
},
47+
{
48+
hasChildren: false,
49+
id: 'f2f81a40-c989-4b6b-84e2-057cecd3grd4',
50+
createDate: '2023-02-06T15:32:05.350038',
51+
parent: null,
52+
noAccess: true,
53+
isTrashed: false,
54+
mediaType: {
55+
id: 'media-type-1-id',
56+
icon: 'icon-picture',
57+
},
58+
values: [
59+
{
60+
editorAlias: 'Umbraco.UploadField',
61+
alias: 'mediaPicker',
62+
value: {
63+
src: '/umbraco/backoffice/assets/installer-illustration.svg',
64+
},
65+
},
66+
{
67+
editorAlias: 'Umbraco.TextBox',
68+
alias: 'mediaType1Property1',
69+
value: 'The daily life at Umbraco HQ',
70+
},
71+
],
72+
variants: [
73+
{
74+
publishDate: '2023-02-06T15:31:51.354764',
75+
culture: null,
76+
segment: null,
77+
name: 'Permissions - No Access',
78+
createDate: '2023-02-06T15:31:46.876902',
79+
updateDate: '2023-02-06T15:31:51.354764',
80+
},
81+
],
82+
flags: [],
83+
},
4784
{
4885
hasChildren: false,
4986
id: '69431027-8867-45bf-a93b-72bbdabfb177',

src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-base/tree-item-element-base.ts

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,29 @@ export abstract class UmbTreeItemElementBase<
3434
}
3535
protected _item?: TreeItemModelType;
3636

37+
/**
38+
* @internal
39+
* Indicates whether the user has no access to this tree item.
40+
* This property is reflected as an attribute for styling purposes.
41+
*
42+
* **Usage Pattern (opt-in):**
43+
* Child classes that support access restrictions should observe their context's `noAccess` observable
44+
* and update this property. The base class provides the property, styling, and interaction prevention,
45+
* but does not subscribe to the observable to avoid forcing all tree item types to implement it.
46+
*
47+
* **Example (in child class api setter):**
48+
* ```typescript
49+
* this.observe(this.#api.noAccess, (noAccess) => (this._noAccess = noAccess));
50+
* ```
51+
*
52+
* **Why not in the base interface?**
53+
* Adding `noAccess` to `UmbTreeItemContext` would be a breaking change, forcing all tree item
54+
* implementations (users, members, data types, etc.) to provide this property even when access
55+
* restrictions don't apply to them.
56+
*/
57+
@property({ type: Boolean, reflect: true, attribute: 'no-access' })
58+
protected _noAccess = false;
59+
3760
/**
3861
* @param item - The item from which to extract flags.
3962
* @description This method is called whenever the `item` property is set. It extracts the flags from the item and assigns them to the `_flags` state property.
@@ -205,15 +228,15 @@ export abstract class UmbTreeItemElementBase<
205228
@selected=${this._handleSelectedItem}
206229
@deselected=${this._handleDeselectedItem}
207230
?active=${this._isActive}
208-
?disabled=${this._isSelectableContext && !this._isSelectable}
231+
?disabled=${(this._isSelectableContext && !this._isSelectable) || this._noAccess}
209232
?selectable=${this._isSelectable}
210233
?selected=${this._isSelected}
211234
.loading=${this._isLoading}
212235
.hasChildren=${this._forceShowExpand || this._hasChildren}
213236
.showChildren=${this._isOpen}
214237
.caretLabel=${this.localize.term(caretLabelKey) + ' ' + this._label}
215238
label=${ifDefined(this._label)}
216-
href=${ifDefined(this._isSelectableContext ? undefined : this._href)}
239+
href=${ifDefined(this._isSelectableContext || this._noAccess ? undefined : this._href)}
217240
.renderExpandSymbol=${this._renderExpandSymbol}>
218241
${this.#renderLoadPrevButton()} ${this.renderIconContainer()} ${this.renderLabel()} ${this.#renderActions()}
219242
${this.#renderChildItems()}

src/Umbraco.Web.UI.Client/src/packages/documents/documents/tree/tree-item/document-tree-item.element.ts

Lines changed: 9 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -11,27 +11,29 @@ export class UmbDocumentTreeItemElement extends UmbTreeItemElementBase<
1111
#api: UmbDocumentTreeItemContext | undefined;
1212

1313
@property({ type: Object, attribute: false })
14-
public override get api(): UmbDocumentTreeItemContext | undefined {
15-
return this.#api;
16-
}
1714
public override set api(value: UmbDocumentTreeItemContext | undefined) {
1815
this.#api = value;
1916

2017
if (this.#api) {
2118
this.observe(this.#api.name, (name) => (this._name = name || ''));
2219
this.observe(this.#api.isDraft, (isDraft) => (this._isDraft = isDraft || false));
23-
this.observe(this.#api.noAccess, (noAccess) => (this._noAccess = noAccess || false));
2420
this.observe(this.#api.hasCollection, (has) => {
2521
const oldValue = this._forceShowExpand;
2622
this._forceShowExpand = has;
2723
this.requestUpdate('_forceShowExpand', oldValue);
2824
});
2925
this.observe(this.#api.icon, (icon) => (this.#icon = icon || ''));
3026
this.observe(this.#api.flags, (flags) => (this._flags = flags || ''));
27+
// Observe noAccess from context and update base class property (_noAccess).
28+
// This enables access restriction behavior (click prevention) and styling from the base class.
29+
this.observe(this.#api.noAccess, (noAccess) => (this._noAccess = noAccess));
3130
}
3231

3332
super.api = value;
3433
}
34+
public override get api(): UmbDocumentTreeItemContext | undefined {
35+
return this.#api;
36+
}
3537

3638
@state()
3739
private _name = '';
@@ -43,27 +45,8 @@ export class UmbDocumentTreeItemElement extends UmbTreeItemElementBase<
4345
@property({ type: Boolean, reflect: true, attribute: 'draft' })
4446
protected _isDraft = false;
4547

46-
/**
47-
* @internal
48-
* Indicates whether the user has no access to this document, this is controlled internally but present as an attribute as it affects styling.
49-
*/
50-
@property({ type: Boolean, reflect: true, attribute: 'no-access' })
51-
protected _noAccess = false;
52-
5348
#icon: string | null | undefined;
5449

55-
constructor() {
56-
super();
57-
this.addEventListener('click', this.#handleClick);
58-
}
59-
60-
#handleClick = (event: MouseEvent) => {
61-
if (this._noAccess) {
62-
event.preventDefault();
63-
event.stopPropagation();
64-
}
65-
};
66-
6750
// eslint-disable-next-line @typescript-eslint/no-unused-vars
6851
protected override _extractFlags(item: UmbDocumentTreeItemModel | undefined) {
6952
// Empty on purpose and NOT calling super to prevent doing what the base does. [NL]
@@ -85,7 +68,9 @@ export class UmbDocumentTreeItemElement extends UmbTreeItemElementBase<
8568
};
8669

8770
override renderLabel() {
88-
return html`<span id="label" slot="label" class=${classMap({ draft: this._isDraft })}>${this._name}</span> `;
71+
return html`<span id="label" slot="label" class=${classMap({ draft: this._isDraft, noAccess: this._noAccess })}>
72+
${this._name}
73+
</span> `;
8974
}
9075

9176
static override styles = [
@@ -97,16 +82,6 @@ export class UmbDocumentTreeItemElement extends UmbTreeItemElementBase<
9782
:host([draft]) umb-icon {
9883
opacity: 0.6;
9984
}
100-
:host([no-access]) {
101-
cursor: not-allowed;
102-
}
103-
:host([no-access]) #label {
104-
opacity: 0.6;
105-
font-style: italic;
106-
}
107-
:host([no-access]) umb-icon {
108-
opacity: 0.6;
109-
}
11085
`,
11186
];
11287
}

src/Umbraco.Web.UI.Client/src/packages/documents/documents/workspace/document-workspace-editor.element.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ export class UmbDocumentWorkspaceEditorElement extends UmbLitElement {
7272
#generateRoutes() {
7373
if (!this.#variants || this.#variants.length === 0 || !this.#appCulture) {
7474
this._routes = [];
75+
this.#ensureForbiddenRoute(this._routes);
7576
return;
7677
}
7778

@@ -143,15 +144,24 @@ export class UmbDocumentWorkspaceEditorElement extends UmbLitElement {
143144
});
144145
}
145146

147+
this.#ensureForbiddenRoute(routes);
148+
149+
this._routes = routes;
150+
}
151+
152+
/**
153+
* Ensure that there is a route to handle forbidden access.
154+
* This route will display a forbidden message when the user does not have permission to access certain resources.
155+
* Also handles not found routes.
156+
*/
157+
#ensureForbiddenRoute(routes: Array<UmbRoute> = []) {
146158
routes.push({
147159
path: '**',
148160
component: async () => {
149161
const router = await import('@umbraco-cms/backoffice/router');
150162
return this.#isForbidden ? router.UmbRouteForbiddenElement : router.UmbRouteNotFoundElement;
151163
},
152164
});
153-
154-
this._routes = routes;
155165
}
156166

157167
private _gotWorkspaceRoute = (e: UmbRouterSlotInitEvent) => {

src/Umbraco.Web.UI.Client/src/packages/media/media/tree/tree-item/media-tree-item.element.ts

Lines changed: 8 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -1,47 +1,18 @@
11
import type { UmbMediaTreeItemModel } from '../types.js';
22
import type { UmbMediaTreeItemContext } from './media-tree-item.context.js';
3-
import { css, html, customElement, nothing, property } from '@umbraco-cms/backoffice/external/lit';
4-
import { UmbTextStyles } from '@umbraco-cms/backoffice/style';
3+
import { css, html, customElement, nothing, classMap } from '@umbraco-cms/backoffice/external/lit';
54
import { UmbTreeItemElementBase } from '@umbraco-cms/backoffice/tree';
65

76
const elementName = 'umb-media-tree-item';
87
@customElement(elementName)
98
export class UmbMediaTreeItemElement extends UmbTreeItemElementBase<UmbMediaTreeItemModel, UmbMediaTreeItemContext> {
10-
#api: UmbMediaTreeItemContext | undefined;
11-
12-
@property({ type: Object, attribute: false })
13-
public override get api(): UmbMediaTreeItemContext | undefined {
14-
return this.#api;
15-
}
169
public override set api(value: UmbMediaTreeItemContext | undefined) {
17-
this.#api = value;
18-
19-
if (this.#api) {
20-
this.observe(this.#api.noAccess, (noAccess) => (this._noAccess = noAccess || false));
21-
}
22-
10+
// Observe noAccess from context and update base class property (_noAccess).
11+
// This enables access restriction behavior (click prevention) and styling from the base class.
12+
this.observe(value?.noAccess, (noAccess) => (this._noAccess = noAccess ?? false));
2313
super.api = value;
2414
}
2515

26-
/**
27-
* @internal
28-
* Indicates whether the user has no access to this media item, this is controlled internally but present as an attribute as it affects styling.
29-
*/
30-
@property({ type: Boolean, reflect: true, attribute: 'no-access' })
31-
protected _noAccess = false;
32-
33-
constructor() {
34-
super();
35-
this.addEventListener('click', this.#handleClick);
36-
}
37-
38-
#handleClick = (event: MouseEvent) => {
39-
if (this._noAccess) {
40-
event.preventDefault();
41-
event.stopPropagation();
42-
}
43-
};
44-
4516
override renderIconContainer() {
4617
const icon = this.item?.mediaType.icon;
4718

@@ -58,7 +29,9 @@ export class UmbMediaTreeItemElement extends UmbTreeItemElementBase<UmbMediaTree
5829
}
5930

6031
override renderLabel() {
61-
return html`<span id="label" slot="label">${this._item?.variants[0].name}</span> `;
32+
return html`<span id="label" slot="label" class=${classMap({ noAccess: this._noAccess })}>
33+
${this._item?.variants[0].name}
34+
</span> `;
6235
}
6336

6437
#renderStateIcon() {
@@ -74,7 +47,7 @@ export class UmbMediaTreeItemElement extends UmbTreeItemElementBase<UmbMediaTree
7447
}
7548

7649
static override styles = [
77-
UmbTextStyles,
50+
...UmbTreeItemElementBase.styles,
7851
css`
7952
#icon-container {
8053
position: relative;
@@ -128,18 +101,6 @@ export class UmbMediaTreeItemElement extends UmbTreeItemElementBase<UmbMediaTree
128101
[disabled] #state-icon {
129102
background-color: var(--uui-color-disabled);
130103
}
131-
132-
/** No Access */
133-
:host([no-access]) {
134-
cursor: not-allowed;
135-
}
136-
:host([no-access]) #label {
137-
opacity: 0.6;
138-
font-style: italic;
139-
}
140-
:host([no-access]) umb-icon {
141-
opacity: 0.6;
142-
}
143104
`,
144105
];
145106
}

0 commit comments

Comments
 (0)