XWIKI-17664: When adding an annotation the focus should be inside WYSIWYG by default #3694

Sereza7 · 2024-11-26T14:27:36Z

Jira URL

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

Changes

Description

Added a comment to explain a line of code that doesn't work with CKEditor anymore
Added a CKE config change so that it gets focus when loaded.

Clarifications

The legacy way was just a textarea for the annotation edition. I left this in for backwards compatibility. However, in parallel I added a CKEditor config change that would focus the field once it's loaded.
Comments explain why I went with this solution precisely.

Screenshots & Video

On the video below, we can see a basic use case and experimentation with adjacent UIs (namely inline editing and comment edition).

2024-11-26.14-56-41.mp4

Executed Tests

Manual tests, see above.
I doubt the changes would change anything to tests since it's only a minor CKEditor config update (and the docker tests pretty much don't care about the focus state).
Built changes with mvn clean install -f xwiki-platform-core/xwiki-platform-annotation/xwiki-platform-annotation-ui and then successfully passed the docker test suite: mvn clean install -f xwiki-platform-core/xwiki-platform-annotation/xwiki-platform-annotation-test/xwiki-platform-annotation-test-docker.

Expected merging strategy

Prefers squash: Yes
Backport on branches:
- 16.10.X

…IWYG by default * Added a comment to explain a line of code that doesn't work with CKEditor anymore * Added a CKE config change so that it gets focus when loaded.

Sereza7 · 2024-11-27T12:28:40Z

Assigning surli to this ticket since he reviewed and merged #3239 which was a very similar solution.

surli · 2024-12-03T09:04:45Z

...latform-annotation/xwiki-platform-annotation-ui/src/main/resources/AnnotationCode/Script.xml

+            // If the WYSIWYG editor used is CKEditor, we make sure its input gets focused as soon as it's loaded
+            // Note that this CKE config is easier than having to handle custom eventListeners.
+            // As of 17.0.0, this does not interfere with the inline editor, so this solution is okay.
+            if(typeof CKEDITOR !== 'undefined') {


@mflorea I need your input there, is that the proper way to detect CKEditor?

Note that this is the same as the way we used at 3b2b6f3 . If this is not a correct way, we would need to fix that place too.

According to tests on my local instance this works but I cannot attest to the stability of this solution.

Hmm actually in the link you provided @mflorea made some comments about this check and about startupFocus.
Feels like it could be better to put that in an internal library that we would reuse where needed, and so if we need or want to change the way to perform this, we wouldn't need to check everywhere.
wdyt? does it worth it?

As I said on my comment for 3b2b6f3 , I think it's wrong to hard-code the CKEditor usage. The solution should be generic, for any WYSIWYG editor:

either by setting some attribute on the text area that is enhanced by CKEditor, before the WYSIWYG editor is loaded, and having some code in a CKEditor plugin that reacts to that attribute

or by triggering some generic event that a CKEditor plugin can listen to and react

or, if the previous two options are not possible, introduce some generic WYSIWYG editor JavaScript API that we implement for now in a CKEditor plugin, but this requires more work

…IWYG by default * (WIP)

…IWYG by default * Reverted unwanted change (WIP)

…IWYG by default (WIP)

…IWYG by default * Implemented a solution that does not assume there's CKEditor

…IWYG by default * Fixed codestyle

…IWYG by default * Removed uneeded change

Sereza7 · 2024-12-17T16:01:01Z

In the last few commits, I implemented a new solution:

Before calling the displayEdit on the field, the velocity template sets the variable $startupFocus to true.
The CKEditor default template in EditSheet checks this value, and update the textarea that will be replaced by adding it an appropriate data-xxx attribute.
The extension to the CKEditor methods extend and inline are now sensitive to the presence of this data-xxx attribute and change the CKEditor config accordingly.

I added comments in the annotation and comment module templates to explain what is this weird assignation. The comments refer to CKEditor for the sake of clarity because as of now it's the only implementation of an editor using this config. The extra cost for other editors is only setting a boolean that won't be used.

@mflorea Does this implementation look okay to you?

Note that I included a fix on the code that was not in the PR initially. We want to keep things consistent, IMO it makes sense to include it in this very PR since it's not a lot of lines.

Here is a quick demo of the updated changes:

2024-12-17.17-01-54.mp4

Note that in order to make it work, I had to disable query caching in my browser, it got me stuck for a while (I was refreshing the page cache with Ctrl+Shift+R but it wasn't enough).

...latform-annotation/xwiki-platform-annotation-ui/src/main/resources/AnnotationCode/Script.xml

...-platform-ckeditor/xwiki-platform-ckeditor-ui/src/main/resources/CKEditor/VelocityMacros.xml

...ingo-skin/xwiki-platform-flamingo-skin-resources/src/main/resources/flamingo/commentfield.vm

...xwiki-platform-ckeditor/xwiki-platform-ckeditor-ui/src/main/resources/CKEditor/EditSheet.xml

mflorea · 2024-12-18T10:06:00Z

...latform-annotation/xwiki-platform-annotation-ui/src/main/resources/AnnotationCode/Macros.xml

@@ -356,6 +356,8 @@

 #macro(displayAnnotationProperty $annotatedDocument $annotationObject $annotationProperty $mode)
  #if ($mode == 'edit' || $mode == 'create')
+    ## This startupFocus parameter is used by the CKEditor WYSIWYG editor.
+    #set ($startupFocus = true)


I'm wondering if we shouldn't make his a bit more generic / extensible to prepare the ground for other configuration parameters we may need in the future:

Suggested change

#set ($startupFocus = true)

#set ($wysiwygEditorConfig = {

'startupFocus': true

})

