XWIKI-20996: AWM entry edit forms don't have labels. #3584

Sereza7 · 2024-10-22T15:20:50Z

Jira URL

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

Changes

Description

Added localized and escaped aria labels for both the title and content fields.
Made the labels actually target their inputs by adding a for attribute value to them.

Clarifications

The content field, at least for the XS wysiwyg editor CKEditor, is hidden as soon as the editor is loaded. Therefore its label is mostly for the sake of validating tests, I doubt any user would hit it in realistic conditions.
It's a bit more important for the title field, but unfortunately the generated ID is wrong so we cannot really rely on explicit labelling. Duplicating the label element for implicit labelling looks like too much changes for such a small use case. Example of wrong ID: testApp.Code.testAppClass_0_title1 contains multiple dots in the middle which is not correct HTML and might break some media.

Screenshots & Video

No visible changes. Semantic changes only.
Here's the WCAG validation result on a basic application with a couple fields, a title and a content:

Executed Tests

Built changes with mvn clean install -f xwiki-platform-core/xwiki-platform-appwithinminutes/ then checked the WCAG warning reported by the CI tests with mvn clean install -f xwiki-platform-core/xwiki-platform-appwithinminutes/xwiki-platform-appwithinminutes-test/xwiki-platform-appwithinminutes-test-docker -Dxwiki.test.ui.wcag=true. In the WCAG report generated with the changes proposed in this PR, the two warnings that caused the ticket we're trying to solve here are not reported anymore.

Manual tests did confirm these results (see screenshot above).

Expected merging strategy

Prefers squash: Yes
Backport on branches:
- 16.4.X
- 15.10.X
  These changes are not related to any other recent changes AFAIK, it should be easy to backport them on both of those branches.

* Added a label to the editable properties in edit mode. Note: this does not solve the reported issue

* Fixed the lack of ids on a couple 'hard coded' fields

* Added localized and escaped aria labels for both the title and content fields.

* Escaped the content aria-label

michitux

I don't agree with the approach of adding aria-label attributes. AWM generates visible labels for these fields, we need to make sure that these labels are properly connected to the inputs instead of adding additional, invisible labels. This would improve the situation for everybody as it would allow clicking on the visible label to focus the input.

michitux · 2024-11-07T09:19:10Z

...inminutes/xwiki-platform-appwithinminutes-ui/src/main/resources/AppWithinMinutes/Content.xml

@@ -61,7 +61,9 @@
      #set ($toolBar = "#template('simpleedittoolbar.vm')")
      $!toolBar.replace('{{', '&amp;#123;&amp;#123;')
      ## Display a simple textarea.
-      &lt;textarea id="$id" cols="80" rows="25" name="$name"&gt;$escapetool.xml($editedDocument.content)&lt;/textarea&gt;
+      &lt;textarea id="$id" cols="80" rows="25" name="$name" ##
+      aria-label="$escapetool.xml($services.localization.render('platform.appwithinminutes.content.edit.label'))"&gt;##


Is this really necessary? Shouldn't this always be used in a form that displays a label? What seems to be rather happening I think is that this textarea has the wrong id/the code for generating the label assumes the wrong id.

Addressed in the latest changes by #3584 (comment) 👍

Note that the there was a comment to make sure the ID wouldn't be changed because it broke style/functionalities. I tried it out and everything seemed fine. With a quick search I could find that there's no specific style anymore.

michitux · 2024-11-07T09:19:19Z

...thinminutes/xwiki-platform-appwithinminutes-ui/src/main/resources/AppWithinMinutes/Title.xml

@@ -53,6 +53,7 @@
  #end
  {{html clean="false"}}
  &lt;input type="text" name="$name" value="$!escapetool.xml($value)"
+    aria-label="$escapetool.xml($services.localization.render('platform.appwithinminutes.title.edit.label'))"


