-
-
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 19 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 |
---|---|---|
|
@@ -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')); | ||
}).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)