Escaping jsTree node ids

[CharpentierLucas]

Hello!

Context

For XWIKI-22154, the document tree widget builds each node’s id straight from the entity reference, e.g. document:xwiki:A B.WebHome. A literal space in an HTML id is invalid.

Solution

The clean fix to this is escaping/unescaping the id everywhere the tree sends or receives it, and exposing the helper $.fn.xtree.unescapeNodeId() so other scripts can get the raw reference back easily. I found 6 places in xwiki-platform itself that read this id and parsed it as a reference (see the draft at PR #6205).

I looked for a way to leave the id untouched and only fix the rendered HTML attribute, but jsTree ties the two together internally: changing one without the other breaks clicking on tree nodes entirely. So the id format itself has to change.

Proposal

Treat this jsTree node id as internal, non-public API, and accept that any contrib extension or customization parsing it the same way our 6 xwiki-platform scripts did will need the same one-line fix.

Opinion

This id isn’t documented anywhere as public API, but the in-repo scripts relying on it shows it’s a de facto one, so this is worth discussing here . A quick GitHub code search confirms this status: xwiki-contrib/application-filemanager sends tree.get_selected() straight to its own download job as a file path, and downloading a file with a space in its name would break with this fix. If this fix is agreed on, I’ll open a ticket on that project to make sure it’s kept up to date.

+1 from me for this somewhat breaking change: I don’t see a way to fix the HTML validity bug (that has real accessibility consequences) otherwise, and I’d rather make the compatibility break explicit (release note + the unescapeNodeId() helper) than leave the bug in place.

Conclusion

Do you know of additional contrib extension or customizations that rely on this raw node id format? Do you agree to break it? Should we mark it as API from now on or leave it as an in-between?

I’ll close this topic in a week if there’s no objection, and proceed from the proposed solution that can be found in #6205.

Thank you for your interest in the topic! I’m looking forward to your answers
Lucas C.

[pjeanjean]

Hi,

@CharpentierLucas

Do you know of additional contrib extension or customizations that rely on this raw node id format?

Cristal does (see cristal/core/navigation-tree/navigation-tree-xwiki/src/components/componentsInit.ts at main · xwiki-contrib/cristal · GitHub), but it shouldn’t be hard to change if the format breaks. Though it would help if there was a way to know for sure which id format to use depending on the XWiki instance.

[vmassol]

Disclaimer up front: I don’t know this topic well, so I asked Claude Code to dig into the PR and the tree code. Below is what it came back with. It looks interesting enough to be worth posting, but please take it as input to be double-checked rather than as my own reviewed opinion.

The escaping in #6205 is entirely client-side, so the wire format is unchanged. tree.js unescapes before every server call and escapes what comes back, which means anything talking to the tree data URL directly is unaffected – including Cristal: its componentsInit.ts fetches bin/get?sheet=XWiki.DocumentTree&data=children&id=document:xwiki:... itself and reads treeNode.id from the JSON response, never touching the jsTree model. Same for the {{tree}}/{{documentTree}} macro’s openTo and root parameters. So @pjeanjean there should be nothing to change on the Cristal side, and no need to detect the id format per instance. The break is narrower than the proposal suggests: only in-page JS that reads an id out of the jsTree model (get_selected(), data.node.id) and parses it as a reference.

But the PR only handles the outbound direction (tree → id); the inbound direction (id → tree) still builds raw ids and feeds them into the tree. Places that look like they would still be broken for a name containing a space: exporter.js:570-573 (builds 'document:' + serialize(currentDocument) then open_node/select_node – it is listed as safe in the PR, and open_node on an unknown id never fires its callback, so the promise never resolves); PanelsCode/NavigationConfigurationSheet.xml:369 (creates a node with a raw id, so the invalid HTML id survives there), plus :413 get_node(...) and :480 hide_node(...); XWiki/InplaceEditing.xml:1266 set_text('document:' + ...); and tree_macros.vm:108/:113, where the macro’s data/rootData options go into data-json/data-root and bypass escapeNodeId() entirely.

Suggested change of invariant: escape on the way in, unescape only on the way out. Right now unescapeId() is applied both to ids coming from the tree model (correct) and to caller-supplied raw ids in getPath()/openTo() (locationPicker.js, attachment/move.js, AnnotationConfigSheet.xml), where it silently turns a page literally named A%20B into A B – the same class of bug as XWIKI-18356/XWIKI-22151. Escaping every entry point instead (openTo, data-openTo, data-json, data-root, job responses) fixes that, fixes the inline-JSON case, and keeps all existing raw-id inputs working, which shrinks the compatibility break down to just “reading ids out of jsTree” – exactly where unescapeNodeId() belongs.

Two smaller things. The escaping only covers space and %, but HTML5 forbids all ASCII whitespace in id (tab, LF, CR, FF too); and since the format is breaking anyway, type: + encodeURIComponent(reference) would be a standard, self-documenting scheme that also escapes the \ which already breaks By.id() in Selenium (see the comment in TreeElement.hasNode). And given this has already regressed twice (#3015 reverted → XWIKI-22151 → XWIKI-22154), a functional test that moves a page into a space literally named A B and asserts no whitespace in the rendered id/aria-activedescendant seems like the thing that makes it stick.

On the actual question: +1 for the break, and the line that seems defensible is “the wire/macro-level id format stays public, the escaped client-side form is an implementation detail of the widget, with escapeNodeId()/unescapeNodeId() as the documented bridge” – which is what the code already does.

[CharpentierLucas]

About the inbound-direction gap: good catch, confirmed by re-reading the diff. open_node/select_node/get_node/hide_node calls that build a raw id and feed it straight into the tree stay broken for names with spaces, since only the outbound direction is escaped in the current PR. Same for tree_macros.vm’s data-json/data-root, which skip escapeNodeId() entirely.

I’ll rework the PR to escape on the way in instead of unescaping on the way out, as you suggest, and fix those spots plus the openTo/getPath code, which wrongly unescapes caller-supplied raw ids.

I’ll escape all HTML5-forbidden whitespace, not just space, and I like type: + encodeURIComponent(reference) better than the current scheme: it’s self-documenting and also avoids the \ issue in TreeElement.hasNode’s Selenium By.id().
I’ll add a functional test to check that the escaping works as expected.

So far the proposal seems to be mostly agreed upon. I’ll wait a bit more before closing but will continue spending some time on implementing it.

Thank you for your feedback!

[CharpentierLucas]

Since there were no more answers on this topic, I confirm we’ll go with this latest solution.
Thanks!

Lucas C.

P.S. Feel free to add anything to the conversation, the sooner the better

[mflorea]

@CharpentierLucas I see this only now. Sorry. Maybe I missed it, but can you describe the problem. You mentioned:

A literal space in an HTML id is invalid.

and:

has real accessibility consequences

What consequences are we talking about? How exactly is the whitespace in the tree node ID affecting the end user?

Thanks,
Marius

[CharpentierLucas]

Hi, thank you for the feedback! Your question made me re-check, and my “real accessibility consequences” wording was stronger than what I can actually find.

The aria-labelledby the ticket was filed against, the one WAVE reported as a “Broken ARIA reference”, is no longer emitted (jsTree 3.3.16 has that line commented out, and nothing in xwiki-platform sets it on tree nodes), so that broken reference is gone. What remains is the invalid id itself, plus the aria-activedescendant pointing at it, and I checked that one in Chromium’s accessibility tree rather than assuming: being a single IDREF rather than a list, it resolves fine despite the space. So no, there is no screen reader regression today.

So what is the actual problem

We generate invalid HTML. The spec is explicit that an id value “must not contain any ASCII whitespace”, so any wiki with a space in a page name emits invalid markup that validators flag. That seems reason enough on its own, without needing a user-visible symptom.

In addition, it’s quite fragile:

• A node id in an ID reference list attribute (aria-labelledby, aria-owns, headers) would resolve to nothing, silently. That is exactly how this bug was found, and how it could come back on the next jsTree update.
• querySelector('#' + id) would throw a SyntaxError, so every caller would have to remember to escape.
• Selenium’s By.id() would break the same way, as the comment in TreeElement.hasNode already notes.

Thanks,
Lucas C.

[mflorea]

@CharpentierLucas

• querySelector('#' + id) would throw a SyntaxError, so every caller would have to remember to escape.

Is that limited to whitespace? On id HTML global attribute - HTML | MDN I read:

Technically, the value for an ID attribute may contain any other Unicode character. However, when used in CSS selectors, either from JavaScript using APIs like Document.querySelector() or in CSS stylesheets, ID attribute values must be valid CSS identifiers. This means that if an ID attribute value is not a valid CSS identifier (for example, my?id or 1234) then it must be escaped before being used in a selector, either using the CSS.escape() method or manually.

So any code that builds CSS selectors from an ID it doesn’t own must escape the ID. That’s common practice.

@CharpentierLucas

• Selenium’s By.id() would break the same way, as the comment in TreeElement.hasNode already notes.

That comment links to driver.findElements(By.id("..")) not working correctly when id contains semicolon · Issue #8173 · SeleniumHQ/selenium-google-code-issue-archive · GitHub which appears to be fixed. Moreover, Claude tells me that By.id(...) ends up as "#" + cssEscape(id) so it should work fine with special characters, including space.

The only problem that stands is the ID-list attributes, which, as you say, are currently not used by the tree. So the question for me is whether it’s worth spending time on this and taking the risk of such a change for a problem we don’t have right now. I’m not convinced.

Thanks,
Marius

[CharpentierLucas]

Thank you for insisting on the topic. So in the end there’s really barely any reason for the complexity of implementing this.

I personally still don’t like the idea of keeping invalid HTML, but the facts show it would do more harm than good to fix this.

I’ll close the PR and close the ticket as a won’t fix.

[Simpel]

As invalid html is an instant fail of the bitv (german wcag) admins could adapt their page naming rules to not use whitespaces right?

[CharpentierLucas]

From what I saw there’s a recent update to EN 301 549 to revoke the need for this (following the deprecation of parsing in WCAG). Hopefully BITV will align soon

Unfortunately XWIKI-22154: "aria-labelledby" for navigation panel entries and corresponding ids contain illegal whitespaces by Sereza5 · Pull Request #6205 · xwiki/xwiki-platform · GitHub spans 13 files changes (without test files), so it’d be quite a pain to apply this patch on an instance

As invalid html is an instant fail of the bitv (german wcag) admins could adapt their page naming rules to not use whitespaces right?

Unfortunately I think that’d be the best solution for now.
The other option would be to remove page trees from your wiki :S

Thank you for sharing your position on the topic!