Same here, I wonder if this is really needed. I've just checked and indeed the title label has a for attribute with an ID that looks good (e.g., AI.Documents.Code.DocumentsClass_0_title in the index for the LLM application) but the input doesn't have the expected ID. I would suggest setting an ID attribute that matches the generated label.

When on the ClassEditSheet, the label of the field becomes itself editable. This means that what's usually a label now contains an input. This is implicit input labelling. I'm not sure HTML contains a definite way to handle such a case (label with both implicit and explicit input labelling) and I'd rather avoid meeting it.

We could "duplicate" the label but I'd rather keep things as simple as possible and just have an aria-label.

So there are two cases:

We're inside the AWM editor that uses ClassEditSheet.

We're inside the generated app.

I was primarily talking about the second case and didn't notice the first one. For the second case, I kind of agree, but to me this doesn't mean that we shouldn't consider the second case which should be the much more frequent case in which we have a visible label that doesn't contain another input that is not correctly connected with the input. For the first case, I'm wondering if we couldn't remove the for attribute on the existing label and add a sr-only second label that has the for attribute. In this first case, the input of the pretty name might also need an explicit aria-label as the content of the label element is wrong, this isn't the label but some hack for resizing the input.

Sorry, I just saw your last answer. I found a compromise:
we use the proper label in all situation except when in the AWM editor. The AWM editor removes the label and sets an aria-label for the defaultValue, pretty name and hint properties of each field. IMO having an aria label is as good as having a sr-only label element, but it's simpler to create.

This is what is implemented at #3584 (comment)

...utes/xwiki-platform-appwithinminutes-ui/src/main/resources/AppWithinMinutes/Translations.xml

michitux · 2024-11-07T09:22:07Z

...m-web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/edit/editableProperty.js

+      // Make sure the edit input has an ID, and use the name of the input as a fallback
+      if (!editInput.attr('id')) {
+        editInput.attr('id', editInput.attr('name'));
+      }
+      // Bind the label to the newly generated edit input
+      editableProperty.find('label').attr('for',editInput.attr('id'));


Shouldn't this whole code be wrapped in an if-statement if there is an editInput?

Also, this change seems to be much more general than AWM. I'm not saying it's bad, I'm just saying that it is more general.

Shouldn't there be code to remove the for attribute again from the label when editing is finished?

Shouldn't this whole code be wrapped in an if-statement if there is an editInput?

Since this only happens when the promise to retrieve the edit display of a field is achieved, I didn't expect this to happen. I added a condition to finish the function early and skip those changes though, better safe than sorry 👍

Also, this change seems to be much more general than AWM. I'm not saying it's bad, I'm just saying that it is more general.

Yup, I was navigating through multiple kinds of UI for objects and there was an issue with AWM inline edition. I tracked the cause and ended up changing things here.

Shouldn't there be code to remove the for attribute again from the label when editing is finished?

Technically it doesn't hurt to leave it. However it's better for HTML clarity to clean up the attribute when it's not useful anymore. Added a couple lines to clean this up on saving or canceling the edition

* Added comments in ClassEditSheet * Extended the input selector in _enhanceFieldDefaultValue so that the labelling would also work on select (eg. fieldType = StaticList) * Set an aria-label on default value fields, equal to the title. We don't care about the existing state, this label is always the same for all fields. * Added a translated label on the field hint/prettyName inputs in class edition mode. * Added the `for` attribute on the label even in view mode, so that the inline edition input is properly labelled. * Updated the id of the content textarea. The only style left that relied on this was unneeded (same property set by another style...). Moreover the functionality did not suffer when changing this. This allows to use the id to label the textarea instead of relying on an aria-label. * Used a proper label instead of aria-label on the Title input.

* Small quality improvement

* Removed unneeded change

* Added a safety condition to the new javascript * Removed the inline label association when the input is removed.

* Updated the translations

Sereza7 · 2024-11-19T16:17:16Z

All of the comments should have been taken into account. Last self-review and opening this PR back for review.

