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-22280: Object editor dropdowns are not keyboard accessible #3254

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Jul 5, 2024

Jira URL

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

Changes

Description

  • Added translations for the hint of the dropdowns
  • Updated the DOM generated for the class and object editors to be more precise and correct on semantics.
  • Updated dataeditors.js to take those changes into account.
  • Updated the styles to take those changes into account
  • Improved organization in dataeditors.css
  • Added a transition on the caret rotations.
  • Added basic keyboard support to the property reorder tool as an alternative to drag and drop
  • Updated button classes to use current bootstrap button styles (btn-default instead of btn-secondary...)
  • Refined a bit more the style to fit better with what it used to be and better UI navigability.
  • Updated translation to reflect the new functionality of the reorder button.

Clarifications

  • We rely on a prototype library, scriptaculous, to power this page. Ideally we'd move everything to Jquery, but this is beyond the scope of this PR, so I just extended the scriptaculous code to make the sortable components keyboard operable.
  • I decided to have the highlight of items on both :focus and :hover. This can create situations where there's two items highlighted. IMO this is okay, but this could be considered as confusing and we might look for another solution (two highlight colors, or keeping it like it used to be, on hover only, and have only the blue-border for the focus state)

Screenshots & Video

Video taken after e030e84 :

2024-07-05.16-23-46.mp4

Executed Tests

See #3254 (comment)

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • None, DOM changes are pretty dangerous. Even if it's an UI which should not have too much custom code relying on.

* Added translations for the hint of the dropdowns
* Updated the DOM generated for the class and object editors to be more precise and correct on semantics.
* Updated the .js to take those changes into account.
* Updated the styles to take those changes into account
* Improved organization in dataeditors.css
* Added a transition on the caret rotations.

TODO:
* remove the value from the titles, the punctuation doesn't match the use
* Accessibility of the reordering feature
* Added basic keyboard support to the reorder tool as an alternative to drag and drop
* Removed titles from the toggling buttons, they only conveyed repetitive info and would need some additional (very similar) translations
* Updated style to use current bootstrap button styles (btn-default instead of btn-secondary...)
* Refined a bit more the style to fit better with what it used to be and better UI navigability.
* Updated translation to reflect the new functionality of the reorder button.
@tkrieck
Copy link
Contributor

tkrieck commented Jul 9, 2024

Hi @Sereza7 some remarks on this:

  • The delete button can be use the trash icon, this way we can standardize delete actions to a trash can icon and remove to an X icon.
  • Could the buttons for delete, move and edit be moved to the beginning of the line? There's a lot of space between line title and these actions.
  • The edit icon seems to be bigger than the others.

The overall design can also be improved by giving more clear delimitations on where something starts and where something ends. It could be by adding some borders and backgrounds and removing unnecessary icons. The example below I managed to do with just CSS, without changing the DOM (by no means I am saying that it should be this way, more like an idea for improvements.).

Screenshot 2024-07-09 at 16 09 27

What do you think?

* Replaced the cross with a trash icon for the delete actions.
@Sereza7
Copy link
Contributor Author

Sereza7 commented Jul 12, 2024

@tkrieck

The delete button can be use the trash icon, this way we can standardize delete actions to a trash can icon and remove to an X icon.

👍 Done in ce80b17

Could the buttons for delete, move and edit be moved to the beginning of the line? There's a lot of space between line title and these actions.

It would need a lot of refactoring, the caret and its title are quite stronly linked now. Putting active elements in the middle is not possible without quite a few fixes. Since it's also a change compared to the old UI, I propose to open a ticket and handle this improvement in another PR later down the line. Is that okay with you? (I'll open the ticket when/if I get your confirmation)

The edit icon seems to be bigger than the others.

It seems to be a quirk of FontAwesome icons, nothing much I can do here except add extra style just to counterbalance this difference in size perceived.
With a trash icon it's way better AFAICS:
Screenshot from 2024-07-12 17-16-08
Screenshot from 2024-07-12 17-15-51

@tkrieck
Copy link
Contributor

tkrieck commented Jul 12, 2024

