Fix: Fallback value of title used for component feedback when _feedback.title not set (509)

[cahirodoherty-learningpool]

Fixes #509

Fix

• Fallback value of title used for component feedback when _feedback.title not set

[oliverfoster]

Could we resolve these issues please?
@kirsty-hames @joe-allen-89 @cahirodoherty-learningpool

These fallback keep coming and going:

• Added originally by Cahir Fix: PartlyCorrect feedback fallback values #416
• Removed by Kirsty and Joe Fix: remove component diplayTitle and Title fallbacks for feedbackTitle #428

It seems it must be possible to leave the feedback title empty in order for altTitle to work. Invariably most components will have either a displayTitle or a title so if either property is used as a default for feedback title, then altTitle never gets applied.

Why should the feedback title default to the question displayTitle or title?

The only way I can reconcile this in my head is to set the schema feedback title to default to {{#if displayTitle}}{{{displayTitle}}}{{else}}{{{title}}}{{/if}} and compile it with the model, such that the feedback title can be set both ways:

    const feedbackConfig = {
      altTitle: feedback.altTitle ||
        Adapt.course.get('_globals')._accessibility.altFeedbackTitle ||
        '',
      title: Handlebars.compile(feedback.title || '', this.toJSON()),
      _classes: feedback._classes,
      ...(isLegacyConfig
        ? getLegacyConfigObject()
        : getConfigObject()
      )
    };

[cahirodoherty-learningpool]

It seems it must be possible to leave the feedback title empty in order for altTitle to work. Invariably most components will have either a displayTitle or a title so if either property is used as a default for feedback title, then altTitle never gets applied.

Yes, I'm a little confused as to the actual code around altTitle and altFeedbackTitle. altFeedbackTitle is the schema property, but I don't think the code actually ever reads that; Instead we have https://github.com/adaptlearning/adapt-contrib-core/blob/master/js/models/questionModel.js#L287, but altTitle won't exist to be read at that point. I think its just a typo, and not central to your argument here Oliver, but worth noting.

Why should the feedback title default to the question displayTitle or title?

This is driven by user expectations from how it was working before and the helper text found on many components, e.g. https://github.com/adaptlearning/adapt-contrib-mcq/blob/master/properties.schema#L243

[kirsty-hames]

Yes, I'm a little confused as to the actual code around altTitle and altFeedbackTitle. altFeedbackTitle is the schema property, but I don't think the code actually ever reads that; Instead we have https://github.com/adaptlearning/adapt-contrib-core/blob/master/js/models/questionModel.js#L287, but altTitle won't exist to be read at that point. I think its just a typo, and not central to your argument here Oliver, but worth noting.

altFeedbackTitle is a global title, set once in course.json rather than per component feedback. altTitle is set per question feedback and overrides altFeedbackTitle. Neither title display on screen and are used by screen readers.

Notify requires a title as this is used to label the Notify 'dialog' popup (see ticket for ref).

If we're providing a fallback title (title and/or displayTitle), then altFeedbackTitle and altTitle should be removed as these won't get applied. Before removing, is there a use case for keeping alt titles? e.g. if using inline feedback displaying a title for the question and feedback can become repetitive etc. I couldn't find the issue/PR for why altTitle got added in the first place but I have seen these used in projects in favor of displaying a title. Do people have a preference?

[oliverfoster]

@kirsty-hames

altTitle original pr and issue

Including @chris-steele

[cahirodoherty-learningpool]

Yes, I'm a little confused as to the actual code around altTitle and altFeedbackTitle. altFeedbackTitle is the schema property, but I don't think the code actually ever reads that; Instead we have https://github.com/adaptlearning/adapt-contrib-core/blob/master/js/models/questionModel.js#L287, but altTitle won't exist to be read at that point. I think its just a typo, and not central to your argument here Oliver, but worth noting.

altFeedbackTitle is a global title, set once in course.json rather than per component feedback. altTitle is set per question feedback and overrides altFeedbackTitle. Neither title display on screen and are used by screen readers.

Notify requires a title as this is used to label the Notify 'dialog' popup (see ticket for ref).

If we're providing a fallback title (title and/or displayTitle), then altFeedbackTitle and altTitle should be removed as these won't get applied. Before removing, is there a use case for keeping alt titles? e.g. if using inline feedback displaying a title for the question and feedback can become repetitive etc. I couldn't find the issue/PR for why altTitle got added in the first place but I have seen these used in projects in favor of displaying a title. Do people have a preference?

Taking a look at the hbs file, it seems we are reading the models altTitle in the elseif. I think this should actually be altFeedbackTitle as that is the custom property pushed onto the model based on feedback.altTitle || _globals.altFeedbackTitle if we are going to keep altTitle. Also, the difference in the title and altTitle html is minimal. Perhaps we can move all the setting of display logic into the View to help clarify our fallbacks.

I'll add this to the current PR for a bit of a cleanup, but we should definitely resolve what our fallbacks are and in which order

[cahirodoherty-learningpool]

I've updated the PR to simplify the hbs by pushing display logic more on the view. We still have the decision to make over the fields we sequentially fall back on for the feedbackTitle field that gets resolved here though, but hopefully it's now easier to see that order

[kirsty-hames]

I've updated the PR to simplify the hbs by pushing display logic more on the view. We still have the decision to make over the fields we sequentially fall back on for the feedbackTitle field that gets resolved here though, but hopefully it's now easier to see that order

Thanks @cahirodoherty-learningpool, this is much clearer. When I update to your latest commit however no title is rendering for me regardless which title I set (title, altTitle or altFeedbackTitle).

In terms of order, I'd propose the following:
_feedback.title with default of 'Feedback'. If left empty, no visual title will display, only an aria-label title will render (altTitle or altFeedbackTitle).
_feedback.altTitle with no default. Optional if question specific alt text is required.
_globals._accessibility.altFeedbackTitle with default of 'Feedback'. Ensures the Feedback always has a label for accessibility.

Reviewing some recent courses, I find feedback title text is one of the following:
Feedback,
Content specific title,
Blank (with an aria-label 'Feedback').

If we want to use the component title/displayTitle as a fallback I'm not sure how we would add this other than providing a config option. If so, is using title/displayTitle as a fallback typically done globally for a course or per question instance?

[joe-allen-89]

👍

[oliverfoster]

🎉 This PR is included in version 6.46.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