michitux

Thank you very much for the improvements, there are just some details left.

michitux · 2024-11-19T16:25:16Z

...thinminutes/xwiki-platform-appwithinminutes-ui/src/main/resources/AppWithinMinutes/Title.xml

    #set ($value = $tdoc.title)
    #if ("$!value" == '')
      #set ($value = $tdoc.documentReference.name)
    #end
  #end
  {{html clean="false"}}
-  &lt;input type="text" name="$name" value="$!escapetool.xml($value)"
+  &lt;input id="${prefix}$name" type="text" name="$name" value="$!escapetool.xml($value)"


This is missing escaping.

Do we need to escape all variables declared locally? This $name, just like the $prefix, is defined a few lines above and I didn't think it was necessary to escape it.
I added escaping on the name in e805384 nevertheless 👍

michitux · 2024-11-19T16:25:29Z

...thinminutes/xwiki-platform-appwithinminutes-ui/src/main/resources/AppWithinMinutes/Title.xml

@@ -45,14 +45,14 @@
    #set ($value = $xwiki.getDocument("$stringtool.removeEnd($className, 'Class')Template").title)
  #else
    ## We are editing an application entry so the title must be read from / written to the current document.
-    #set ($name = 'title')
+    #set ($name = 'title1')


This change looks wrong/would need some motivation.

Same comment and similar resolution as #3584 (comment)

...mingo-skin/xwiki-platform-flamingo-skin-resources/src/main/resources/flamingo/less/edit.less

michitux · 2024-11-19T16:27:56Z

...inminutes/xwiki-platform-appwithinminutes-ui/src/main/resources/AppWithinMinutes/Content.xml

@@ -53,8 +53,7 @@
    #set ($useWysiwygEditor = $xwiki.getUserPreference('editor') == 'Wysiwyg')
  #end
  {{html clean="false"}}
-  ## The "content" id is expected by some JavaScript and CSS code.
-  #set ($id = 'content')
+  #set ($id = "${prefix}${name}1")


Is this really the correct ID? Shouldn't this number be based on some variable?

So I thought that the content and title fields were unique.
It turns out we don't prevent this when the user edits the app. I'm not sure what are the repercussion on the results, but if one provides multiple fields for those, there will be an issue with labelling 👍

Looking for a way to access this variable. AFAIR it's not straightforward and will probably require a hackish solution.

Okay, it's just stored in the $name variable we get from the context in which those templates are used.
I could find this thanks to displayer_date.vm which is not defined the same way as those two templates, but has a similar role.
Note that in both of those templates, the value $name is overridden, we want to compute with the info from the context so we have to compute the ID at the start of the template.

I just used #set ($id = $escapetool.xml("${prefix}${name}")) on both and we're good :)
This works both on classEdition and entryEdition.

* Added escaping on the name used in the id for the title

* Updated the content and title fields displays to use the object no.

michitux

Thank you, this looks good now apart from the one escaping issue.

michitux · 2024-11-29T12:10:44Z

...inminutes/xwiki-platform-appwithinminutes-ui/src/main/resources/AppWithinMinutes/Content.xml

@@ -39,6 +39,7 @@
  <content>{{velocity}}
 #if ($type == 'edit')
  #set ($className = $object.getxWikiClass().name)
+  #set ($id = $escapetool.xml("${prefix}${name}"))


That escaping looks wrong, as the $id variable is later passed to $services.edit.syntaxContent.wysiwyg, and I would be surprised if it expected an escaped value. In the other case, where the ID is used directly for a textarea element, the escaping is needed, of course.

That escaping looks wrong, as the $id variable is later passed to $services.edit.syntaxContent.wysiwyg, and I would be surprised if it expected an escaped value.

You're right, thanks for finding this 👍 From what I remember, it would be incorrect for such a service to expect the value escaped, so we can assume we shouldn't escape it. If there's an issue, it should be solved on the service provider instead.

