XWIKI-22696: Annotation marker is not keyboard accessible #4960
Conversation
* Added a button to be able to have all the expected click behavior Bugs left: * When clicking the button with the keyboard, the event position is wrong. * The button is not removed properly when removing the highlights or refreshing them (remove+add)
* Updated the semantics and fixed previous bugs.
* Fixed incorrect ranges and hopefully some tests
* Updated the style * Added a variable to the XS variableSet. * TODO: Fix a bug with highlighting. The solution right now is buggy. The goal is to be able to highlight the whole set of marks when one is hovered. The issue is that they are not linked except by a shared --programatically variable-- class. I'm not sure how to have some CSS applying on all of those.
* Fixed escaping * Replaced the buttons with a * Added logic to handle the hover effects. * Removed hard-coded background colors.
* Fixed docker tests. The added javascript was bugging when the annotations could not be found.
michitux
left a comment
michitux
left a comment
There was a problem hiding this comment.
At the moment, when you highlight a link, you can still click this link when annotations are displayed. From what I understood from the code, with this change this is no longer possible. This seems like an important regression to me. Also, from what I understood, if a link is inside the first span, you would also nest a elements which isn't allowed in HTML.
I agree that the tiny icon isn't ideal but just disabling all interaction with the highlighted content is also not okay in my opinion. Maybe we could put a bigger icon behind the highlighted text? I've added a comment on https://forum.xwiki.org/t/brainstorming-interactivity-of-annotations/17998
| We use this class set dynamically in JS instead of pseudo classes because we want highlights to span multiple | ||
| marks across text blocks. */ | ||
| /* We repeat the selector because of nested annotation marks. */ | ||
| background-color: var(--btn-warning-bg); |
There was a problem hiding this comment.
Shouldn't this then also use the foreground color of the warning button? Otherwise, this might not work with color themes where the text color doesn't have enough contrast with the warning background. See also https://www.w3.org/WAI/WCAG21/Techniques/failures/F24
There was a problem hiding this comment.
I'm not sure about this.
The design proposal did not show any change in foreground color. And the old UI used to not change the foreground color while applying a background color.
I agree using the --btn-warning-bg variable by itself is not really clean, but it's either that or introducing a second color to the color theme --mark-bg-highlighted. WDYT?
The more I think about it and the more itnroducing that second variable seems best. We can still make its default value var(--btn-warning-bg) but users can override it without messing the warning colors.
| "font-weight-semibold": 700, | ||
| "font-weight-bold": "900" | ||
| "font-weight-bold": "900", | ||
| "mark-bg": "color-mix(in srgb, var(--btn-warning-bg), white)" |
There was a problem hiding this comment.
I think it is dangerous to mix colors this way. How do you know that mixing this color with white will give you a color that has enough contrast with the text color? What about making this an actual color theme preference that defaults to this mix?
There was a problem hiding this comment.
It's WCAG on the Iceberg colorTheme, and it can easily be overridden from pretty much anywhere (thanks to CSS variables :) ). Adding a color theme preference comes with a lot of noise on the PR so I wanted to avoid this here.
This should work on most light colorthemes and it's way better than hardcoding it completely like it was done so I figured it was okay to do.
IMO this can be an improvement that comes later if there's a user need for it. If you agree that this can wait, I'll open a ticket :)
|
We need the link or a button to make sure things are keyboard interactive, but it just occurred to me we didn't NEED the first text node to be inside it. There's already everything done to handle extra injected nodes for the link description, so we might as well just add an "invisible button" for this. A bit like the one that's available for image lightbox availability, it only appears above the content when it's keyboard focused. This way we could even use a small button, because the content is known and small. In the end it's pretty much just adding the previously CSS only icon to the DOM and showing it only when it's focused with a keyboard. We still have the clickable marks as an improvement for mouse users. @michitux
We need the link to be the first text node element parent for this to be disabled. It's not easy to start a mouse selection exactly at the start of a link. When testing, naturally I'd get at least one character (often a space) next to the link in the content I want to annotate. In half of those cases, it means the anchor is added around the space right before the link. We could disable this wrapping only if we see that the first text node is in a link but it will make the annotatedDOM even less consistent/bug prone/difficult to debug.
It's disabling interaction with the first text Node parent, pretty much only if it's a link AFAIU. Buttons and hard-coded divs should still be able to process the event like they usually do. All the other wrappers are spans that do not block any interaction. It just looks a bit weird because the loading bubble is displayed a few frames before the other action usually has time to occur. |
|
On the standard sandbox page that contains a list of links, I can easily select just one of those list items. Then the annotation is fully contained inside the link, and, even with the current implementation, cannot be opened anymore after creating it as any attempt to open it will trigger the link. I like your idea of adding an extra button that is only visible in certain cases. Maybe we could add a button with a bigger icon before or after (I would prefer after) the annotation that becomes visible either when focused or when the annotation is hovered (with a comfortable delay before disappearing to make it possible to reach it even when not following a perfect path with the mouse)? The position of the button in the DOM should be outside the closest interactive ancestor if there is any to avoid interfering with links. As the button wouldn't be constantly visible I guess it would be okay if it would hide some content. Regarding the clickable marks, I'm not fully convinced yet that this won't cause any negative side effects. Like, can you still open the image lightbox when an image is inside the annotation? I've just tried, and it seems annotations cannot include images. Still, it feels like there is a lot of potential for confusion with this overloaded click behavior that might not be obvious to (in particular non-technical) users. |
|
I reread the other proposals, and there's plans to enable annotations by default, so we need to make sure the UX is seamless. Some users might interact with it without knowing what it is.
+1 for everything but I'm not too sure about the button appearing when hovered. The hover already changes the background color of the text. We'd need to put another delay before the button appears, else it would make the feature very intrusive. This was not in the design. For keyboard/focus users, it feels like a small oversight where it's fair to add. Here I'm not sure about adding changes, the hover state was discussed.
Let's assume images can be added to the mark. It could be a macro or any custom UI.
If the users relies on AT completely, this is not a problem at all (focus is in the right place still :)). Else, we might get the mark bubble displayed above the lightbox bubble. That could cause a problem: accessing the lightbox feature this way is really difficult. |
* Added the note icon * Moved the toggle so that it wouldn't break interactivity with links * Added a bit of logic to the toggle. * Added parsing to a script, forgot to add it earlier. It surprisingly didn't fail any tests... * Updated the filter function for text nodes.
|
There's still a failing test, the new button cannot be scrolled to and clicked. |
* Made the test click on the annotation itself instead of the toggle button.
* Removed the z-index. This lowers display priority of the bubble which avoids it overlapping actual feature bubbles/toggles that could be triggered.
|
Docker tests fixed. I updated the PR based on what was discussed above. Here is what the UI looks like with the latest changes applied: 2025-12-22.10-58-05.mp4 |
2 participants
Jira URL
https://jira.xwiki.org/browse/XWIKI-22696
Changes
Description
Clarifications
Screenshots & Video
This video showcases the changes applied on my local instance. In this, I had already set up four annotations:
Towards the end of the demo, I add a new annotation in a place "entertwined" with the large one.
We can see that everything behaves as expected. The only odd point is the case where we click on multiple annotations at once, it will open up multiple annotation bubbles at once. It could be very confusing, but IMO this is quite a niche case, usually there will be threads on the same annotation instead of wrapped ones.
2025-12-17.17-54-33.mp4
When creating an annotation in the video demo, the page reloads without a message. It's been fixed since this was recorded.
Executed Tests
Manual tests on my local instance (see video above).
Built the changes successfully with
mvn clean install -f xwiki-platform-core/xwiki-platform-annotation/xwiki-platform-annotation-test/xwiki-platform-annotation-test-pageobjects -Pqualitymvn clean install -f xwiki-platform-core/xwiki-platform-annotation/xwiki-platform-annotation-ui -PqualityThen succesfully ran the docker tests at
mvn clean install -f xwiki-platform-core/xwiki-platform-annotation/xwiki-platform-annotation-test/xwiki-platform-annotation-test-docker/.Expected merging strategy