XWIKI-22495: Missing text content in the resource picker dropdown #3612

Sereza7 · 2024-10-30T16:00:37Z

Jira URL

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

Changes

Description

Added a translated title/content to the dropdown toggle
Updated the style to allow for the use of .sr-only in this specific context (overriding CKEditor CSS reset)

Clarifications

The CKEditor CSS reset works on .cke_reset_all *. Pretty much anything more specific than .sr-only is enough to take priority over it. I decided to go with a local definition (only useful for this specific element). If any other sr-only elements are needed in our CKEditor UI, we might want to write a style rule for all of them with .cke_reset_all .sr-only.

Screenshots & Video

No visual changes in between before and after the PR, except for the title tooltip appearing when hovering the button.
Here is what the UI would look like after the changes proposed in this PR:

Executed Tests

Manual tests on a local instance, see screenshot above.
As far as I can see, integration-tests should not be impacted by this change.

Expected merging strategy

Prefers squash: Yes
Backport on branches:
- 15.10.x
- 16.4.x

* Added a translated title/content to the dropdown toggle * Updated the style to allow for the use of .sr-only in this specific context (overriding CKEditor CSS reset)

manuelleduc · 2024-12-13T09:13:24Z

...rm-ckeditor/xwiki-platform-ckeditor-plugins/src/main/webjar/xwiki-resource/resourcePicker.js

@@ -35,7 +36,9 @@ define('resourcePicker', [
          '<button type="button" class="resourceType btn btn-default">' +
            '<span class="icon">' +
          '</button>' +
-          '<button type="button" class="btn btn-default dropdown-toggle" data-toggle="dropdown">' +
+          '<button type="button" class="btn btn-default dropdown-toggle" data-toggle="dropdown" ' +
+            'title="' + translations.get('dropdown.toggle.title') + '">' +


is the result of translation.get html safe or does it needs an xml escaping?

https://github.com/search?q=repo%3Axwiki%2Fxwiki-platform%20translations.get(&type=code

There's no other use where we just use it as plain HTML. I'm pretty sure using it in Element.text escapes it.
From what I can see at

xwiki-platform/xwiki-platform-core/xwiki-platform-localization/xwiki-platform-localization-webjar/src/main/webjar/l10n.js

Lines 52 to 63 in 76b6530

var getTranslation = function(key) {

var translation = this[key];

if (typeof translation === 'string') {

// Naive implementation for message parameter substitution that suits our current needs.

for (var i = 1; i < arguments.length; i++) {

translation = translation.replace(new RegExp('\\{' + (i - 1) + '\\}', 'g'), arguments[i]);

}

} else {

translation = key;

}

return translation;

};

the translation function doesn't care about escaping (which is correct as far as I know).
I couldn't find a proper way to escape things directly in here (except implementing a barebones HTML escaper ^^').
I decided to fill up the template with the translations using jquery methods. These methods escape their inputs so it's safe.

See edc6d87

Here is what a quick test of the updated PR gave me (I didn't properly move the translations to my instance - which is why it's somewhat broken in the screenshot.):

manuelleduc · 2024-12-13T09:14:25Z

...m-ckeditor/xwiki-platform-ckeditor-plugins/src/main/webjar/xwiki-resource/resourcePicker.css

@@ -19,6 +19,18 @@
 */
 #template('colorThemeInit.vm')

+/* We need to redefine this class to have better priority than the CKEditor CSS reset. */


I'd be interested to have an explanation of why this class is needed.
What happens when this CSS is not defined?

If this CSS is not defined, we cannot use the XWiki defined class .sr-only to hide visually elements that are only useful for screen readers.
This is an issue because it's a class we should be able to rely on in all our codebase.

The CKEditor reset takes precedence over our definition of .sr-only and decides the style of this element:

Updated the comment to provide more detail in 5c7dfef 👍

* Escaped the two translations added.

* Updated CSS comment

manuelleduc · 2024-12-31T15:31:06Z

...m-ckeditor/xwiki-platform-ckeditor-plugins/src/main/webjar/xwiki-resource/resourcePicker.css

+/* We need to redefine this class to have better priority than the CKEditor CSS reset. 
+Without this higher priority redefinition, the elements with this class are still shown. */


It is not exactly clear to what is redefined, and why exactly this redefinition is needed.

I updated the comment in 3fe586f to be more explicit. Let me know if there's still something unclear.

For maintainability, I avoided providing links in the comment.

manuelleduc

lgtm, maybe a quick check from @mflorea as well?

* Updated CSS comment

mflorea · 2025-01-10T14:27:07Z

...m-ckeditor/xwiki-platform-ckeditor-plugins/src/main/webjar/xwiki-resource/resourcePicker.css

+  overflow: hidden;
+  clip: rect(0, 0, 0, 0);
+  border: 0;
+}


If any other sr-only elements are needed in our CKEditor UI, we might want to write a style rule for all of them with .cke_reset_all .sr-only.

