-
-
Notifications
You must be signed in to change notification settings - Fork 555
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-20996: AWM entry edit forms don't have labels. #3584
base: master
Are you sure you want to change the base?
Changes from 13 commits
6eff29e
14d4148
4b7fe5c
b06e52a
85773a0
6477c85
00583b6
e565116
71292c4
9e02c72
5c7cd93
b6a9fb9
2e76e37
e805384
7cb96fc
c9a33a3
6a45848
f0a99b8
23d77a5
2ee99f1
c030a53
be46a71
a1d296b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this really the correct ID? Shouldn't this number be based on some variable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I thought that the content and title fields were unique. Looking for a way to access this variable. AFAIR it's not straightforward and will probably require a hackish solution. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, it's just stored in the I just used |
||
#if (!$useWysiwygEditor) | ||
<div id="xwikieditcontentinner"> | ||
## The tool bar may have an entry to insert an HTML macro. Make sure it doesn't break the HTML macro we are currently in. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change looks wrong/would need some motivation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment and similar resolution as #3584 (comment) |
||
#set ($value = $tdoc.title) | ||
#if ("$!value" == '') | ||
#set ($value = $tdoc.documentReference.name) | ||
#end | ||
#end | ||
{{html clean="false"}} | ||
<input type="text" name="$name" value="$!escapetool.xml($value)" | ||
<input id="${prefix}$name" type="text" name="$name" value="$!escapetool.xml($value)" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is missing escaping. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
## 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,7 +130,17 @@ 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'); | ||
// If we cannot find any kind of editInput, then we're in a weird edge case | ||
// and we don't want to apply any of the following changes. | ||
if(!editInput) { return; } | ||
Sereza7 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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')); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a UUID to this id attribute in be46a71 . I don't like using random to set something as an ID (might introduce hard to reproduce inconsistencies/bugs), but as far as I understand, we don't really have a choice if we want to be sure that the id is unique here. For the sake of testing, I reverted the unique ID we provide for the |
||
// 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')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 👍
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.
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(); | ||
|
@@ -148,11 +158,15 @@ define('editableProperty', ['jquery', 'xwiki-meta'], function($, xcontext) { | |
.next('.editableProperty-editor').filter(':visible').trigger('xwiki:actions:cancel').hide(); | ||
editableProperty.find('.editableProperty-save, .editableProperty-cancel').hide(); | ||
editableProperty.find('.editableProperty-edit').show(); | ||
// Remove the for attribute from the label, resetting it to its default state | ||
Sereza7 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
editableProperty.find('label').removeAttr('for'); | ||
}; | ||
|
||
var save = function(editableProperty) { | ||
// Disable the save and cancel actions while the property is being saved. | ||
editableProperty.find('.editableProperty-save, .editableProperty-cancel').addClass('disabled'); | ||
// Remove the for attribute from the label, resetting it to its default state | ||
Sereza7 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
editableProperty.find('label').removeAttr('for'); | ||
// Show progress notification message. | ||
var notification = new XWiki.widgets.Notification(l10n['core.editors.saveandcontinue.notification.inprogress'], | ||
'inprogress'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking for a while to see what I did wrong, it seems like this PR doesn't especially break this. As of now, the wiki editor toolbar with inline AWM field edition is broken ( it replaces the whole HTML )...
I just pushed a small fix in c030a53 that should make things okay once the regression is handled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2025-01-15.18-08-19.mp4
This regression has been in XWiki standard for more than a few months ^^'
I just tested XWiki 14.10.22 and it's already there (see video above)