Editing form display bug

Submitted by papi Jo on Sat, 03/13/2021 - 18:09
papi Jo's picture
Forums: 
Developers

When editing H5P activities with list fields, the default label of each new field is automatically replaced with the (cleaned) content of the 1st text field it contains.
Example: MultiChoice
The default label for each potential answer is "ADD OPTION". When a new "option", i.e. a new answer is added, the "ADD OPTION" label is replaced with its (cleaned) answer text.

This is OK, as it makes things clear when the fields are Collapsed.

HOWEVER...
The same re-naming mechanism is applied to group fields where it is not needed and even confusing.

Example.-
Each MCQ answer field contains a group field labelled "Tips and feedback". That label also gets re-named with the contents of the first text field it contents, i.e. the "Tip text" field.

The Tips and feedback field should retain its title, never get its title from its first text field content.

On the other hand, group fields containing only one text field OR no text field (of course) do not display this unwanted display behaviour.
Examples of group fields containing only one text field: - Essay / Keywords / Variations


- Interactive Video / Summary / Set of statements / Statement
and Interactive Video / Summary / Set of statements / Tip

Case of group field with a label:
A much better display behaviour can be found in Video / Accessibility / Add video track because the first text field in the Track element is clearly marked "Track label"

In conclusion

This display bug has been in existence, AFAIK, since the very beginning of H5P, and it's nagging me each time I author a content instance. I hope the H5P team will find the time to fix it any time soon.

 

otacke's picture

otacke

Sun, 03/14/2021 - 08:57

Hi!

What makes you think this is a bug? The code at h ttps://github.com/h5p/h5p-editor-php-library/blob/master/scripts/h5peditor-group.js#L236-L302 rather leads me to believe this is intended behavior - although I am not saying it should be this way.

Best,

Oliver 

papi Jo's picture

papi Jo

Sun, 03/14/2021 - 18:17

You are right, Oliver, this is not a bug in the sense that it does not break anything. I used the word "bug" for want of a better word. It is certainly the intended behaviour, but I maintain that that intended behaviour is wrong, from an ergonomics point of view.

Thanks for pointing me to https://github.com/h5p/h5p-editor-php-library/blob/master/scripts/h5pedi... which I will study carefully and hopefully suggest changes.

otacke's picture

otacke

Mon, 03/15/2021 - 10:30

Hi Papi Jo!

"but I maintain that that intended behaviour is wrong, from an ergonomics point of view"

Where in "I am not saying it should be this way" did you get the impression that I was trying to convince you otherwise?

Best,

Oliver 

BV52's picture

BV52

Mon, 03/15/2021 - 15:22

Hi Papi Jo,

Thank you for the observations/suggestions I'll pass this along to the H5P core team.

-BV

papi Jo's picture

papi Jo

Sun, 04/04/2021 - 18:56

The following fix is based on the "importance" semantics value. I have tested it on as many H5P contents as possible and it does work as expected. Here's the patch:

diff --git a/scripts/h5peditor-group.js b/scripts/h5peditor-group.js
index 2c15696..6fcf859 100644
--- a/scripts/h5peditor-group.js
+++ b/scripts/h5peditor-group.js
@@ -248,13 +248,15 @@ ns.Group.prototype.findSummary = function () {
     var widget = ns.getWidgetName(child.field);
 
     if (widget === 'text' || widget === 'html') {
-      if (params !== undefined && params !== '') {
+        // Do not replace a group's name with the contents of its first text child if that child's importance is low.
+      if (params !== undefined && params !== '' && child.field.importance !== 'low') {
         summary = params.replace(/(<([^>]+)>)/ig, "");
       }
 
       child.$input.change(function () {
         var params = (that.hasSingleChild() && !that.isSubContent()) ? that.params : that.params[child.field.name];
-        if (params !== undefined && params !== '') {
+        // Do not replace a group's name with the contents of its first text child if that child's importance is low.
+        if (params !== undefined && params !== '' && child.field.importance !== 'low') {
           that.setSummary(params.replace(/(<([^>]+)>)/ig, ""));
         }
       });

Degrange

Sat, 04/24/2021 - 17:41

This bug is really annoying.

I am not sure a fix based on importance is the best. Maybe just a attribute 'noSummary' could be set to field to prevent this behaviour. It seems more logical, and better for compatibility.

 

Degrange

Sat, 04/24/2021 - 17:39

> On the other hand, group fields containing only one text field OR no text field (of course) do not display this unwanted display behaviour.

I had issue where group field containing only one element acting as if it was the element itself (i.e. instead of the field value being {foo:'bar'} in will directly be equal to 'bar'). I don't know why (maybe it's an issue from my side, I didn't tested in a clean situation). Maybe this issue prevent the issue of your post.