Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XWIKI-19145: Edit button must be unaccessible for keyboard while in edit mode #2191

Merged
merged 4 commits into from Jun 1, 2023

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented May 26, 2023

Jira: https://jira.xwiki.org/browse/XWIKI-19145

PR Changes:

  • Made the anchor semantically disabled when it used to do so visually (and get back to the regular state)
  • Removed the role=button from the menu anchors.
  • Updated LESS to fit code style and keep a visually appealing

Notes

  • I decided to remove the role=button from the anchors. See the jira comments for details as to why.
  • There is a difference in the colors of the disabled button compared to before. However now it works properly with dark themes:
    Before vv
    19145-formerDarkly
    After vv
    19145-updatedDarkly

View

Video demo: https://up1.xwikisas.com/#hTNmIXq9INwB0FZ7m11OBw

Tests

Successfully passed the tests:

  • mvn clean install -Pquality,integration-tests,docker -f xwiki-platform-core/xwiki-platform-test/ -Dxwiki.test.ui.wcag=true
  • mvn clean install -Pquality,integration-tests,docker -f xwiki-platform-core/xwiki-platform-edit/ -Dxwiki.test.ui.wcag=true

…dit mode

* Made the anchor semantically disabled when it used to do so visually (and get back to the regular state)
* Removed the role=button from the menu anchors.
* Updated LESS to fit code style and keep a visually appealing

TODO: check tests
@michitux
Copy link
Contributor

As far as I can see, these changes only target inplace editing. What about the other edit modes (like WYSIWYG) that are also mentioned in the issue?

@Sereza7
Copy link
Contributor Author

Sereza7 commented May 30, 2023

@michitux
I could not test the issue with other edit modes. I tested this on a fresh install and could only witness an 'unabled' edit button when starting inplace editing.
See this video displaying it on a fresh 15.4 install.
I checked out the code quickly and from what I could see, there is no reference to "#tmEdit" (which is the root element of the button we want to change) in the wysiwyg editor files.

(Added this as a comment on the jira ticket so it'll be easier to find later)

@manuelleduc
Copy link
Contributor

manuelleduc commented May 31, 2023

As far as I can see, these changes only target inplace editing. What about the other edit modes (like WYSIWYG) that are also mentioned in the issue?

In other words, did you test with a page like localhost:8080/xwiki/bin/edit/Sandbox/WebHome?editor=wysiwyg ?
Afaics, the edit button can be edited from the keyboard here too.

image

Also, I'm not fully sure to understand the issue but some other advanced edit mode could be impacted as well.

@Sereza7
Copy link
Contributor Author

Sereza7 commented May 31, 2023

@manuelleduc
On your screen, it seems like the button is not visually "disabled", and it should still react to mouse clicks. There isn't any accessibility issue here (might be a funcitonality bug but I doubt it).
View for wysiwyg editor:
19145-WysiWygedit

The accessibility issue reported is for when the button looks "disabled" such as when editing inplace:
19145-inplaceEdit

You can see on the screens above that the looks of the edit button are not the same. The issue is that the button, even if it looked disabled and did not react to mouse clicks during inplace editing, was not disabled for keyboard users.

At first when reading the issue I thought that the edit button would be greyed out even in Wysiwyg edit mode, but it is not.
I checked the other advanced user edit modes, and it seems to be the same here.
Moreover, the css class responsible for making this "disabled" effect is only found in the code of the inplace editor.
So I figured there was an imprecision in the ticket, some of it could have been fixed as a side effect to another change at one point between the ticket report and now.

@manuelleduc
Copy link
Contributor

On your screen, it seems like the button is not visually "disabled"

ok, that's the part I was missing :) Thanks!

Copy link
Contributor

@michitux michitux left a comment

Choose a reason for hiding this comment

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

Overall this looks good, but the page object has a conflict that needs to be fixed and I would suggest to re-run tests to ensure that this still works after the recent changes regarding button vs. link in #2171.

@michitux michitux merged commit 2eecf3f into xwiki:master Jun 1, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants