From 0f80ec4abf8bdd63fe447de19dbfbce75d2b452a Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Sat, 6 Jul 2024 19:29:02 +0200 Subject: [PATCH] fix: Correctly punctuate a status content description (#808) Previous code blindly inserted commas and semi-colons as separators between the components of a content description. If some of those components were null you could have a content description that looked like "... , , , ..." or similar, and the repeated reading of "comma" by screen readers was jarring and reduced accessibility. Fix this by inserting punctuation only where necessary, building up the string piece by piece instead of using a string resource with hardcoded punctuation. Fixes #791. --- app/lint-baseline.xml | 15 +- .../pachli/adapter/StatusBaseViewHolder.kt | 149 ++++++++++++------ .../adapter/StatusDetailedViewHolder.kt | 6 +- .../java/app/pachli/util/StatusExtensions.kt | 8 +- app/src/main/res/values/donottranslate.xml | 5 - .../core/network/model/TimelineAccount.kt | 4 + 6 files changed, 122 insertions(+), 65 deletions(-) diff --git a/app/lint-baseline.xml b/app/lint-baseline.xml index 3f141f1cf..73132caf4 100644 --- a/app/lint-baseline.xml +++ b/app/lint-baseline.xml @@ -716,6 +716,17 @@ column="51"/> + + + + @@ -1196,7 +1207,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~"> diff --git a/app/src/main/java/app/pachli/adapter/StatusBaseViewHolder.kt b/app/src/main/java/app/pachli/adapter/StatusBaseViewHolder.kt index 3538d469f..bcced8ac1 100644 --- a/app/src/main/java/app/pachli/adapter/StatusBaseViewHolder.kt +++ b/app/src/main/java/app/pachli/adapter/StatusBaseViewHolder.kt @@ -740,7 +740,7 @@ abstract class StatusBaseViewHolder protected constructor(i ) setRebloggingEnabled(actionable.rebloggingAllowed(), actionable.visibility) setSpoilerAndContent(viewData, statusDisplayOptions, listener) - setDescriptionForStatus(viewData, statusDisplayOptions) + setContentDescriptionForStatus(viewData, statusDisplayOptions) // Workaround for RecyclerView 1.0.0 / androidx.core 1.0.0 // RecyclerView tries to set AccessibilityDelegateCompat to null @@ -759,41 +759,86 @@ abstract class StatusBaseViewHolder protected constructor(i } } - private fun setDescriptionForStatus( - viewData: T, - statusDisplayOptions: StatusDisplayOptions, - ) { + /** Creates and sets the content description for the status. */ + private fun setContentDescriptionForStatus(viewData: T, statusDisplayOptions: StatusDisplayOptions) { val (_, _, account, _, _, _, _, createdAt, editedAt, _, reblogsCount, favouritesCount, _, reblogged, favourited, bookmarked, sensitive, _, visibility) = viewData.actionable - val description = context.getString( - R.string.description_status, - account.displayName, - getContentWarningDescription(context, viewData), - if (TextUtils.isEmpty(viewData.spoilerText) || !sensitive || viewData.isExpanded) viewData.content else "", - getCreatedAtDescription(createdAt, statusDisplayOptions), - editedAt?.let { context.getString(R.string.description_post_edited) } ?: "", - getReblogDescription(context, viewData), - viewData.username, - if (reblogged) context.getString(R.string.description_post_reblogged) else "", - if (favourited) context.getString(R.string.description_post_favourited) else "", - if (bookmarked) context.getString(R.string.description_post_bookmarked) else "", - getMediaDescription(context, viewData), - visibility.description(context), - getFavsText(favouritesCount), - getReblogsText(reblogsCount), + + // Build the content description using a string builder instead of a string resource + // as there are many places where optional ";" and "," are needed. + // + // The full string will look like (content in parentheses is optional), + // + // account.name + // (; contentWarning) + // ; (content) + // (, poll) + // , relativeDate + // (, editedAt) + // (, reblogDescription) + // , username + // (, "Reblogged") + // (, "Favourited") + // (, "Bookmarked") + // (, "n Favorite") + // (, "n Boost") + val description = StringBuilder().apply { + append(account.name) + + getContentWarningDescription(context, viewData)?.let { append("; ", it) } + + append("; ") + + // Content is optional, and hidden if there are spoilers or the status is + // marked sensitive, and it has not been expanded. + if (TextUtils.isEmpty(viewData.spoilerText) || !sensitive || viewData.isExpanded) { + append(viewData.content, ", ") + } + viewData.actionable.poll?.let { - pollView.getPollDescription( - from(it), - statusDisplayOptions, - numberFormat, - absoluteTimeFormatter, + append( + pollView.getPollDescription( + from(it), + statusDisplayOptions, + numberFormat, + absoluteTimeFormatter, + ), + ", ", ) - } ?: "", - ) + } + + append(getCreatedAtDescription(createdAt, statusDisplayOptions)) + + editedAt?.let { append(", ", context.getString(R.string.description_post_edited)) } + + getReblogDescription(context, viewData)?.let { append(", ", it) } + + append(", ", viewData.username) + + if (reblogged) { + append(", ", context.getString(R.string.description_post_reblogged)) + } + + if (favourited) { + append(", ", context.getString(R.string.description_post_favourited)) + } + if (bookmarked) { + append(", ", context.getString(R.string.description_post_bookmarked)) + } + getMediaDescription(context, viewData)?.let { append(", ", it) } + + append("; ") + + visibility.description(context)?.let { append(it) } + getFavouritesCountDescription(favouritesCount)?.let { append(", ", it) } + getReblogsCountDescription(reblogsCount)?.let { append(", ", it) } + } + itemView.contentDescription = description } - protected fun getFavsText(count: Int): CharSequence { - if (count <= 0) return "" + /** @return "n Favourite", for use in a content description. */ + protected fun getFavouritesCountDescription(count: Int): CharSequence? { + if (count <= 0) return null val countString = numberFormat.format(count.toLong()) return HtmlCompat.fromHtml( @@ -802,8 +847,9 @@ abstract class StatusBaseViewHolder protected constructor(i ) } - protected fun getReblogsText(count: Int): CharSequence { - if (count <= 0) return "" + /** @return "n Boost", for use in a content description. */ + protected fun getReblogsCountDescription(count: Int): CharSequence? { + if (count <= 0) return null val countString = numberFormat.format(count.toLong()) return HtmlCompat.fromHtml( @@ -826,7 +872,10 @@ abstract class StatusBaseViewHolder protected constructor(i cardView ?: return val (_, _, _, _, _, _, _, _, _, _, _, _, _, _, _, _, sensitive, _, _, attachments, _, _, _, _, _, poll, card) = viewData.actionable - if (cardViewMode !== CardViewMode.NONE && attachments.isEmpty() && poll == null && card != null && + if (cardViewMode !== CardViewMode.NONE && + attachments.isEmpty() && + poll == null && + card != null && !TextUtils.isEmpty(card.url) && (!sensitive || expanded) && (!viewData.isCollapsible || !viewData.isCollapsed) @@ -883,34 +932,32 @@ abstract class StatusBaseViewHolder protected constructor(i return attachments.all { it.isPreviewable() } } - private fun getReblogDescription(context: Context, status: IStatusViewData): CharSequence { + /** @return "{account.username} boosted", for use in a content description. */ + private fun getReblogDescription(context: Context, status: IStatusViewData): String? { return status.rebloggingStatus?.let { context.getString(R.string.post_boosted_format, it.account.username) - } ?: "" + } } - private fun getMediaDescription(context: Context, status: IStatusViewData): CharSequence { - if (status.actionable.attachments.isEmpty()) return "" + /** @return "Media: {0-n descriptions}", for use in a content description. */ + private fun getMediaDescription(context: Context, status: IStatusViewData): String? { + if (status.actionable.attachments.isEmpty()) return null - val mediaDescriptions = - status.actionable.attachments.fold(StringBuilder()) { builder: StringBuilder, (_, _, _, _, _, description): Attachment -> - if (description == null) { - val placeholder = - context.getString(R.string.description_post_media_no_description_placeholder) - return@fold builder.append(placeholder) - } else { - builder.append("; ") - return@fold builder.append(description) - } - } - return context.getString(R.string.description_post_media, mediaDescriptions) + val missingDescription = context.getString(R.string.description_post_media_no_description_placeholder) + + val mediaDescriptions = status.actionable.attachments.map { + it.description ?: missingDescription + } + + return context.getString(R.string.description_post_media, mediaDescriptions.joinToString(", ")) } - private fun getContentWarningDescription(context: Context, status: IStatusViewData): CharSequence { + /** @return "Content warning: {spoilerText}", for use in a content description. */ + private fun getContentWarningDescription(context: Context, status: IStatusViewData): String? { return if (!TextUtils.isEmpty(status.spoilerText)) { context.getString(R.string.description_post_cw, status.spoilerText) } else { - "" + null } } } diff --git a/app/src/main/java/app/pachli/adapter/StatusDetailedViewHolder.kt b/app/src/main/java/app/pachli/adapter/StatusDetailedViewHolder.kt index 12b502f5c..b1e6cf7ad 100644 --- a/app/src/main/java/app/pachli/adapter/StatusDetailedViewHolder.kt +++ b/app/src/main/java/app/pachli/adapter/StatusDetailedViewHolder.kt @@ -38,7 +38,7 @@ class StatusDetailedViewHolder( visibilityIcon?.also { val alignment = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) DynamicDrawableSpan.ALIGN_CENTER else DynamicDrawableSpan.ALIGN_BASELINE val visibilityIconSpan = ImageSpan(it, alignment) - sb.setSpan(visibilityIconSpan, 0, visibilityString.length, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE) + sb.setSpan(visibilityIconSpan, 0, visibilityString?.length ?: 0, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE) } val metadataJoiner = context.getString(R.string.metadata_joiner) sb.append(" ") @@ -82,13 +82,13 @@ class StatusDetailedViewHolder( listener: StatusActionListener, ) { if (reblogCount > 0) { - binding.statusReblogs.text = getReblogsText(reblogCount) + binding.statusReblogs.text = getReblogsCountDescription(reblogCount) binding.statusReblogs.show() } else { binding.statusReblogs.hide() } if (favCount > 0) { - binding.statusFavourites.text = getFavsText(favCount) + binding.statusFavourites.text = getFavouritesCountDescription(favCount) binding.statusFavourites.show() } else { binding.statusFavourites.hide() diff --git a/app/src/main/java/app/pachli/util/StatusExtensions.kt b/app/src/main/java/app/pachli/util/StatusExtensions.kt index b8a5e49c6..f5fb563b5 100644 --- a/app/src/main/java/app/pachli/util/StatusExtensions.kt +++ b/app/src/main/java/app/pachli/util/StatusExtensions.kt @@ -30,17 +30,17 @@ import app.pachli.core.network.model.Status // store resources (yet). /** - * @return A description for this visibility, or "" if it's null or [Status.Visibility.UNKNOWN]. + * @return A description for this visibility, or null if it's null or [Status.Visibility.UNKNOWN]. */ -fun Status.Visibility?.description(context: Context): CharSequence { - this ?: return "" +fun Status.Visibility?.description(context: Context): CharSequence? { + this ?: return null val resource: Int = when (this) { Status.Visibility.PUBLIC -> R.string.description_visibility_public Status.Visibility.UNLISTED -> R.string.description_visibility_unlisted Status.Visibility.PRIVATE -> R.string.description_visibility_private Status.Visibility.DIRECT -> R.string.description_visibility_direct - Status.Visibility.UNKNOWN -> return "" + Status.Visibility.UNKNOWN -> return null } return context.getString(resource) } diff --git a/app/src/main/res/values/donottranslate.xml b/app/src/main/res/values/donottranslate.xml index 53b424675..833ab2cf1 100644 --- a/app/src/main/res/values/donottranslate.xml +++ b/app/src/main/res/values/donottranslate.xml @@ -192,11 +192,6 @@ never - - - %1$s; %2$s; %3$s, %15$s %4$s, %5$s, %6$s; %7$s, %8$s, %9$s, %10$s, %11$s; %12$s, %13$s, %14$s - - @string/duration_5_min @string/duration_30_min diff --git a/core/network/src/main/kotlin/app/pachli/core/network/model/TimelineAccount.kt b/core/network/src/main/kotlin/app/pachli/core/network/model/TimelineAccount.kt index d0c571752..ec03ce19f 100644 --- a/core/network/src/main/kotlin/app/pachli/core/network/model/TimelineAccount.kt +++ b/core/network/src/main/kotlin/app/pachli/core/network/model/TimelineAccount.kt @@ -40,6 +40,10 @@ data class TimelineAccount( val emojis: List? = emptyList(), ) { + /** + * The account's [displayName], falling back to [localUsername] if + * [displayName] is null or empty. + */ @Suppress("DEPRECATION") val name: String get() = if (displayName.isNullOrEmpty()) {