I propose to open a ticket and handle this improvement in another PR later down the line. Is that okay with you? (I'll open the ticket when/if I get your confirmation)

Yes, that's perfect! 👍

With a trash icon it's way better AFAICS:
Yes, definitely.

@Sereza7
Copy link
Contributor Author

Sereza7 commented Jul 16, 2024

Created https://jira.xwiki.org/browse/XWIKI-22333 👍

@Sereza7
Copy link
Contributor Author

Sereza7 commented Sep 11, 2024

Testing session:
First, we built the changes successfully with mvn clean install -f xwiki-platform-core/xwiki-platform-flamingo/xwiki-platform-flamingo-skin/xwiki-platform-flamingo-skin-resources and mvn clean install -f xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/ -Pquality,docker .

  • Looked for xclass-title and toggle-collapsable, no result outside of .vm, .css and dataeditors.js
  • Looked at the page obejcts ObjectEditPaqge.java and ClassEditPage.java. Those look too high level to handle the togglers. Togglers are about the objects/properties inside of those pages
  • Looked at the BaseElements ObjectEditPane.java
    • xobject_%s_%s_content should not be impacted by our changes
    • xobject_%s_%s_title has its content changed, but it does not seem like it's breaking any selector or how we use the object
    • In order to make sure, I looked for a test that used displayObject from ObjectEditPane. SkinxTest fit the bill. mvn clean install -f xwiki-platform-distribution/xwiki-platform-distribution-flavor/xwiki-platform-distribution-flavor-test/xwiki-platform-distribution-flavor-test-ui/ -Pchrome ran without issue
    • Similar selector used in ClassPropertyEditPane expand(). Following this function usage, I tested it with addProperty from the EditClassIT tests. mvn clean install -f xwiki-platform-core/xwiki-platform-flamingo/xwiki-platform-flamingo-skin/xwiki-platform-flamingo-skin-test/xwiki-platform-flamingo-skin-test-docker passed successfully.

@Sereza7 Sereza7 marked this pull request as ready for review September 11, 2024 14:28
@surli
Copy link
Member

surli commented Sep 12, 2024

Similar selector used in ClassPropertyEditPane expand(). Following this function usage, I tested it with addProperty from the EditClassIT tests. mvn clean install -f xwiki-platform-core/xwiki-platform-flamingo/xwiki-platform-flamingo-skin/xwiki-platform-flamingo-skin-test/xwiki-platform-flamingo-skin-test-docker passed successfully.

Frankly for such changes I'd run entirely ObjectEditorIT

<h2>
$!escapetool.xml($doc.displayView($field.xWikiClass.get('prettyName'), "${field.name}_" , $field))
($doc.displayView($field.xWikiClass.get('name'), "${field.name}_" , $field):
<h3>
Copy link
Member

Choose a reason for hiding this comment

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

Was a mistake before to use h2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added additional structure to the page with a H2 :
https://github.com/xwiki/xwiki-platform/pull/3254/files#diff-6188397dc36b434aca69e83161e131eb37f552008a22b918a6b8bd1bc925b22eR183-R189

This is clearly a subcategory of the propertylist, so I think a H3 here would be appropriate.

item.makePositioned();
item.appendChild(movebutton);
movebutton.observe('click', function(event) {
event.stop();
}.bindAsEventListener());
// Extend the Sortable items with arrow keys support
movebutton.observe('keydown', function(event){
if (![38, 40].includes(event.keyCode)) { return; }
Copy link
Member

Choose a reason for hiding this comment

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

Worth a comment to indicate what 38/40 are (Up/Down I guess?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in ebc4586 👍

item.makePositioned();
item.appendChild(movebutton);
movebutton.observe('click', function(event) {
event.stop();
}.bindAsEventListener());
// Extend the Sortable items with arrow keys support
movebutton.observe('keydown', function(event){
Copy link
Member

Choose a reason for hiding this comment

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

I'd externalize the whole function to simplify future refactoring. And possibly for testing too...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get exactly what level of externalization I should aim for. Do you have a reference I could check to externalize this function?

var item = event.target.parentElement.parentElement.parentElement;
/* We need to recompute the name of the entry similarly to what's done in scriptaculous
to make sure it matches the one we get in the sequence. */
let format = /^[^_\-](?:[A-Za-z0-9\-\_]*)[_](.*)$/;
Copy link
Member

Choose a reason for hiding this comment

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

Where did you get this regex from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Directly from the scriptaculous codebase --> https://github.com/madrobby/scriptaculous/blob/b0a8422f7f6f2e2e17f0d5ddfef1d9a6f5428472/src/dragdrop.js#L592

Extended the comment to be more precise in ebc4586 👍

@Sereza7
Copy link
Contributor Author

Sereza7 commented Sep 22, 2024

The object addition field does get the user out of the edit page as of now. For usability and accessibility it would be better to fix this item here.

@Sereza7 Sereza7 marked this pull request as draft September 22, 2024 08:33
@Sereza7
Copy link
Contributor Author

Sereza7 commented Oct 4, 2024

@surli
#3254 (comment)

Frankly for such changes I'd run entirely ObjectEditorIT

The command did run it entirely :)


This PR is mostly waiting on #3528 to be finished and merged.

There's still this topic that's not solved but it'll probably be part of the changes needed in the update after the refactoring of dataeditors.js anyways.

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