henningn commented on code in PR #7743:
URL: https://github.com/apache/myfaces-tobago/pull/7743#discussion_r4295649780
##########
tobago-theme/tobago-theme-standard/src/main/ts/tobago-tree.ts:
##########
@@ -85,6 +104,169 @@ export class Tree extends HTMLElement {
get selectable(): Selectable {
return Selectable[this.getAttribute("selectable")] as Selectable;
}
+
+ get nodes(): NodeListOf<TreeNode> {
+ return this.querySelectorAll("tobago-tree-node");
+ }
+
+ get visibleNodes(): TreeNode[] {
+ return Array.from(this.nodes).filter(node => (node as
HTMLElement).offsetParent !== null) as TreeNode[];
Review Comment:
offsetParent hat doch nichts mit der Sichtbarkeit zu tun? Man kann aber auf
"d-none" prüfen.
!(node as HTMLElement).classList.contains(".d-none")
##########
tobago-theme/tobago-theme-standard/src/main/ts/tobago-tree-static.ts:
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export class TreeStatic {
+
+ /**
+ * Bestimmt den Tabindex-Wert für einen Tree-Node-Container.
+ * Tree-Nodes selbst sollten nicht im Tab-Fokus liegen (tabindex="-1"),
+ * nur die Sub-Elemente (Toggle, Checkbox, Radio).
+ * @returns -1 für den Node-Container
+ */
+ static getTabindexForNode(): number {
Review Comment:
Diese Funktion wie nicht benutzt. Aber selbst wenn, dann wäre an der Stelle
eine Variable die richtige Wahl.
##########
tobago-theme/tobago-theme-standard/src/main/ts/tobago-tree.ts:
##########
@@ -85,6 +104,169 @@ export class Tree extends HTMLElement {
get selectable(): Selectable {
return Selectable[this.getAttribute("selectable")] as Selectable;
}
+
+ get nodes(): NodeListOf<TreeNode> {
+ return this.querySelectorAll("tobago-tree-node");
+ }
+
+ get visibleNodes(): TreeNode[] {
+ return Array.from(this.nodes).filter(node => (node as
HTMLElement).offsetParent !== null) as TreeNode[];
+ }
+
+ private get hasCheckboxes(): boolean {
+ return this.querySelector("input[type=checkbox], input[type=radio]") !==
null;
+ }
+
+ connectedCallback(): void {
+ for (const node of Array.from(this.nodes)) {
+ (node as HTMLElement).setAttribute("tabindex", "-1");
+
+ const subElements = node.querySelectorAll(".tobago-toggle,
input[type=checkbox], input[type=radio]");
+ subElements.forEach(el => el.setAttribute("tabindex", "0"));
+ }
+
+ if (Tree.focusedNodeId) {
+ const savedNode =
this.querySelector(`tobago-tree-node[id="${Tree.focusedNodeId}"]`) as
HTMLElement;
+ if (savedNode) {
+ const targetSelector = Tree.focusedSubElementClass || ".tobago-toggle,
input";
+ const subEl = savedNode.querySelector(targetSelector) as HTMLElement;
+ if (subEl) {
+ subEl.focus();
+ }
+ this.addEventListener("keydown", this.handleKeydown);
Review Comment:
Warum muss hier eigentlich ein keydown-Listener gesetzt werden? In Zeile 141
gibts doch schon einen, der auf die tobago-tree-Komponente gesetzt wird. Fängt
der nicht alles ab? Und wenn nicht, was ist die Ursache?
##########
tobago-theme/tobago-theme-standard/src/main/ts/tobago-tree-static.ts:
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export class TreeStatic {
+
+ /**
+ * Bestimmt den Tabindex-Wert für einen Tree-Node-Container.
+ * Tree-Nodes selbst sollten nicht im Tab-Fokus liegen (tabindex="-1"),
+ * nur die Sub-Elemente (Toggle, Checkbox, Radio).
+ * @returns -1 für den Node-Container
+ */
+ static getTabindexForNode(): number {
+ return -1;
+ }
+
+ /**
+ * Bestimmt den Tabindex-Wert für Sub-Elemente eines Nodes (Toggle,
Checkbox, Radio).
+ * Diese sind normalerweise tabbbar und erhalten tabindex="0".
+ * @returns 0 für sub-Elemente, die tabbbar sein sollen
+ */
+ static getTabindexForSubElement(): number {
Review Comment:
Diese Funktion wie nicht benutzt. Aber selbst wenn, dann wäre an der Stelle
eine Variable die richtige Wahl.
##########
tobago-theme/tobago-theme-standard/src/main/ts/tobago-tree.ts:
##########
@@ -85,6 +104,169 @@ export class Tree extends HTMLElement {
get selectable(): Selectable {
return Selectable[this.getAttribute("selectable")] as Selectable;
}
+
+ get nodes(): NodeListOf<TreeNode> {
+ return this.querySelectorAll("tobago-tree-node");
+ }
+
+ get visibleNodes(): TreeNode[] {
+ return Array.from(this.nodes).filter(node => (node as
HTMLElement).offsetParent !== null) as TreeNode[];
+ }
+
+ private get hasCheckboxes(): boolean {
+ return this.querySelector("input[type=checkbox], input[type=radio]") !==
null;
+ }
+
+ connectedCallback(): void {
+ for (const node of Array.from(this.nodes)) {
+ (node as HTMLElement).setAttribute("tabindex", "-1");
+
+ const subElements = node.querySelectorAll(".tobago-toggle,
input[type=checkbox], input[type=radio]");
+ subElements.forEach(el => el.setAttribute("tabindex", "0"));
+ }
+
+ if (Tree.focusedNodeId) {
+ const savedNode =
this.querySelector(`tobago-tree-node[id="${Tree.focusedNodeId}"]`) as
HTMLElement;
+ if (savedNode) {
+ const targetSelector = Tree.focusedSubElementClass || ".tobago-toggle,
input";
+ const subEl = savedNode.querySelector(targetSelector) as HTMLElement;
+ if (subEl) {
+ subEl.focus();
+ }
+ this.addEventListener("keydown", this.handleKeydown);
+ return;
+ }
+ }
+
+ this.addEventListener("keydown", this.handleKeydown);
Review Comment:
Bitte
private listeners: EventListenerStore = new EventListenerStore();
benutzen.
##########
tobago-theme/tobago-theme-standard/src/main/ts/tobago-tree.ts:
##########
@@ -85,6 +104,169 @@ export class Tree extends HTMLElement {
get selectable(): Selectable {
return Selectable[this.getAttribute("selectable")] as Selectable;
}
+
+ get nodes(): NodeListOf<TreeNode> {
+ return this.querySelectorAll("tobago-tree-node");
+ }
+
+ get visibleNodes(): TreeNode[] {
+ return Array.from(this.nodes).filter(node => (node as
HTMLElement).offsetParent !== null) as TreeNode[];
+ }
+
+ private get hasCheckboxes(): boolean {
+ return this.querySelector("input[type=checkbox], input[type=radio]") !==
null;
+ }
+
+ connectedCallback(): void {
+ for (const node of Array.from(this.nodes)) {
+ (node as HTMLElement).setAttribute("tabindex", "-1");
+
+ const subElements = node.querySelectorAll(".tobago-toggle,
input[type=checkbox], input[type=radio]");
+ subElements.forEach(el => el.setAttribute("tabindex", "0"));
Review Comment:
Grundsätzlich sollte der DOM so gerendert werden, dass man nicht
nachträglich im TypeScript noch Änderungen durchführen muss. Manchmal mag es
aber Gründe dafür geben.
Hier sieht es so aus, als wenn alle Checkboxen/Toggles immer ein tabindex=0
haben. Würde ich hier
https://github.com/apache/myfaces-tobago/blob/1d32b3e477e4afc622ea9fad1d52d2cd09a20d63/tobago-core/src/main/java/org/apache/myfaces/tobago/internal/renderkit/renderer/TreeIndentRenderer.java#L68
und
https://github.com/apache/myfaces-tobago/blob/1d32b3e477e4afc622ea9fad1d52d2cd09a20d63/tobago-core/src/main/java/org/apache/myfaces/tobago/internal/renderkit/renderer/TreeSelectRenderer.java#L134
hinschreiben.
##########
tobago-theme/tobago-theme-standard/src/main/ts/tobago-tree.ts:
##########
@@ -85,6 +104,169 @@ export class Tree extends HTMLElement {
get selectable(): Selectable {
return Selectable[this.getAttribute("selectable")] as Selectable;
}
+
+ get nodes(): NodeListOf<TreeNode> {
+ return this.querySelectorAll("tobago-tree-node");
+ }
+
+ get visibleNodes(): TreeNode[] {
+ return Array.from(this.nodes).filter(node => (node as
HTMLElement).offsetParent !== null) as TreeNode[];
+ }
+
+ private get hasCheckboxes(): boolean {
+ return this.querySelector("input[type=checkbox], input[type=radio]") !==
null;
+ }
+
+ connectedCallback(): void {
+ for (const node of Array.from(this.nodes)) {
+ (node as HTMLElement).setAttribute("tabindex", "-1");
Review Comment:
Focussiert werden sollen nur die Checkboxen und die .tobago-toggle Elemente.
Die tobago-tree-node Element brauchen keinen tabindex.
##########
tobago-theme/tobago-theme-standard/src/main/ts/tobago-tree.ts:
##########
@@ -85,6 +104,169 @@ export class Tree extends HTMLElement {
get selectable(): Selectable {
return Selectable[this.getAttribute("selectable")] as Selectable;
}
+
+ get nodes(): NodeListOf<TreeNode> {
+ return this.querySelectorAll("tobago-tree-node");
+ }
+
+ get visibleNodes(): TreeNode[] {
+ return Array.from(this.nodes).filter(node => (node as
HTMLElement).offsetParent !== null) as TreeNode[];
+ }
+
+ private get hasCheckboxes(): boolean {
+ return this.querySelector("input[type=checkbox], input[type=radio]") !==
null;
+ }
+
+ connectedCallback(): void {
Review Comment:
Den connectedCallback() und disconnectedCallback() würde ich unter den
constructor() schreiben. Die Getter/Setter am Ende der Datei.
##########
tobago-theme/tobago-theme-standard/src/main/ts/tobago-tree-static.ts:
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export class TreeStatic {
+
+ /**
+ * Bestimmt den Tabindex-Wert für einen Tree-Node-Container.
+ * Tree-Nodes selbst sollten nicht im Tab-Fokus liegen (tabindex="-1"),
+ * nur die Sub-Elemente (Toggle, Checkbox, Radio).
+ * @returns -1 für den Node-Container
+ */
+ static getTabindexForNode(): number {
+ return -1;
+ }
+
+ /**
+ * Bestimmt den Tabindex-Wert für Sub-Elemente eines Nodes (Toggle,
Checkbox, Radio).
+ * Diese sind normalerweise tabbbar und erhalten tabindex="0".
+ * @returns 0 für sub-Elemente, die tabbbar sein sollen
+ */
+ static getTabindexForSubElement(): number {
+ return 0;
+ }
+
+ /**
+ * Bestimmt, ob eine Checkbox/Radio bei der Navigation fokussiert werden
soll.
+ * Dies hängt davon ab, ob Checkboxen im Tree vorhanden sind,
+ * welches Element aktuell fokussiert ist (Toggle oder Input),
+ * und ob der nächste Node ein Toggle hat.
+ *
+ * @param hasCheckboxes - ob der Tree Checkboxes/Radios enthält
+ * @param currentTargetIsToggle - ob das aktuelle fokussierte Element ein
Toggle ist
+ * @param nextNodeHasToggle - ob der nächste Node ein Toggle hat
+ * @returns true, wenn die Checkbox fokussiert werden soll, false für Toggle
+ */
+ static shouldFocusCheckbox(
Review Comment:
Hab jetzt bemerkt, dass keine Funktion in tobago-tree-static.ts benutzt
wird. Dann kann die Datei auch weg.
##########
tobago-theme/tobago-theme-standard/src/main/ts/tobago-tree.ts:
##########
@@ -85,6 +104,169 @@ export class Tree extends HTMLElement {
get selectable(): Selectable {
return Selectable[this.getAttribute("selectable")] as Selectable;
}
+
+ get nodes(): NodeListOf<TreeNode> {
+ return this.querySelectorAll("tobago-tree-node");
+ }
+
+ get visibleNodes(): TreeNode[] {
+ return Array.from(this.nodes).filter(node => (node as
HTMLElement).offsetParent !== null) as TreeNode[];
+ }
+
+ private get hasCheckboxes(): boolean {
+ return this.querySelector("input[type=checkbox], input[type=radio]") !==
null;
+ }
+
+ connectedCallback(): void {
+ for (const node of Array.from(this.nodes)) {
+ (node as HTMLElement).setAttribute("tabindex", "-1");
+
+ const subElements = node.querySelectorAll(".tobago-toggle,
input[type=checkbox], input[type=radio]");
+ subElements.forEach(el => el.setAttribute("tabindex", "0"));
+ }
+
+ if (Tree.focusedNodeId) {
+ const savedNode =
this.querySelector(`tobago-tree-node[id="${Tree.focusedNodeId}"]`) as
HTMLElement;
+ if (savedNode) {
+ const targetSelector = Tree.focusedSubElementClass || ".tobago-toggle,
input";
+ const subEl = savedNode.querySelector(targetSelector) as HTMLElement;
+ if (subEl) {
+ subEl.focus();
+ }
+ this.addEventListener("keydown", this.handleKeydown);
+ return;
+ }
+ }
+
+ this.addEventListener("keydown", this.handleKeydown);
+ }
+
+ disconnectedCallback(): void {
+ this.removeEventListener("keydown", this.handleKeydown);
Review Comment:
private listeners: EventListenerStore = new EventListenerStore();
hier dann "this.listeners.disconnect()" benutzen.
##########
tobago-theme/tobago-theme-standard/src/main/ts/tobago-tree.ts:
##########
@@ -85,6 +104,169 @@ export class Tree extends HTMLElement {
get selectable(): Selectable {
return Selectable[this.getAttribute("selectable")] as Selectable;
}
+
+ get nodes(): NodeListOf<TreeNode> {
+ return this.querySelectorAll("tobago-tree-node");
+ }
+
+ get visibleNodes(): TreeNode[] {
+ return Array.from(this.nodes).filter(node => (node as
HTMLElement).offsetParent !== null) as TreeNode[];
+ }
+
+ private get hasCheckboxes(): boolean {
+ return this.querySelector("input[type=checkbox], input[type=radio]") !==
null;
+ }
+
+ connectedCallback(): void {
+ for (const node of Array.from(this.nodes)) {
+ (node as HTMLElement).setAttribute("tabindex", "-1");
+
+ const subElements = node.querySelectorAll(".tobago-toggle,
input[type=checkbox], input[type=radio]");
+ subElements.forEach(el => el.setAttribute("tabindex", "0"));
+ }
+
+ if (Tree.focusedNodeId) {
+ const savedNode =
this.querySelector(`tobago-tree-node[id="${Tree.focusedNodeId}"]`) as
HTMLElement;
+ if (savedNode) {
+ const targetSelector = Tree.focusedSubElementClass || ".tobago-toggle,
input";
+ const subEl = savedNode.querySelector(targetSelector) as HTMLElement;
+ if (subEl) {
+ subEl.focus();
+ }
+ this.addEventListener("keydown", this.handleKeydown);
+ return;
+ }
+ }
+
+ this.addEventListener("keydown", this.handleKeydown);
+ }
+
+ disconnectedCallback(): void {
+ this.removeEventListener("keydown", this.handleKeydown);
+ }
+
+ focusNode(node: TreeNode | HTMLElement, preferCheckbox: boolean = false):
void {
Review Comment:
Soll nicht immer die Checkbox fokussiert werden, falls eine vorhanden ist?
Dann wird man den zweiten Parameter los.
Beim node-Parameter reicht es, wenn nur HTMLElement übergeben wird.
##########
tobago-theme/tobago-theme-standard/src/main/ts/tobago-tree.ts:
##########
@@ -85,6 +104,169 @@ export class Tree extends HTMLElement {
get selectable(): Selectable {
return Selectable[this.getAttribute("selectable")] as Selectable;
}
+
+ get nodes(): NodeListOf<TreeNode> {
+ return this.querySelectorAll("tobago-tree-node");
+ }
+
+ get visibleNodes(): TreeNode[] {
+ return Array.from(this.nodes).filter(node => (node as
HTMLElement).offsetParent !== null) as TreeNode[];
+ }
+
+ private get hasCheckboxes(): boolean {
+ return this.querySelector("input[type=checkbox], input[type=radio]") !==
null;
+ }
+
+ connectedCallback(): void {
+ for (const node of Array.from(this.nodes)) {
+ (node as HTMLElement).setAttribute("tabindex", "-1");
+
+ const subElements = node.querySelectorAll(".tobago-toggle,
input[type=checkbox], input[type=radio]");
+ subElements.forEach(el => el.setAttribute("tabindex", "0"));
+ }
+
+ if (Tree.focusedNodeId) {
+ const savedNode =
this.querySelector(`tobago-tree-node[id="${Tree.focusedNodeId}"]`) as
HTMLElement;
+ if (savedNode) {
+ const targetSelector = Tree.focusedSubElementClass || ".tobago-toggle,
input";
+ const subEl = savedNode.querySelector(targetSelector) as HTMLElement;
+ if (subEl) {
+ subEl.focus();
+ }
+ this.addEventListener("keydown", this.handleKeydown);
+ return;
+ }
+ }
+
+ this.addEventListener("keydown", this.handleKeydown);
+ }
+
+ disconnectedCallback(): void {
+ this.removeEventListener("keydown", this.handleKeydown);
+ }
+
+ focusNode(node: TreeNode | HTMLElement, preferCheckbox: boolean = false):
void {
+ if (!node) {
+ return;
+ }
+
+ const toggle = node.querySelector(".tobago-toggle") as HTMLElement;
+ const input = node.querySelector("input[type=checkbox],
input[type=radio]") as HTMLInputElement;
+
+ if (!this.hasCheckboxes && toggle) {
+ toggle.focus();
+ return;
+ }
+
+ if (input && (preferCheckbox || !toggle)) {
+ input.focus();
+ } else if (toggle) {
+ toggle.focus();
+ } else if (input) {
+ input.focus();
+ }
+ }
+
+ private handleKeydown(event: KeyboardEvent): void {
+ const key = event.key || (event as any).key || "";
Review Comment:
Ich hätte so ein switch-case verwendet. Siehe:
https://github.com/apache/myfaces-tobago/blob/1d32b3e477e4afc622ea9fad1d52d2cd09a20d63/tobago-theme/tobago-theme-standard/src/main/ts/tobago-select-list-base.ts#L90
Man muss dann einige Blöcke in Funktionen zusammenfassen. z.B. den Block für
up/down
```
private selectPreviousNextNode() : void {
const visibleNodes = this.visibleNodes;
const idx = visibleNodes.indexOf(node as any);
if (idx === -1) {
return;
}
const next = isUp ? visibleNodes[idx - 1] : visibleNodes[idx + 1];
if (next) {
const nextHasCheckbox = next.querySelector("input[type=checkbox],
input[type=radio]") !== null;
const nextHasToggle = next.querySelector(".tobago-toggle") !== null;
const preferCheckbox = this.hasCheckboxes && (nextHasCheckbox ||
!nextHasToggle);
this.focusNode(next, preferCheckbox);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
