Skip to content

XWIKI-24144: Clarify original and target pages during replace dialog#5369

Open
Sereza7 wants to merge 2 commits intoxwiki:masterfrom
Sereza7:XWIKI-24144
Open

XWIKI-24144: Clarify original and target pages during replace dialog#5369
Sereza7 wants to merge 2 commits intoxwiki:masterfrom
Sereza7:XWIKI-24144

Conversation

@Sereza7
Copy link
Copy Markdown
Contributor

@Sereza7 Sereza7 commented Apr 2, 2026

Jira URL

https://jira.xwiki.org/browse/XWIKI-24144

Changes

Description

  • Edited the DOM slightly to improvew semantics and reduce confusion
  • Added hints

Clarifications

  • There's a lot more semantic improvements to do in this template, but I limited the change to what was needed to update for the sake of this ticket.
  • I moved the icon in the link. It makes the icon clickable and its color changes accordingly with font iconthemes. This is not needed for this ticket but a small improvement
  • I decided to keep the old translations to keep things consistent, but to answer the request made in the ticket to increase understandability I added hints and moved the translation to a label instead of being the content of the anchor itself. The anchor content describes the destination and is enough to understand where it goes without any context, which is better than before :) There's a bit of repeated information (technically the titles of both pages are the same), but IMO it just helps understandability.

Screenshots & Video

Here is what
Screenshot From 2026-04-02 17-33-59

Executed Tests

Manual tests on my local instance.
Built the changes with mvn clean install -f xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-templates -Pquality
and passed successfully mvn clean install -f xwiki-platform-core/xwiki-platform-refactoring/ -Pquality,integration-tests,docker.

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • 17.10.X , very low scope bugfix with pretty standard changes.

* Edited the DOM slightly to improvew semantics and reduce confusion
* Added hints
</h5>
<label>$title</label>
<div class="xHint">$hint</div>
<a href="$document.getURL()">$services.icon.renderHTML('file-white')$document.plainTitle</a>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not too much work, I'd be curious to see the difference between

  • this
  • this but with a non-breakable space between the icon of the text
  • the icon outside the link

I'm also interested in what it looks like on hover. I think someone mentioned a possibly odd look with the underline spanning below the icon.

The rest of the PR looks ok, thanks for your work :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Printing $document.plainTitle without XML-escaping introduces an XSS vulnerability. This needs to be fixed (escaped).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manuelleduc

If not too much work, I'd be curious to see the difference between [...]

Here are screenshots of the three options in the same order, with the mouse hovering on one of the two links:
Screenshot From 2026-04-09 13-59-09
Screenshot From 2026-04-09 13-52-07
Screenshot From 2026-04-09 13-58-27

I decided to add the nbsp in 6ff4ece because I believe it's what we do in most places and it looks a bit better. This looks a bit odd when hovered but IMO it's alright.
The only strong opinion I have on this is that I think it makes sense to make the icon clickable so we should include it in the link.

I think someone mentioned a possibly odd look with the underline spanning below the icon.

Just to be sure, I checked with Silk and IMO things look okay here too:
Screenshot From 2026-04-09 14-02-23

I think this was a comment made about the underlining under "nothing" between the icon and the text. I remember a comment was done when we changed it in the notification dropdown:
image

This is an argument against adding a NBSP here, but in my opinion it's not strong enough to enforce the choice.

It's a small layout detail, but it could be something to include in the UX guidelines to share with Cristal @tkrieck has proposed on the forum. Something like:
When using an icon with its explaining text, separate them with a non breaking space. (vocab is probably too implementation centered)
or
When using an icon with its explaining text, do not separate them with any character.


@michitux

Printing $document.plainTitle without XML-escaping introduces an XSS vulnerability. This needs to be fixed (escaped).

Good find! Sorry for missing this one. I fixed it in 6ff4ece 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants