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-16216: Allow users to rebind the shortcuts from their user profile UI #2981

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

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Mar 13, 2024

Jira URL

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

Changes

Description

  • Added user preferences for most shortcuts
  • Added tests for this new feature
  • Only display view shortcuts in the preferences when the user can actually use them (usertype = advanced)

Clarifications

  • Some shortcuts are hard-coded, making it impossible to change them easily. They aren't supported in our documentation about changing shortcuts, so I made the choice to not include them in this solution either. They can be added later as an improvement :)
  • Tests don't check every single shortcut, but only the most important use cases on a couple of them.
  • For now, only a simple shortcut syntax is supported. I don't think it's essential to provide a way to input key combo shortcuts (such as x+x+x+a). However this could be an improvement implemented later on.
  • If this PR is merged as is, I'll create tickets for both improvements mentionned above.

Screenshots & Video

16216-demo.mp4

Side note: this video was taken before the last commit that fixed the title attribute on edit action buttons to match the shortcut value whatever the situation.

Executed Tests

Extensive manual tests (see video above).
Passed mvn clean install -f xwiki-platform-core/xwiki-platform-user/xwiki-platform-user-profile/ -Pquality,integration-tests,docker
Passed regular builds for the various packages where there's changes.

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • None, changes are pretty widespread and not the safest.

Sereza7 and others added 17 commits February 23, 2024 17:20
…ile UI

* Added the parameters in the backend
* Added a tab with some default content

TODO:
* Use the backend values when defining shortcuts e.g. in menus_content.vm
* Add the template to view/edit the values of each options. Note that the organization in XWikiPreferencesDocumentInitializer.java can be used as a skeleton for the architecture of the section.
* Add translation keys for all the fields
* Add explanations and a link to xwiki.org doc in the template
…ile UI

* Started using the backend values when defining shortcuts e.g. in menus_content.vm
…ile UI

* Reverted the addition of a category
…ile UI

* Updated preference sheet and translations
…ile UI

* Removed unwanted changes from ImageIT
* Moved the backend preferences from wiki administration to user preferences...
* Added use of those preferences everywhere where we used some translations
* Improved the template for the shortcut preferences
…ile UI

* Updated some tests, not tested yet...
…ile UI

* Fixed the developer shortcut to use the same kind of shortcut as others... highlighted a limitation of this system
* Fixed a typo in translation key
* Added a condition to display view shortcuts only when they are usable (user is advanced).
…ile UI

* Updated tests

TODO: Make sure the changeShortcutInformation test passes. The first one should be good
…ile UI

* Updated a parameter name to fit the shortcut identifier
* Matched the edit action button titles with their new shortcuts
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.

I'm wondering if it wouldn't be better to introduce a script service or at least a Velocity macro to get a keyboard shortcut to avoid these many if/else statements in Velocity.

Regarding the translation keys, all of them start with "Key" even though some of them could clearly be several keys. I would try to find a more generic formulation for this.

For all keyboard shortcuts, the UI should display the default values both in view and edit mode, this would dramatically improve the usability and value of this UI.

I'm wondering if it really makes sense to introduce so many properties in the user class. Sure, it seems like the easiest solution for now, but it hardly scales if, e.g., an extension wants to add another keyboard shortcut that should be configurable. I wonder if it wouldn't make more sense to have one XObject per customized keyboard shortcut on the user profile and a management Java class that provides the list of available keyboard shortcuts and allows querying their values (both with/without user customization to display the global default that could also be customized through a custom translation document) and that provides a way to extend them.

Was there any proposal on the forum for this feature?

Sereza7 and others added 2 commits April 16, 2024 11:51
…ile UI


* Updated default translation value

Co-authored-by: Michael Hamann <michael@content-space.de>
…ile UI


* Updated default translation value

Co-authored-by: Michael Hamann <michael@content-space.de>
@Sereza7 Sereza7 marked this pull request as draft April 19, 2024 08:23
Sereza7 and others added 5 commits April 22, 2024 18:03
…ile UI

* Refactored the way to get the shortcut values.
* Tested the new velocity macro in all its application contexts, everything still works properly (there has been issues with newlines instead of an empty result. Moreover testing on a live distrib was a bit of a pain because of caching and javascript bundling).
@Sereza7
Copy link
Contributor Author

Sereza7 commented Jun 18, 2024

@michitux

I'm wondering if it wouldn't be better to introduce a script service or at least a Velocity macro to get a keyboard shortcut to avoid these many if/else statements in Velocity.

Done in d7fa7e1 👍

Regarding the translation keys, all of them start with "Key" even though some of them could clearly be several keys. I would try to find a more generic formulation for this.

I wanted to keep things simple in those descriptions, because there's a lot of them, and most of the important information to understand those would be repeated. This info is found in the hints for each section (see the video in the PR starting message).
IMO the issue with composite shortcut or key combo is that some people would understand that we rule out simple keys from there. Starting every hint with Simple or composite key combo is a bit too heavy for the improved clarity we get IMO.
The primary use case is still a simple keystroke, so I decided to go with Key.

For all keyboard shortcuts, the UI should display the default values both in view and edit mode, this would dramatically improve the usability and value of this UI.

It seems like we'd need to update/extend the form generation template on the user profile to do so. IMO it's not necessary for a first release of the feature but it'd be a nice improvement. If you're okay with leaving this out of this PR, I'll open a ticket to record this possible improvement.

I'm wondering if it really makes sense to introduce so many properties in the user class. Sure, it seems like the easiest solution for now, but it hardly scales if, e.g., an extension wants to add another keyboard shortcut that should be configurable. I wonder if it wouldn't make more sense to have one XObject per customized keyboard shortcut on the user profile and a management Java class that provides the list of available keyboard shortcuts and allows querying their values (both with/without user customization to display the global default that could also be customized through a custom translation document) and that provides a way to extend them.

I agree that it's not the best. I'd rather keep it simple for now and create a more complete/complex shortcut management system later if there's a need for it. Do you know of extensions that are out of the standard flavor that use shortcuts for common use cases?
Note that the main point of this PR is to improve the accessibility state of the standard flavor.

Was there any proposal on the forum for this feature?

There was https://forum.xwiki.org/t/shortcut-settings-position/14116 for the UI side of things.

@Sereza7 Sereza7 marked this pull request as ready for review June 18, 2024 15:08
…ile UI

* Added an indication for unbound shortcuts in the view mode
…ile UI

* Added a margin between the hint and the list of shortcut preferences
* Escaped characters for XML
@Sereza7
Copy link
Contributor Author

Sereza7 commented Oct 8, 2024

@tkrieck
Because we use regular text inputs, I changed a bit the look of the UI from the wireframe you shared. The view mode is way more compact (it was the default but I quite like it -- there's not much to do for the user here anyways). Here is a video to showcase how it's displayed on different screen sizes and zoom levels:
https://github.com/user-attachments/assets/397a988f-b5f2-4967-9a09-0a9c49576773

After this video I added a small change (margin between the xHint and the table for shortcuts). Here is the final look (100% zoom on a 2k screen):
Simple user view
Screenshot from 2024-10-08 16-54-12
Advanced user view
Screenshot from 2024-10-08 16-53-57
Advanced user edit
image

Does it look okay to you?

PS: Created https://jira.xwiki.org/browse/XWIKI-22558 to keep track of progress on the design improvements for the fields.

@tkrieck
Copy link
Contributor

tkrieck commented Oct 22, 2024

@Sereza7 Thank you very much for the changes. From viewing the final layout, we can improve it a little bit to make reading the page faster. Could please test three other changes?

  1. Provide a line between fields. This way is easier for the eye to read the name of the command and follow the lines to its hotkey. Maybe you will need to space each line a little bit on view mode, otherwise it can look cramped. A rule of thumb you can follow for this is to make each line the same height on view and edit modes.
  2. User bold text to separate sections ("Edit modes", "View Page Information", etc). This way, it is easier to scan the whole content and look for the section that might correlate with a given shortcut. Another alternative is to use the appropriate <H#>...</H#> tag.
  3. Give the edit input a narrower size. When declaring sole keys (a, h, e...) the field is much bigger than it needs to be, but ofc we need to give it sufficient space to view a combination of keys also (alt+shift+e) so I guess a good compromise would be half of the size that you are currently using.

…ile UI

* Improved performance of the shortcut preference retrieval
@Sereza7 Sereza7 requested a review from michitux October 23, 2024 10:07
…ile UI

* Improved preference UI mostly for the view mode.
@Sereza7
Copy link
Contributor Author

Sereza7 commented Oct 23, 2024

@tkrieck
Here's two quick screenshots of the updated UI at 100% zoom on a 2k monitor:
Screenshot from 2024-10-23 14-47-45
Screenshot from 2024-10-23 14-47-13

  1. I wasn't sure what kind of line to go for. With the old colortheme variable as far as I can see (AFAICS) we don't have a very low contrast grey. I used a dashed line so that it wouldn't clutter the UI too much. I hard coded the height of the lines to be the exact same in view and edit mode.
  2. 👍 It's a bit tricky since this part of the UI uses a bold that's not standard for XWiki. I tried to strike a better balance, but now it's a bit more difficult to distinguish between h2 "Keyboard shortcut preferences" and h3s.
  3. I went with 10em. It sounded correct to use it: 1em is the size of 1 character in the context of the block. 10em would fit 10 characters whatever the zoom level or user preferences.

All of those changes were brought in fbcd0f6

@Sereza7
Copy link
Contributor Author

Sereza7 commented Oct 23, 2024

Both presentation and performance questions have been answered, opening this PR back for reviews.

@Sereza7 Sereza7 marked this pull request as ready for review October 23, 2024 12:59
@tkrieck
Copy link
Contributor

tkrieck commented Oct 23, 2024

I wasn't sure what kind of line to go for. With the old colortheme variable as far as I can see (AFAICS) we don't have a very low contrast grey. I used a dashed line so that it wouldn't clutter the UI too much. I hard coded the height of the lines to be the exact same in view and edit mode.

Looks good to me, eventually we will need a very low contrast for these situations, but it can be left for future work.

👍 It's a bit tricky since this part of the UI uses a bold that's not standard for XWiki. I tried to strike a better balance, but now it's a bit more difficult to distinguish between h2 "Keyboard shortcut preferences" and h3s.

It is tricky indeed, you can leave it as it was then. For future work it would make sense to review the CSS of this whole UI. I can see it set its own custom CSS for the headings, I just don't know why.

I went with 10em. It sounded correct to use it: 1em is the size of 1 character in the context of the block. 10em would fit 10 characters whatever the zoom level or user preferences.

Em is about only the height of the font, as far as I know. On non-monospaced fonts, the width of each character will vary ("X" is wider than "I"). Unfortunately, we can't really predict the width of the user chosen font, so we have to eyeball it. https://fonts.google.com/knowledge/glossary/monospaced

@Sereza7
Copy link
Contributor Author

Sereza7 commented Oct 23, 2024

Reverted the change on boldness of the subsection titles 👍
image

I can see it set its own custom CSS for the headings, I just don't know why.

It's a good example of technical debt :) It was a good change back when it was implemented, but it didn't get properly updated, and now we have abstract styles that do not match our current system and cause extra bugs.

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.

Much better regarding performance I think, just a few small comments.

'action_inline' : "$escapetool.javascript("#getShortcutValue('core.shortcuts.edit.backtoedit')")",
'action_save' : "$escapetool.javascript("#getShortcutValue('core.shortcuts.edit.save')")",
'action_propupdate' : "$escapetool.javascript("#getShortcutValue('core.shortcuts.edit.saveandview')")",
'action_saveandcontinue' : "$escapetool.javascript("#getShortcutValue('core.shortcuts.edit.saveandcontinue')")"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as a suggestion, I think it would be much easier to read if this used $escapetool.json() to serialize one big map from Velocity to a JSON object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow I just noticed when testing that the crtUser is not properly set when in the context of velocity evaluation of this script. The shortcuts are not properly set here since we use $crtUserDoc... Looking further into it.

My guess is that we need to make sure frequentlyUserDoc.vm is loaded even for this velocity template evaluation. I need to find the place responsible for evaluating this .js file.

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 the main topic of this comment and the bug I found in 150f1e6 👍

Note that the solution to the bug is the low level solution. It's not a solution to the fact that the frequentlyUsedDocs template is not in the context of $xwiki.jsfx.use.

Copy link
Contributor Author

@Sereza7 Sereza7 Oct 31, 2024

Choose a reason for hiding this comment

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

Seems like this error I made could be related to https://jira.xwiki.org/browse/XWIKI-19455

justify-content: space-between;
align-items: center;
/* Keep the line large enough to fit an input in both view and edit mode. */
height: 38px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this clash with the idea that font size is variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is a CSS file, I decided to hardcode this instead of relying on LESS variables.
Using LESS variables, it would have been quite a lot of operations to get this result:
max(@line-height-base*@font-size-base+ 2*@padding-base-vertical + 2*2 + 21 px, @input-height-base + 21 px)
We want to fit both the button and the input in Edit mode, that both use different variables for their height...

The only impact of defining this height is the min-height. With different sizes of font sizes:

  • Larger font sizes: the edit mode lines are larger than the view mode ones, in order to fit the large inputs.
  • Smaller font sizes: the input and buttons have more space around them in edit mode.

I changed the style to make it clearer that this is not an upper limit on the height of the lines. If the content is large, the line height will increase to fit this content :)

Those are very small drawbacks to hardcoding. I'd rather keep it hardcoded with a comment rather than spend time trying to get something to work with our current LESS and need to rework it soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you use a height based on em?

* @since 16.9.0-rc-1
*#
#macro(getShortcutValue $translationKey)
#set ($shortcutPreferenceObject = $xwiki.getDocument($xcontext.getUser()).getObject('XWiki.UserPreferenceShortcutsClass', 'translationKey', $translationKey))
Copy link
Contributor

Choose a reason for hiding this comment

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

While a script service could certainly make things a bit cleaner, the performance should indeed be okay that way. The clone happens only once this way the first time an object is accessed.

developerShortcutsRestCall("${request.contextPath}/rest/currentuser/properties/usertype/next",
"$escapetool.javascript($services.localization.render('core.shortcuts.developer.user.type.error'))");
}, {'type': shortcut.type.SEQUENCE, 'disable_in_input': true });
}, {'type': #if("$!xwiki.getDocument($xcontext.getUser()).getObject('XWiki.UserPreferenceShortcutsClass', 'translationKey', 'core.shortcuts.developer.user.type')"!='')shortcut.type.SIMPLE#{else}shortcut.type.SEQUENCE#end, 'disable_in_input': true });
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, as written above, this looks okay from a performance point of view.

@Sereza7 Sereza7 marked this pull request as draft October 25, 2024 15:51
@tkrieck
Copy link
Contributor

tkrieck commented Oct 25, 2024

It's a good example of technical debt :) It was a good change back when it was implemented, but it didn't get properly updated, and now we have abstract styles that do not match our current system and cause extra bugs.

I see, it will be nice to revisit these styles in the future. I will keep a tab on my notes about it. Thank you!

…ile UI

* Fixed a bug caused by the recent change of the getShortcutValue macro in actionButtons.js.
* Created the map of shortcuts directly in velocity and serialized it directly to JSON.
…ile UI

* Updated the style to be a bit more specific
@Sereza7 Sereza7 marked this pull request as ready for review October 29, 2024 15:49
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.

We're getting there, thank you for the updates! I think the big issues have been solved, there are just a few details left.

@@ -155,8 +155,9 @@
## Override keyboard shortcut (if any)
##
#if ($keyboardShortcutsEnabled && "$!extraShortcut" != "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this condition still correct? It isn't possible to set a shortcut value if $extraShortcut is empty?

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 updated this in ac1e5f6 👍
Thank you for noticing, the logic here was off.

@@ -155,8 +155,9 @@
## Override keyboard shortcut (if any)
##
#if ($keyboardShortcutsEnabled && "$!extraShortcut" != "")
shortcut.remove("$extraShortcut");
shortcut.add("$extraShortcut", function() { XWiki.displayDocExtra("${extraAnchor}", "${extraTemplate}", true); }, { 'disable_in_input':true });
#set ($extraShortcutWithUserPreferences = "#getShortcutValue($extraShortcut)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this is correct? From what I understand, #getShortcutValue expects a translation key but as this used directly in shortcut.remove, I assume that $extraShortcut is the actual shortcut and not a translation key. Did you verify this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for noticing this 👍
I fixed this implementation in ac1e5f6

I tested this new version locally and it worked properly with unset shortcuts (empty) and regular edited values

#set ($value = $doc.getValue('value', $shortcutPreference))
#if ($isEdit)
&lt;input type="text" class="shortcutPreference" name="$escapedSelectId" id="$escapedSelectId" size="30"
data-translatekey="$translationKey"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use kebab case here, too, so data-translate-key. Note that in JavaScript, this nicely translates to element.dataset.translateKey, see the dataset documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, that's some nice QoL I wasn't aware of! :D

I'm not very knowledgeable on custom data fields, thanks for the help :)

In the javascript I wrote to leverage those custom data fields, I used JQuery.
I updated it to work with those new fields in 9b4344c

With a quick Ctrl+F on this PR's diff files, I checked that those were the only places where those custom properties for the inputs were used.

/* Preferences */
/* Shortcut preferences */
span.xHint:has(+ ul.shortcut-list) {
margin-bottom: 10px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as below, shouldn't this be based on font size/em?

justify-content: space-between;
align-items: center;
/* Keep the line large enough to fit an input in both view and edit mode. */
height: 38px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you use a height based on em?

&lt;label #if($isEdit)for="$escapedSelectId"#end&gt;$!escapetool.xml($services.localization.render($label))&lt;/label&gt;
#if ($services.localization.get($hintKey))
&lt;span id="$escapedHintId" class='xHint'&gt;$!escapetool.xml($services.localization.render($hintKey)) ##
$escapetool.xml($services.localization.render('userprofile.default')): $defaultValue##
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually use the approach to follow best practices in new code without changing everything else. Now if this exact pattern is already used, I'm fine to keep it here.

$.ajax({url: updateUrl,
type: "PUT",
data: data
});
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to handle errors here and maybe also display a notification if the changes have been saved. Otherwise, users have no way to understand that there was an error saving the keyboard shortcuts. See, e.g.,

// Show progress notification message.
var notification = new XWiki.widgets.Notification(l10n['core.editors.saveandcontinue.notification.inprogress'],
'inprogress');
// Collect the submit data.
var editor = editableProperty.next('.editableProperty-viewer').next('.editableProperty-editor');
// Notify the others that we're about to save so that they have the chance to update the submit data.
editor.trigger('xwiki:actions:beforeSave');
var data = editor.find(':input').serializeArray();
data.push({name: 'language', value: xcontext.locale});
data.push({name: 'comment', value: 'Update property ' + editableProperty.data('property')});
data.push({name: 'minorEdit', value: true});
data.push({name: 'form_token', value: xcontext.form_token});
data.push({name: 'ajax', value: true});
data.push({name: 'objectPolicy', value: editableProperty.data('objectPolicy')});
// Make the request to save the property.
return Promise.resolve($.post(XWiki.currentDocument.getURL('save'), data)).then(() => {
editor.trigger('xwiki:document:saved');
notification.replace(new XWiki.widgets.Notification(l10n['core.editors.saveandcontinue.notification.done'],
'done'));
}).catch(response => {
editor.trigger('xwiki:document:saveFailed');
notification.replace(new XWiki.widgets.Notification(l10n['core.editors.saveandcontinue.notification.error'],
'error'));
for an example how this is handled in other places.

Sereza7 and others added 4 commits November 8, 2024 17:16
…ile UI

* Fixed the extra tabs shortcuts implementation
…ile UI

* Fixed the case on the custom data fields (velocity template and js>jquery that relies on it)
…ile UI


* Added a space between the shortcut value and the `(Default)` hint that would be displayed next to it when it has the default value.

Co-authored-by: Michael Hamann <michael@content-space.de>
@Sereza7 Sereza7 marked this pull request as draft November 14, 2024 08:59
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