Addressed in f0a99b8 👍

...m-web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/edit/editableProperty.js

* Fixed escaping

* Fixed codestyle Co-authored-by: Michael Hamann <[email protected]>

Sereza7 · 2024-12-17T17:09:38Z

Thank you for your review!
Sorry for the delay in my response, I didn't notice the fixes needed would be so quick.

The PR is ready for further review or a merge.

michitux · 2025-01-07T14:23:09Z

@mflorea I saw some mention of you that real-time editing only works for the content property? Could you confirm that this PR doesn't break anything in this regard?

michitux

Looks good to me, but I would like to get some feedback from @mflorea if removing the content ID could break anything.

...m-web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/edit/editableProperty.js

mflorea · 2025-01-10T13:56:41Z

...m-web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/edit/editableProperty.js

+      // Make sure the edit input has an ID, and use the name of the input as a fallback
+      if (!editInput.attr('id')) {
+        editInput.attr('id', editInput.attr('name'));
+      }


I'm not convinced by this. The value of the name attribute can be (and often is) very generic which means it's not a good id because it can conflict with other ids on the page. I'd rather default to a random id, that can have the name as prefix.

...m-web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/edit/editableProperty.js

mflorea · 2025-01-10T14:08:13Z

...inminutes/xwiki-platform-appwithinminutes-ui/src/main/resources/AppWithinMinutes/Content.xml

-  ## The "content" id is expected by some JavaScript and CSS code.
-  #set ($id = 'content')


This is an example of JavaScript code that looks for a text area with content id https://github.com/xwiki/xwiki-platform/blob/master/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/wikieditor/wikibits.js#L57 . Have you checked that the "wiki editor" toolbar is properly loaded for the Content AWM field when the user prefers the text editor (rather than the WYSIWYG editor)?

mflorea · 2025-01-10T14:15:13Z

Looks good to me, but I would like to get some feedback from @mflorea if removing the content ID could break anything.

@michitux realtime editing is currently disabled for inline form edit mode or when editing a property in-place, even if the value is stored in the document content. So removing the content ID shouldn't have any effect on realtime editing, but it may have an effect on things like https://github.com/xwiki/xwiki-platform/blob/master/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war/src/main/webapp/resources/js/xwiki/wikieditor/wikibits.js#L57 (the "wiki editor" toolbar that is re-used by AWM for Content fields when the user prefers text editing rather than WYSIWYG editing).

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

Sereza7 and others added 7 commits November 28, 2023 11:26

XWIKI-20996: AWM entry edit forms don't have labels.

6eff29e

* Added a label to the editable properties in edit mode. Note: this does not solve the reported issue

XWIKI-20996: AWM entry edit forms don't have labels.

14d4148

* Fixed the lack of ids on a couple 'hard coded' fields

XWIKI-20996: AWM entry edit forms don't have labels.

4b7fe5c

* Fixed the lack of ids on a couple 'hard coded' fields

Merge branch 'xwiki:master' into XWIKI-20996

b06e52a

Merge branch 'xwiki:master' into XWIKI-20996

85773a0

XWIKI-20996: AWM entry edit forms don't have labels.

6477c85

* Added localized and escaped aria labels for both the title and content fields.

XWIKI-20996: AWM entry edit forms don't have labels.

00583b6

* Escaped the content aria-label

Sereza7 assigned michitux Oct 22, 2024

Sereza7 added backport stable-15.10.x Used for automatic backport to 15.10.x branch. backport stable-16.4.x labels Oct 22, 2024

Sereza7 removed the backport stable-15.10.x Used for automatic backport to 15.10.x branch. label Oct 31, 2024

michitux requested changes Nov 7, 2024

View reviewed changes

Sereza7 marked this pull request as draft November 8, 2024 16:22

Sereza7 added 6 commits November 19, 2024 14:57

XWIKI-20996: AWM entry edit forms don't have labels.