This will make the configuration more explicit, and it will reduce the risk of naming conflicts with other entries from the script context. WDYT?

Sounds good 👍
I added changes related to this in 359f650

Note that using the standard XWiki config for intellij idea, it auto formatted the two xmls into a wrong style. I had to deactivate the feature and fix it by hand in b7d3008 .

…IWYG by default * Codestyle Co-authored-by: Marius Dumitru Florea <[email protected]>

…IWYG by default * Renamed the editor config option

…IWYG by default * Codestyle

surli · 2025-01-10T10:41:28Z

...xwiki-platform-ckeditor/xwiki-platform-ckeditor-ui/src/main/resources/CKEditor/EditSheet.xml


    var config = {
      filebrowserUploadUrl: uploadDisabled ? '' : getUploadURL(sourceDocument, 'filebrowser'),
+      startupFocus,


shouldn't it be startupFocus: startupFocus? it's a key: value config object no?

It's called "shorthand property name" see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#property_definitions (syntax sugar to reduce boilerplate).

I didn't know this notation well enough to use it either ^^'
#3694 (comment)

ok thanks for the information, I'm not sure I'd have changed it since IMO it's not helping much reading but that's fine :)

mflorea

Looks good, with just a small indentation issue.

mflorea · 2025-01-10T14:43:51Z

...ingo-skin/xwiki-platform-flamingo-skin-resources/src/main/resources/flamingo/commentfield.vm

@@ -72,8 +72,10 @@

 <label for="$name">$services.localization.render('core.viewers.comments.add.comment.label')</label>
 #initRequiredSkinExtensions()
-  ## This startupFocus parameter is used by the CKEditor WYSIWYG editor.
-  #set ($startupFocus = true)
+## This startupFocus parameter is used by the CKEditor WYSIWYG editor.


Suggested change

## This startupFocus parameter is used by the CKEditor WYSIWYG editor.

## This startupFocus parameter is used by the CKEditor WYSIWYG editor.

Wrong indentation.

Thanks! Fixed in 27ead97 👍

…IWYG by default * Fixed indentation

