Missing titles for feedback Notify popups

[swashbuck]

Subject of the issue

I believe this is a bug, but let me know if it is intended behavior.

If _feedback.altTitle is not present, is an empty value, or is the same value as _feedback.title, the title is not shown in the popup. However, if _feedback.altTitle is different from _feedback.title (either different text or just a single space character), then the title appears as expected.

You can test this with the core course from running adapt create course and viewing the Question Components topic.

This may be caused by #509 .

Your environment

• FW 5.38.2
• Core 6.46.7

Steps to reproduce

Configure the _feedback object in one of the following ways:

1. _feedback.title is the same as _feedback.altTitle
2. _feedback.altTitle is removed completely
3. _feedback.altTitle is an empty string ""

For example:

"_feedback": {
  "title": "Feedback",
  "altTitle": "Feedback",
  "correct": "Correct feedback text.",
  "_incorrect": {
    "notFinal": "",
    "final": "Incorrect feedback text."
  }
},

Expected behaviour

"Feedback" title should appear.

Actual behaviour

No title appears

[oliverfoster]

Have you read #510 ? Does it cover your bug?

[swashbuck]

Have you read #510 ? Does it cover your bug?

@oliverfoster I've read through that ticket, but it's a bit confusing. There's also the comment from @kirsty-hames that was left unresolved, from what I can tell.

If _feedback.altTitle is meant to be for screenreaders, why is the _feedback.title visibility dependent on it (see "Steps to reproduce" above)? It just seems unintuitive to me. And if the developer wants to intentionally hide the Notify title, this seems like a weird way to go about it.

[oliverfoster]

Is the supposition that altTitle should change the way title is read but that title should still remain visible when supplied?
I had thought that altTitle were only to be used when no title was defined.

It's probably either/or because the notify role=dialog has an aria-labelledby requirement that must be configured with the id of a readable area. A visible title would read secondarily to the altTitle but not as the notify dialog label.

There's also the #510 (comment) from @kirsty-hames that was left unresolved, from what I can tell.

Cahir implicitly resolved it with a subsequent pr I think. I was positive that there was a further comment from him, but I can't see it now.

[swashbuck]

Is the supposition that altTitle should change the way title is read but that title should still remain visible when supplied? I had thought that altTitle were only to be used when no title was defined.

@oliverfoster I believe so, yeah. For instance, why is the title hidden completely if altTitle is absent or empty but there is still a value for title? Shouldn't it show the title value rather than hiding it? I can see this causing confusion in the AAT if course creators remove the altTitle value.

[oliverfoster]

Weird.

adapt-contrib-core/js/models/questionModel.js

Line 286 in 6a0014b

const altTitle = feedback.altTitle || Adapt.course.get('_globals')._accessibility.altFeedbackTitle;

It seems that altTitle defaults to _globals._accessibility.altFeedbackTitle when no _feedback.altTitle is present.

adapt-contrib-core/js/models/questionModel.js

Line 287 in 6a0014b

const title = feedback.title || this.get('title') || altTitle || '';

And that title can only be altTitle when no _feedback.title nor component.title is set.

adapt-contrib-core/js/models/questionModel.js

Line 290 in 6a0014b

isAltTitle: (title === altTitle),

Then going forward altTitle is used for isAltTitle, so there must have been no _feedback.title nor component.title for this to be true or title was explicitly set to the _feedback.title value or the _globals._accessibility.altFeedbackTitle value where no _feedback.title is given.

It then goes almost straight through tutor and notify view into the notifyPopup template

adapt-contrib-core/templates/notifyPopup.hbs

Lines 16 to 20 in 6a0014b

	{{#if title}}
	<div class="notify__title{{#if isAltTitle}} aria-label{{/if}}" id="notify-heading">
	<div class="notify__title-inner" role="heading" aria-level="{{a11y_aria_level _id 'notify' _ariaLevel}}">
	{{{compile title}}}
	</div>

Is it that when _feedback.altTitle is empty it equals _globals._accessibility.altFeedbackTitle which set to "Feedback", which makes altTitle "Feedback" and that then equals _feedback.title which is also "Feedback" in your example?

It's difficult without stepping through the values in the debugger and seeing the output html.

[swashbuck]

Is it that when _feedback.altTitle is empty it equals _globals._accessibility.altFeedbackTitle which set to "Feedback", which makes altTitle "Feedback" and that then equals _feedback.title which is also "Feedback" in your example?

@oliverfoster Yes, that seems to be the explanation for all 3 cases under "Steps to reproduce" above. The only way that the title will appear is if altTitle is set to something other than title. It can even be a space character like " " since that would technically be different from title.

[oliverfoster]

We could probably solve it by changing the isAltTitle derivation to the following:

const isAltTitle = altTitle && !feedback.title && !this.get('title');

Where it's only true when there is no feedback title or component tile and there is a _feedback.altTitle or _globals._accessibility.altFeedbackTitle configured.

We could go one step further and force it to be true if _feedback.altTitle is explicitly set? That would make that property very clear. It depends if we think altTitle should take precedence of title when set or if altTitle should only be used when title isn't set.

[swashbuck]

We could go one step further and force it to be true if _feedback.altTitle is explicitly set? That would make that property very clear. It depends if we think altTitle should take precedence of title when set or if altTitle should only be used when title isn't set.

I think I prefer this option. So, a developer / course creator would only set a value for altTitle if we do not want to show a title in the Notify. If we do this, I'd suggest removing any altTitle values from the sample course since it is currently confusing.

Curious what others think, especially those involved with some of these recent changes. @kirsty-hames @cahirodoherty-learningpool @joe-allen-89 @joe-replin

[kirsty-hames]

We could go one step further and force it to be true if _feedback.altTitle is explicitly set? That would make that property very clear. It depends if we think altTitle should take precedence of title when set or if altTitle should only be used when title isn't set.

I think altTitle should only be used when a title isn't set. I think on screen and accessible text should be consistent. So yes to forcing isAltTitle to be true if _feedback.altTitle is explicitly set.

I think I prefer this option. So, a developer / course creator would only set a value for altTitle if we do not want to show a title in the Notify. If we do this, I'd suggest removing any altTitle values from the sample course since it is currently confusing.

I'd agree. Yes to removing any altTitle values from the sample course.

[swashbuck]

Would it make sense to treat _feedback.altTitle like a component's title and then _feedback.title like a component's displayTitle? This would mean:

• If _feedback.title is set, a title is displayed with this value
• If _feedback.title is not set, it falls back to the component title/displayTitle and that is displayed
• If _feedback.altTitle is set, it is used as an aria label
• If both are set, _feedback.title is displayed and _feedback.altTitle is used as an aria label
• Essentially, _feedback.altTitle would never be seen by users as it would only be used by screenreaders. I'm not sure if this was the original purpose?

[oliverfoster]

No, it would not make sense to use altTitle as an aria-label when there is a visible title.

[kirsty-hames]

• If _feedback.title is set, a title is displayed with this value
• If _feedback.title is not set, it falls back to the component title/displayTitle and that is displayed
• If _feedback.altTitle is set, it is used as an aria label

This makes sense to me. With the following amend to the last point.

If _feedback.altTitle is set, it is used as an aria label and no visual title is displayed.

[oliverfoster]

@swashbuck are you fixing?

[swashbuck]

Working on this now. I've already created an issue to remove the altTitle values from the example course:

adaptlearning/adapt_framework#3566

[swashbuck]

It would also be useful to update the feedback titles section in the Question Model wiki while we're at it.

https://github.com/adaptlearning/adapt_framework/wiki/Question-model#feedback-titles