71292c4

* Small quality improvement

XWIKI-20996: AWM entry edit forms don't have labels.

9e02c72

* Small quality improvement

XWIKI-20996: AWM entry edit forms don't have labels.

5c7cd93

* Removed unneeded change

XWIKI-20996: AWM entry edit forms don't have labels.

b6a9fb9

* Added a safety condition to the new javascript * Removed the inline label association when the input is removed.

XWIKI-20996: AWM entry edit forms don't have labels.

2e76e37

* Updated the translations

Sereza7 marked this pull request as ready for review November 19, 2024 16:25

michitux requested changes Nov 19, 2024

View reviewed changes

Sereza7 added 3 commits November 26, 2024 16:39

XWIKI-20996: AWM entry edit forms don't have labels.

e805384

* Added escaping on the name used in the id for the title

XWIKI-20996: AWM entry edit forms don't have labels.

7cb96fc

* Updated the content and title fields displays to use the object no.

XWIKI-20996: AWM entry edit forms don't have labels.

c9a33a3

* Updated the content and title fields displays to use the object no.

Sereza7 requested a review from michitux December 13, 2024 11:09

michitux requested changes Dec 16, 2024

View reviewed changes

Sereza7 and others added 3 commits December 17, 2024 17:56

Merge branch 'xwiki:master' into XWIKI-20996

6a45848

XWIKI-20996: AWM entry edit forms don't have labels

f0a99b8

* Fixed escaping

XWIKI-20996: AWM entry edit forms don't have labels.

23d77a5

* Fixed codestyle Co-authored-by: Michael Hamann <[email protected]>

Sereza7 requested a review from michitux December 17, 2024 17:09

Sereza7 added backport stable-16.10.x and removed backport stable-16.4.x labels Jan 3, 2025

michitux requested a review from mflorea January 7, 2025 14:23

michitux approved these changes Jan 7, 2025

View reviewed changes

mflorea requested changes Jan 10, 2025

View reviewed changes

XWIKI-20996: AWM entry edit forms don't have labels.

2ee99f1

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XWIKI-20996: AWM entry edit forms don't have labels. #3584

XWIKI-20996: AWM entry edit forms don't have labels. #3584

Sereza7 commented Oct 22, 2024

michitux left a comment

michitux Nov 7, 2024

Sereza7 Nov 19, 2024

michitux Nov 7, 2024

Sereza7 Nov 19, 2024

michitux Nov 19, 2024

Sereza7 Nov 19, 2024

michitux Nov 7, 2024

Sereza7 Nov 19, 2024

Sereza7 commented Nov 19, 2024

michitux left a comment

michitux Nov 19, 2024

Sereza7 Nov 26, 2024

michitux Nov 19, 2024

Sereza7 Nov 26, 2024

michitux Nov 19, 2024

Sereza7 Nov 26, 2024

Sereza7 Nov 26, 2024

michitux left a comment •

edited

Loading

michitux Nov 29, 2024

Sereza7 Dec 17, 2024

Sereza7 commented Dec 17, 2024

michitux commented Jan 7, 2025

michitux left a comment

mflorea Jan 10, 2025

mflorea Jan 10, 2025

mflorea commented Jan 10, 2025

		## The "content" id is expected by some JavaScript and CSS code.
		#set ($id = 'content')

XWIKI-20996: AWM entry edit forms don't have labels. #3584

Are you sure you want to change the base?

XWIKI-20996: AWM entry edit forms don't have labels. #3584

Conversation

Sereza7 commented Oct 22, 2024

Jira URL

Changes

Description

Clarifications

Screenshots & Video

Executed Tests

Expected merging strategy

michitux 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

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

Sereza7 commented Nov 19, 2024

michitux 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

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

michitux left a comment • 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

michitux commented Jan 7, 2025

michitux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mflorea commented Jan 10, 2025

michitux left a comment •

edited

Loading