…IWYG by default (#3694) * Added a comment to explain a line of code that doesn't work with CKEditor anymore * Added a CKE config change so that it gets focus when loaded. * Implemented a solution that does not assume there's CKEditor * Fixed codestyle --------- Co-authored-by: Marius Dumitru Florea <[email protected]> (cherry picked from commit 957d0e5)

XWIKI-17664: When adding an annotation the focus should be inside WYS…

2360eec

…IWYG by default * Added a comment to explain a line of code that doesn't work with CKEditor anymore * Added a CKE config change so that it gets focus when loaded.

Sereza7 added the backport stable-16.10.x label Nov 26, 2024

Sereza7 assigned surli Nov 27, 2024

surli reviewed Dec 3, 2024

View reviewed changes

Sereza7 marked this pull request as draft December 3, 2024 12:41

Sereza7 added 6 commits December 12, 2024 10:35

XWIKI-17664: When adding an annotation the focus should be inside WYS…

8ba454d

…IWYG by default * (WIP)

XWIKI-17664: When adding an annotation the focus should be inside WYS…

8306db2

…IWYG by default * Reverted unwanted change (WIP)

XWIKI-17664: When adding an annotation the focus should be inside WYS…

c2fbd00

…IWYG by default (WIP)

XWIKI-17664: When adding an annotation the focus should be inside WYS…

7ac4b35

…IWYG by default * Implemented a solution that does not assume there's CKEditor

XWIKI-17664: When adding an annotation the focus should be inside WYS…

368e04a

…IWYG by default * Fixed codestyle

XWIKI-17664: When adding an annotation the focus should be inside WYS…

14dee2d

…IWYG by default * Removed uneeded change

Sereza7 marked this pull request as ready for review December 17, 2024 16:01

Sereza7 requested review from surli and mflorea December 17, 2024 16:01

mflorea requested changes Dec 18, 2024

View reviewed changes

mflorea reviewed Dec 18, 2024

View reviewed changes

Sereza7 and others added 4 commits December 30, 2024 17:22

Merge branch 'xwiki:master' into XWIKI-17664

c90ab7e

XWIKI-17664: When adding an annotation the focus should be inside WYS…

10d3272

…IWYG by default * Codestyle Co-authored-by: Marius Dumitru Florea <[email protected]>

XWIKI-17664: When adding an annotation the focus should be inside WYS…

359f650

…IWYG by default * Renamed the editor config option

XWIKI-17664: When adding an annotation the focus should be inside WYS…

b7d3008

…IWYG by default * Codestyle

Sereza7 requested a review from mflorea January 3, 2025 10:23

surli reviewed Jan 10, 2025

View reviewed changes

mflorea approved these changes Jan 10, 2025

View reviewed changes

XWIKI-17664: When adding an annotation the focus should be inside WYS…

27ead97

…IWYG by default * Fixed indentation

surli merged commit 957d0e5 into xwiki:master Jan 15, 2025

github-actions bot mentioned this pull request Jan 15, 2025

[Backport stable-16.10.x] XWIKI-17664: When adding an annotation the focus should be inside WYSIWYG by default #3801

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XWIKI-17664: When adding an annotation the focus should be inside WYSIWYG by default #3694

XWIKI-17664: When adding an annotation the focus should be inside WYSIWYG by default #3694

Sereza7 commented Nov 26, 2024

Sereza7 commented Nov 27, 2024

surli Dec 3, 2024

Sereza7 Dec 3, 2024 •

edited

Loading

surli Dec 3, 2024

mflorea Dec 11, 2024

Sereza7 commented Dec 17, 2024 •

edited

Loading

mflorea Dec 18, 2024

Sereza7 Jan 3, 2025

surli Jan 10, 2025

mflorea Jan 10, 2025

Sereza7 Jan 14, 2025

surli Jan 15, 2025

mflorea left a comment

mflorea Jan 10, 2025

Sereza7 Jan 14, 2025

-    #set ($startupFocus = true)
+    #set ($wysiwygEditorConfig = {
+      'startupFocus': true
+    })

	## This startupFocus parameter is used by the CKEditor WYSIWYG editor.
	## This startupFocus parameter is used by the CKEditor WYSIWYG editor.

XWIKI-17664: When adding an annotation the focus should be inside WYSIWYG by default #3694

XWIKI-17664: When adding an annotation the focus should be inside WYSIWYG by default #3694

Conversation

Sereza7 commented Nov 26, 2024

Jira URL

Changes

Description

Clarifications

Screenshots & Video

Executed Tests

Expected merging strategy

Sereza7 commented Nov 27, 2024

Choose a reason for hiding this comment

Sereza7 Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sereza7 commented Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mflorea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sereza7 Dec 3, 2024 •

edited

Loading

Sereza7 commented Dec 17, 2024 •

edited

Loading