I think I prefer the generic rule because there's a high chance we'll need this on other parts of CKEditor UI. Maybe put this in CKEditor.EditSheet .

mflorea · 2025-01-10T14:29:35Z

...rm-ckeditor/xwiki-platform-ckeditor-plugins/src/main/webjar/xwiki-resource/resourcePicker.js

+    '<button type="button" class="btn btn-default dropdown-toggle" data-toggle="dropdown">' +
+    '<span class="sr-only"></span>' +


Would be nice to keep the indentation.

mflorea · 2025-01-10T14:31:37Z

...rm-ckeditor/xwiki-platform-ckeditor-plugins/src/main/webjar/xwiki-resource/resourcePicker.js

+    resourcePicker.find('button.dropdown-toggle').first().attr('title', translations.get('dropdown.toggle.title'));
+    resourcePicker.find('button.dropdown-toggle .sr-only').first().text(translations.get('dropdown.toggle.title'));


Suggested change

resourcePicker.find('button.dropdown-toggle').first().attr('title', translations.get('dropdown.toggle.title'));

resourcePicker.find('button.dropdown-toggle .sr-only').first().text(translations.get('dropdown.toggle.title'));

resourcePicker.find('button.dropdown-toggle').attr('title', translations.get('dropdown.toggle.title'));

resourcePicker.find('button.dropdown-toggle .sr-only').text(translations.get('dropdown.toggle.title'));

No need to limit the collection to the first item.

XWIKI-22495: Missing text content in the resource picker dropdown

d49ceb2

* Added a translated title/content to the dropdown toggle * Updated the style to allow for the use of .sr-only in this specific context (overriding CKEditor CSS reset)

Sereza7 assigned manuelleduc Oct 30, 2024

manuelleduc self-requested a review December 13, 2024 08:07

manuelleduc reviewed Dec 13, 2024

View reviewed changes

Sereza7 added 3 commits December 18, 2024 11:12

Merge branch 'xwiki:master' into XWIKI-22495

fdbebba

XWIKI-22495: Missing text content in the resource picker dropdown

edc6d87

* Escaped the two translations added.

XWIKI-22495: Missing text content in the resource picker dropdown

5c7dfef

* Updated CSS comment

manuelleduc added the backport stable-15.10.x Used for automatic backport to 15.10.x branch. label Dec 31, 2024

manuelleduc reviewed Dec 31, 2024

View reviewed changes

manuelleduc approved these changes Dec 31, 2024

View reviewed changes

manuelleduc requested a review from mflorea December 31, 2024 15:31

Sereza7 added 2 commits January 3, 2025 11:39

XWIKI-22495: Missing text content in the resource picker dropdown

3fe586f

* Updated CSS comment

XWIKI-22495: Missing text content in the resource picker dropdown

9bc5095

* Updated CSS comment

Sereza7 added backport stable-16.10.x and removed backport stable-15.10.x Used for automatic backport to 15.10.x branch. labels Jan 3, 2025

mflorea requested changes Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XWIKI-22495: Missing text content in the resource picker dropdown #3612

XWIKI-22495: Missing text content in the resource picker dropdown #3612

Sereza7 commented Oct 30, 2024

manuelleduc Dec 13, 2024

Sereza7 Dec 18, 2024

manuelleduc Dec 13, 2024

Sereza7 Dec 18, 2024

Sereza7 Dec 18, 2024

manuelleduc Dec 31, 2024

Sereza7 Jan 3, 2025

manuelleduc left a comment

mflorea Jan 10, 2025

mflorea Jan 10, 2025

mflorea Jan 10, 2025

	var getTranslation = function(key) {
	var translation = this[key];
	if (typeof translation === 'string') {
	// Naive implementation for message parameter substitution that suits our current needs.
	for (var i = 1; i < arguments.length; i++) {
	translation = translation.replace(new RegExp('\\{' + (i - 1) + '\\}', 'g'), arguments[i]);
	}
	} else {
	translation = key;
	}
	return translation;
	};

		/* We need to redefine this class to have better priority than the CKEditor CSS reset.
		Without this higher priority redefinition, the elements with this class are still shown. */

		'<button type="button" class="btn btn-default dropdown-toggle" data-toggle="dropdown">' +
		'<span class="sr-only"></span>' +

		resourcePicker.find('button.dropdown-toggle').first().attr('title', translations.get('dropdown.toggle.title'));
		resourcePicker.find('button.dropdown-toggle .sr-only').first().text(translations.get('dropdown.toggle.title'));

XWIKI-22495: Missing text content in the resource picker dropdown #3612

Are you sure you want to change the base?

XWIKI-22495: Missing text content in the resource picker dropdown #3612

Conversation

Sereza7 commented Oct 30, 2024

Jira URL

Changes

Description

Clarifications

Screenshots & Video

Executed Tests

Expected merging strategy

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

Choose a reason for hiding this comment

manuelleduc left a 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