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.

Sign up for GitHub

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-20996: AWM entry edit forms don't have labels. #3584

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
+32 −12
Open

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6eff29e
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Nov 28, 2023
14d4148
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Nov 29, 2023
4b7fe5c
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Dec 1, 2023
b06e52a
Merge branch 'xwiki:master' into XWIKI-20996
Sereza7 Jul 1, 2024
85773a0
Merge branch 'xwiki:master' into XWIKI-20996
Sereza7 Oct 22, 2024
6477c85
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Oct 22, 2024
00583b6
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Oct 22, 2024
e565116
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Nov 19, 2024
71292c4
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Nov 19, 2024
9e02c72
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Nov 19, 2024
5c7cd93
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Nov 19, 2024
b6a9fb9
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Nov 19, 2024
2e76e37
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Nov 19, 2024
e805384
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Nov 26, 2024
7cb96fc
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Nov 26, 2024
c9a33a3
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Nov 26, 2024
6a45848
Merge branch 'xwiki:master' into XWIKI-20996
Sereza7 Dec 17, 2024
f0a99b8
XWIKI-20996: AWM entry edit forms don't have labels
Sereza7 Dec 17, 2024
23d77a5
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Dec 17, 2024
2ee99f1
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Jan 15, 2025
c030a53
XWIKI-20996: AWM entry edit forms don't have labels.
Sereza7 Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ...inutes/xwiki-platform-appwithinminutes-ui/src/main/resources/AppWithinMinutes/Content.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@
#set ($toolBar = "#template('simpleedittoolbar.vm')")
$!toolBar.replace('{{', '{{')
## Display a simple textarea.
<textarea id="$id" cols="80" rows="25" name="$name">$escapetool.xml($editedDocument.content)</textarea>
<textarea id="$id" cols="80" rows="25" name="$name" ##
aria-label="$escapetool.xml($services.localization.render('platform.appwithinminutes.content.edit.label'))">##
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 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.

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 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.

$escapetool.xml($editedDocument.content)</textarea>
#end
#if ($useWysiwygEditor)
$!services.edit.syntaxContent.wysiwyg($editedDocument.content, $editedDocument.syntax, {
Expand Down
1 change: 1 addition & 0 deletions ...nminutes/xwiki-platform-appwithinminutes-ui/src/main/resources/AppWithinMinutes/Title.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
#end
{{html clean="false"}}
<input type="text" name="$name" value="$!escapetool.xml($value)"
aria-label="$escapetool.xml($services.localization.render('platform.appwithinminutes.title.edit.label'))"
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

image

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

Copy link
Contributor

Choose a reason for hiding this comment

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

So there are two cases:

  1. We're inside the AWM editor that uses ClassEditSheet.
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

## The default value for an AppWithinMinutes field should be optional so we make only the actual page title
## mandatory and not the template title, which holds the default title value.
#if ($name == 'title' && $xwiki.getSpacePreference('xwiki.title.mandatory') == 1)required #end
Expand Down
2 changes: 2 additions & 0 deletions ...s/xwiki-platform-appwithinminutes-ui/src/main/resources/AppWithinMinutes/Translations.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ platform.appwithinminutes.appsLiveTable.doc.author=Last author
platform.appwithinminutes.appsLiveTable.doc.date=Update date
platform.appwithinminutes.appsLiveTable._actions=Actions
platform.appwithinminutes.grantSpaceAdminRightsSaveComment=Grant space administration rights.
platform.appwithinminutes.title.edit.label=Edit the title of this entry.
platform.appwithinminutes.content.edit.label=Edit the content of this entry.
Sereza7 marked this conversation as resolved.
Show resolved Hide resolved

### AWM Applications panel entry
platform.appwithinminutes.appPanelCreateEntry=Create your own!
Expand Down
9 changes: 8 additions & 1 deletion ...eb/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/edit/editableProperty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,14 @@ define('editableProperty', ['jquery', 'xwiki-meta'], function($, xcontext) {
// Allow others to enhance the editor.
$(document).trigger('xwiki:dom:updated', {'elements': editor.toArray()});
// Focus the first visible input.
editor.find(':input').filter(':visible').focus();
var editInput = editor.find(':input').filter(':visible');
editInput.focus();
// 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'));
}
Comment on lines +140 to +143
Copy link
Member

Choose a reason for hiding this comment

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

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.

// Bind the label to the newly generated edit input
Sereza7 marked this conversation as resolved.
Show resolved Hide resolved
editableProperty.find('label').attr('for',editInput.attr('id'));
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Sereza7 marked this conversation as resolved.
Show resolved Hide resolved
}).catch(() => {
new XWiki.widgets.Notification(l10n['web.editableProperty.editFailed'], 'error');
return Promise.reject();
Expand Down