XWIKI-22154: "aria-labelledby" for navigation panel entries and corresponding ids contain illegal whitespaces

* Escaped the node ids on the way into the tree and unescaped them only on the way out
* Percent-encoded the node ids, keeping the colon and the at sign as is
* Escaped the node ids passed to openTo() and the ones specified inline through data-json
* Unescaped the selected node ids written into the tree field value
* Escaped the node ids built by the export tree, the navigation panel configuration, the in-place editor and the annotation configuration
* Unescaped the selected node ids in the template provider picker and the WYSIWYG image selector
* Escaped and unescaped the node ids in the tree page objects
* Added a functional test for the escaping of the node ids
* Added a functional test for moving a page to a parent whose name contains white space

Author: Sereza5

xwiki-platform-core/xwiki-platform-administration/xwiki-platform-administration-ui/src/main/resources/XWiki/TemplateProviderMacros.xml

@@ -214,7 +214,8 @@
     $('.location-picker.modal').prop('modalTrigger', $(this)).modal();
   });
 
-  var getLocalSpaceReference = function(nodeId) {
+  var getLocalSpaceReference = function(escapedNodeId) {
+    var nodeId = $.fn.xtree.unescapeNodeId(escapedNodeId);
     var separatorIndex = nodeId.indexOf(':');
     var nodeType = nodeId.substr(0, separatorIndex);
     var nodeStringReference = nodeId.substr(separatorIndex + 1);

xwiki-platform-core/xwiki-platform-annotation/xwiki-platform-annotation-ui/src/main/resources/AnnotationCode/AnnotationConfigSheet.xml

@@ -289,21 +289,22 @@ $annotationsConfigDoc.display('annotationClass', 'edit')
             // Once the node is loaded, check it, if possible.
             // If the node did not load (lack of permissions, hidden document, etc.), nothing happens.
             tree.check_node(idToCheck);
-          }.bind(this, savedId));
+          }.bind(this, $.fn.xtree.escapeNodeId(savedId)));
         }
       });
 
       // Listen to tree nodes being checked/unchecked and save the changes in the DOM to be saved on submit.
       treeElement.on('changed.jstree', function(event, data) {
-        var node = data.node;
+        // The savedIds list holds the node ids as they are serialized in the form field, i.e. unescaped.
+        var nodeId = $.fn.xtree.unescapeNodeId(data.node.id);
         var initialNrOfNodes = savedIds.length;
 
         // Update the savedIds list.
-        var savedIdIndex = savedIds.indexOf(node.id);
+        var savedIdIndex = savedIds.indexOf(nodeId);
         if (data.action == 'select_node') {
           // If it's not in the list, add it.
           if (savedIdIndex == -1) {
-            savedIds.push(node.id);
+            savedIds.push(nodeId);
           }
         } else if (data.action == 'deselect_node') {
           // If it's in the list, remove it.

xwiki-platform-core/xwiki-platform-ckeditor/xwiki-platform-ckeditor-test/xwiki-platform-ckeditor-test-pageobjects/src/main/java/org/xwiki/ckeditor/test/po/LinkTreeElement.java

@@ -87,7 +87,7 @@ public LinkTreeElement waitForNewPageCreation(EntityReference reference)
      */
     public void createNode(DocumentReference origin, String name)
     {
-        String createDocumentNodeId = getCreateDocumentNodeId(origin);
+        String createDocumentNodeId = escapeNodeId(getCreateDocumentNodeId(origin));
         WebElement originNode = getDriver().findElement(By.id(createDocumentNodeId));
         originNode.findElement(By.tagName("a")).click();
         // We cannot reuse the element
@@ -97,7 +97,7 @@ public void createNode(DocumentReference origin, String name)
         SpaceReference targetSpace = new SpaceReference(name, origin.getLastSpaceReference());
         DocumentReference target = new DocumentReference("WebHome", targetSpace);
         getDriver().waitUntilCondition(
-            ExpectedConditions.presenceOfElementLocated(By.id(getNodeId(target)))
+            ExpectedConditions.presenceOfElementLocated(By.id(escapeNodeId(getNodeId(target))))
         );
         openTo(getNodeId(target));
     }

xwiki-platform-core/xwiki-platform-edit/xwiki-platform-edit-ui/src/main/resources/XWiki/InplaceEditing.xml

@@ -1263,7 +1263,7 @@ require(['jquery', 'xwiki-meta'], function($, meta) {
   var updateDocTrees = function(xwikiDocument) {
     var plainTitle = xwikiDocument.getPlainTitle();
     $('.jstree-xwiki').each(function() {
-      $(this).jstree?.(true)?.set_text?.('document:' + xwikiDocument.id, plainTitle);
+      $(this).jstree?.(true)?.set_text?.($.fn.xtree.escapeNodeId('document:' + xwikiDocument.id), plainTitle);
     });
   };
 

xwiki-platform-core/xwiki-platform-flamingo/xwiki-platform-flamingo-skin/xwiki-platform-flamingo-skin-test/xwiki-platform-flamingo-skin-test-docker/src/test/it/org/xwiki/flamingo/test/docker/RenamePageIT.java

@@ -35,6 +35,7 @@
 import org.xwiki.flamingo.skin.test.po.AttachmentsPane;
 import org.xwiki.flamingo.skin.test.po.AttachmentsViewPage;
 import org.xwiki.flamingo.skin.test.po.JobQuestionPane;
+import org.xwiki.index.tree.test.po.DocumentPickerModal;
 import org.xwiki.model.reference.AttachmentReference;
 import org.xwiki.model.reference.DocumentReference;
 import org.xwiki.model.reference.EntityReference;
@@ -869,4 +870,42 @@ void convertTerminalPageToNestedPage(TestUtils setup) throws Exception
         assertTrue(breadcrumb.endsWith("/4/child"),
             "Expected breadcrumb to end with /4/child but was [" + breadcrumb + "]");
     }
+
+    /**
+     * Move a page to a parent whose name contains white space, using the tree picker. The tree escapes the identifier
+     * of its nodes so that they can be used as HTML identifiers, so this checks that it unescapes them back before
+     * sending them to the server. See XWIKI-22151 and XWIKI-22154.
+     */
+    @Order(13)
+    @Test
+    void renamePageToParentWithWhiteSpaceUsingTreePicker(TestUtils setup) throws Exception
+    {
+        String targetSpaceName = "Rename Space";
+        DocumentReference targetParent = new DocumentReference("xwiki", targetSpaceName, "WebHome");
+        DocumentReference source =
+            new DocumentReference("xwiki", Arrays.asList("RenamePageIT", "MoveToSpaceWithWhiteSpace"), "WebHome");
+        DocumentReference target =
+            new DocumentReference("xwiki", Arrays.asList(targetSpaceName, "MoveToSpaceWithWhiteSpace"), "WebHome");
+
+        setup.rest().delete(targetParent);
+        setup.rest().delete(source);
+        setup.rest().delete(target);
+
+        setup.createPage(targetParent, "", targetSpaceName);
+        setup.createPage(source, "", "MoveToSpaceWithWhiteSpace");
+
+        new SolrTestUtils(setup).waitEmptyQueue();
+
+        RenamePage renamePage = setup.gotoPage(source).rename();
+        renamePage.getDocumentPicker().browseDocuments();
+        new DocumentPickerModal().selectDocument(targetSpaceName, "WebHome");
+        renamePage.clickRenameButton().waitUntilFinished();
+
+        // The page landed in the space that was selected in the tree, not in a space named after its escaped
+        // identifier (e.g. "Rename%20Space").
+        assertTrue(setup.pageExists(Arrays.asList(targetSpaceName, "MoveToSpaceWithWhiteSpace"), "WebHome"),
+            "The page was not moved to the [" + targetSpaceName + "] space.");
+        assertFalse(setup.pageExists(Arrays.asList("RenamePageIT", "MoveToSpaceWithWhiteSpace"), "WebHome"),
+            "The page is still in its original location.");
+    }
 }

xwiki-platform-core/xwiki-platform-index/xwiki-platform-index-test/xwiki-platform-index-test-docker/src/test/it/org/xwiki/index/test/ui/docker/DocumentTreeMacroIT.java

@@ -22,6 +22,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.regex.Pattern;
 
 import org.junit.jupiter.api.Order;
 import org.junit.jupiter.api.Test;
@@ -51,6 +52,13 @@
 @UITest
 class DocumentTreeMacroIT
 {
+    /**
+     * The HTML attributes that hold a tree node identifier.
+     */
+    private static final List<String> ID_ATTRIBUTE_NAMES = List.of("id", "aria-activedescendant", "aria-labelledby");
+
+    private static final Pattern WHITE_SPACE = Pattern.compile("\\s");
+
     @Test
     @Order(1)
     void sortDocumentsBy(TestUtils setup, TestReference testReference) throws Exception
@@ -477,6 +485,55 @@ void documentTreeRootOpenTo(TestUtils setup, TestReference testReference)
         assertFalse(tree.hasNode(getNodeId(subAlice)));
     }
 
+    /**
+     * Verify that the node identifiers rendered by the tree don't contain characters that are forbidden in an HTML
+     * identifier, and that the tree still resolves them back to the right entity reference when it talks to the server.
+     */
+    @Test
+    @Order(7)
+    void escapeNodeIds(TestUtils setup, TestReference testReference)
+    {
+        SpaceReference testSpaceReference = testReference.getLastSpaceReference();
+        DocumentReference parent = new DocumentReference("WebHome", new SpaceReference("A B", testSpaceReference));
+        DocumentReference child =
+            new DocumentReference("WebHome", new SpaceReference("C D", parent.getLastSpaceReference()));
+
+        setup.loginAsSuperAdmin();
+        // Clean up.
+        setup.deletePage(testReference, true);
+        createPage(setup, parent, "", "");
+        createPage(setup, child, "", "");
+
+        TreeElement tree = getDocumentTree(setup, testReference, true, Map.of("openTo", getNodeId(child)));
+
+        // The tree opened to the specified node, which means it was able to send the escaped node identifiers back to
+        // the server, unescaped, both to compute the path and to load the child nodes.
+        assertTrue(tree.hasNode(getNodeId(child)));
+        assertNodeLabels(tree.getNode(getNodeId(parent)).getChildren(), "C D");
+        assertEquals(List.of(getNodeId(child)), tree.getSelectedNodeIDs());
+
+        // None of the identifiers rendered by the tree contains white space, which is forbidden in an HTML identifier.
+        List<String> renderedIds = getRenderedIds(setup, testSpaceReference.getName());
+        assertTrue(renderedIds.contains(TreeElement.escapeNodeId(getNodeId(child))),
+            "The node with white space in its reference was not rendered: " + renderedIds);
+        renderedIds.forEach(renderedId -> assertFalse(WHITE_SPACE.matcher(renderedId).find(),
+            "The rendered identifier [" + renderedId + "] contains white space."));
+    }
+
+    /**
+     * @return the values of the attributes that hold a node identifier, collected from the tree with the given HTML
+     *         identifier and from all its descendants
+     */
+    @SuppressWarnings("unchecked")
+    private List<String> getRenderedIds(TestUtils setup, String treeId)
+    {
+        return (List<String>) setup.getDriver().executeScript("const attributeNames = arguments[0];"
+            + "const tree = document.getElementById(arguments[1]);"
+            + "return [tree, ...tree.querySelectorAll('*')]"
+            + "  .flatMap(element => attributeNames.map(name => element.getAttribute(name)))"
+            + "  .filter(value => value !== null);", ID_ATTRIBUTE_NAMES, treeId);
+    }
+
     private ViewPage createPage(TestUtils setup, DocumentReference documentReference, String title, String content)
     {
         // We don't care what parent page is used, we just want to avoid creating orphan pages in order to not interfere

xwiki-platform-core/xwiki-platform-panels/xwiki-platform-panels-ui/src/main/resources/PanelsCode/NavigationConfigurationSheet.xml

@@ -316,6 +316,7 @@ require([
   'xwiki-l10n!xwiki-panels-navigation-config-messages',
   'xwiki-meta',
   'jquery-ui',
+  'xwiki-tree',
   'xwiki-events-bridge'
 ], function($, l10n, xwikiMeta) {
   var getLocalDocumentReference = function(absoluteDocumentReference) {
@@ -366,7 +367,7 @@ require([
   var getNodeData = function(exclusion) {
     var documentReference = exclusion.data('reference');
     return {
-      id: 'document:' + documentReference,
+      id: $.fn.xtree.escapeNodeId('document:' + documentReference),
       text: exclusion.text(),
       children: exclusion.data('children') !== false,
       data: {
@@ -410,7 +411,7 @@ require([
   };
 
   var showOrCreateNode = function(tree, page) {
-    var node = tree.get_node('document:' + page.data('reference'));
+    var node = tree.get_node($.fn.xtree.escapeNodeId('document:' + page.data('reference')));
     if (node) {
       tree.show_node(node.id);
     } else {
@@ -477,7 +478,7 @@ require([
 
   var excludePagesFromDynamicFilter = function(pages, tree) {
     tree && pages.find('.page').not('.included').each(function() {
-      tree.hide_node('document:' + $(this).data('reference'));
+      tree.hide_node($.fn.xtree.escapeNodeId('document:' + $(this).data('reference')));
     });
   };
 

xwiki-platform-core/xwiki-platform-tree/xwiki-platform-tree-test-pageobjects/src/main/java/org/xwiki/tree/test/po/TreeElement.java

@@ -19,6 +19,8 @@
  */
 package org.xwiki.tree.test.po;
 
+import java.net.URLDecoder;
+import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
@@ -37,6 +39,12 @@
  */
 public class TreeElement extends BaseElement
 {
+    /**
+     * The characters that {@code encodeURIComponent()} leaves unchanged.
+     */
+    private static final String UNRESERVED_CHARACTERS =
+        "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.!~*'()";
+
     /**
      * The element that represents the tree.
      */
@@ -58,7 +66,7 @@ public TreeElement(WebElement element)
      */
     public TreeNodeElement getNode(String nodeId)
     {
-        return new TreeNodeElement(this.element, By.id(nodeId));
+        return new TreeNodeElement(this.element, By.id(escapeNodeId(nodeId)));
     }
 
     /**
@@ -67,12 +75,10 @@ public TreeNodeElement getNode(String nodeId)
      */
     public boolean hasNode(String nodeId)
     {
-        // We cannot use By.id(nodeId) because findElements returns 0 elements if the id contains special characters
-        // such as backslash (which is used to escape special characters in an entity reference which the node id can
-        // be). Such an element id is technically invalid but the browsers are handling it fine.
-        // See https://code.google.com/p/selenium/issues/detail?id=8173
+        // We cannot use By.id() because findElements returns 0 elements if the id contains special characters that
+        // Selenium doesn't escape properly. See https://code.google.com/p/selenium/issues/detail?id=8173
         return !getDriver().findElementsWithoutWaiting(this.element,
-            By.xpath(".//*[@id = '" + nodeId + "']")).isEmpty();
+            By.xpath(".//*[@id = '" + escapeNodeId(nodeId) + "']")).isEmpty();
     }
 
     /**
@@ -133,7 +139,7 @@ public TreeElement waitForIt()
     public TreeElement waitForNodeSelected(String nodeId)
     {
         String selectedNodeXPath =
-            String.format(".//*[@id = '%s_anchor' and contains(@class, 'jstree-clicked')]", nodeId);
+            String.format(".//*[@id = '%s_anchor' and contains(@class, 'jstree-clicked')]", escapeNodeId(nodeId));
         getDriver().waitUntilElementIsVisible(this.element, By.xpath(selectedNodeXPath));
         return this;
     }
@@ -149,8 +155,9 @@ protected WebElement getElement()
     @SuppressWarnings("unchecked")
     public List<String> getSelectedNodeIDs()
     {
-        return (List<String>) getDriver()
+        List<String> selectedNodeIds = (List<String>) getDriver()
             .executeScript("return jQuery.jstree.reference(jQuery(arguments[0])).get_selected()", this.element);
+        return selectedNodeIds.stream().map(TreeElement::unescapeNodeId).toList();
     }
 
     /**
@@ -163,7 +170,48 @@ public List<String> getNodeIDs()
                 + "{flat:true, no_data:true, no_state:true})" + ".map(function(element) {return element.id});",
             this.element);
 
-        return Arrays.asList(selectedNodeIDs);
+        return Arrays.stream(selectedNodeIDs).map(TreeElement::unescapeNodeId).toList();
+    }
+
+    /**
+     * Percent-encodes the given node identifier the same way the tree widget does, so that it matches the {@code id}
+     * attribute of the corresponding HTML element. Page objects that locate a tree node by its identifier need this
+     * because entity references can contain characters that are forbidden in an HTML identifier, such as white space.
+     *
+     * @param nodeId the node identifier, holding the entity reference as is
+     * @return the escaped node identifier
+     * @since 18.8.0RC1
+     */
+    public static String escapeNodeId(String nodeId)
+    {
+        if (nodeId == null) {
+            return null;
+        }
+        StringBuilder result = new StringBuilder();
+        for (byte nodeIdByte : nodeId.getBytes(StandardCharsets.UTF_8)) {
+            char character = (char) (nodeIdByte & 0xFF);
+            if (UNRESERVED_CHARACTERS.indexOf(character) >= 0) {
+                result.append(character);
+            } else {
+                result.append(String.format("%%%02X", nodeIdByte & 0xFF));
+            }
+        }
+        // The colon and the at sign separate the components of a node identifier and are both valid in an HTML
+        // identifier, so the tree widget leaves them as is, in order to keep the escaped node identifiers readable.
+        return result.toString().replace("%3A", ":").replace("%40", "@");
+    }
+
+    /**
+     * Reverts {@link #escapeNodeId(String)}, turning a node identifier read from the tree back into the entity
+     * reference it holds.
+     *
+     * @param nodeId the escaped node identifier
+     * @return the node identifier holding the entity reference as is
+     * @since 18.8.0RC1
+     */
+    public static String unescapeNodeId(String nodeId)
+    {
+        return nodeId == null ? null : URLDecoder.decode(nodeId, StandardCharsets.UTF_8);
     }
 
     /**