-
-
Notifications
You must be signed in to change notification settings - Fork 550
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
base: master
Are you sure you want to change the base?
Conversation
* 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.
Hi @Sereza7 some remarks on this:
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.). What do you think? |
* Replaced the cross with a trash icon for the delete actions.
👍 Done in ce80b17
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)
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. |
Yes, that's perfect! 👍
|
Testing session:
|
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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
...amingo-skin/xwiki-platform-flamingo-skin-resources/src/main/resources/flamingo/editobject.vm
Outdated
Show resolved
Hide resolved
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; } |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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){ |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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\-\_]*)[_](.*)$/; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
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. |
* Added escaping for the titles
* Added comments
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. |
Jira URL
https://jira.xwiki.org/browse/XWIKI-22280
Changes
Description
Clarifications
: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