Skip to content

Commit

Permalink
Fix oppia#4790 Talkback will read toolbar title clickable (oppia#5027)
Browse files Browse the repository at this point in the history
Fix oppia#4790 For toolbars that have marquee effects, add additional checks
so that the effect will only be enabled when the accessibility service
is not on.
Also updated the existing test for the marquee effect. The test now
checks if the effect is set and activated correctly depending on the
accessibility service status.
Update the test setting to ensure that test behavior is correctly
simulated.

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation

Whenever users use vocal assisting tools such as talkback, the
accessibility service we inject in the field will be enabled. We will
choose either to use a marquee effect for visual display, or just pure
vocal display when the user enables the service.

For a toolbar that doesn't have an individual view for the toolbar
title, setting 'onNavgation..listener' will make the whole toolbar
clickable, and talkback will read "double click.."-
While we want to keep the navigation function, we can use getChildAt(0)
to set the listener to just the nav icon.
 
<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
<!-- Delete these section if this PR does not include UI-related
changes. -->

- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
Exploration Activity:

- Normal:


https://github.com/oppia/oppia-android/assets/74568012/74f6eaf8-8560-4d1a-bfb4-d378177e31b6

- Talkback On:


https://github.com/oppia/oppia-android/assets/74568012/11285aee-ed5e-4355-97b1-c18df646efd7

Topic Fragment:

- Talkback On:


https://github.com/oppia/oppia-android/assets/74568012/f8cec635-b1c3-4540-afcb-990a42a9a38b


## Issue with Espresso test:
when readerOff, toolbar.title.click() behaves different from manual
test.

- Video record for manual test 

Listener function is set and is called when click is performed.
Regardless of title length, is selected will always be modified.


https://github.com/oppia/oppia-android/assets/74568012/7f9a04b6-e3e3-4803-9465-89543b091458


- Video record for espresso test 
     Listener function is set but not called when click is performed.
     The isSelected attribute does not been modified after click
    


https://github.com/oppia/oppia-android/assets/74568012/c2984b5f-c9fe-4a35-a36f-27d05a935210

## Solution to Solve Error in Espresso Tests.

- Setting the accessibility service before starting the activity solves
accessibility service not correctly set except in TopicFragmentTest.
**Reason** The set listener code in the app executes before setting the
accessibility service in the test.

- Add markSpotlightSeen before the start of the activity.
**Reason** The spotlight might cause the toolbar unable to reach to
simulate a click.
  • Loading branch information
XichengSpencer authored Sep 15, 2023
1 parent b643538 commit 89e8032
Show file tree
Hide file tree
Showing 10 changed files with 515 additions and 125 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import org.oppia.android.R
import org.oppia.android.app.activity.ActivityScope
import org.oppia.android.app.translation.AppLanguageResourceHandler
import org.oppia.android.databinding.LicenseTextViewerActivityBinding
import org.oppia.android.util.accessibility.AccessibilityService
import javax.inject.Inject

/** The presenter for [LicenseTextViewerActivity]. */
Expand All @@ -14,6 +15,7 @@ class LicenseTextViewerActivityPresenter @Inject constructor(
private val activity: AppCompatActivity,
private val resourceHandler: AppLanguageResourceHandler
) {
@Inject lateinit var accessibilityService: AccessibilityService

/** Handles onCreate() method of the [LicenseTextViewerActivity]. */
fun handleOnCreate(dependencyIndex: Int, licenseIndex: Int) {
Expand Down Expand Up @@ -44,8 +46,10 @@ class LicenseTextViewerActivityPresenter @Inject constructor(
(activity as LicenseTextViewerActivity).finish()
}

binding.licenseTextViewerActivityToolbarTitle.setOnClickListener {
binding.licenseTextViewerActivityToolbarTitle.isSelected = true
if (!accessibilityService.isScreenReaderEnabled()) {
binding.licenseTextViewerActivityToolbarTitle.setOnClickListener {
binding.licenseTextViewerActivityToolbarTitle.isSelected = true
}
}

if (getLicenseTextViewerFragment() == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import org.oppia.android.domain.exploration.ExplorationDataController
import org.oppia.android.domain.oppialogger.OppiaLogger
import org.oppia.android.domain.survey.SurveyGatingController
import org.oppia.android.domain.translation.TranslationController
import org.oppia.android.util.accessibility.AccessibilityService
import org.oppia.android.util.data.AsyncResult
import org.oppia.android.util.data.DataProviders.Companion.toLiveData
import javax.inject.Inject
Expand All @@ -61,6 +62,9 @@ class ExplorationActivityPresenter @Inject constructor(
private val resourceHandler: AppLanguageResourceHandler,
private val surveyGatingController: SurveyGatingController
) {
@Inject
lateinit var accessibilityService: AccessibilityService

private lateinit var explorationToolbar: Toolbar
private lateinit var explorationToolbarTitle: TextView
private lateinit var profileId: ProfileId
Expand Down Expand Up @@ -89,7 +93,7 @@ class ExplorationActivityPresenter @Inject constructor(
parentScreen: ExplorationActivityParams.ParentScreen,
isCheckpointingEnabled: Boolean
) {
binding = DataBindingUtil.setContentView<ExplorationActivityBinding>(
binding = DataBindingUtil.setContentView(
activity,
R.layout.exploration_activity
)
Expand All @@ -102,8 +106,10 @@ class ExplorationActivityPresenter @Inject constructor(
explorationToolbarTitle = binding.explorationToolbarTitle
activity.setSupportActionBar(explorationToolbar)

binding.explorationToolbarTitle.setOnClickListener {
binding.explorationToolbarTitle.isSelected = true
if (!accessibilityService.isScreenReaderEnabled()) {
binding.explorationToolbarTitle.setOnClickListener {
binding.explorationToolbarTitle.isSelected = true
}
}

binding.explorationToolbar.setNavigationOnClickListener {
Expand Down Expand Up @@ -181,7 +187,11 @@ class ExplorationActivityPresenter @Inject constructor(
activity.supportFragmentManager.beginTransaction().add(
R.id.exploration_fragment_placeholder,
ExplorationFragment.newInstance(
profileId, topicId, storyId, explorationId, readingTextSize
profileId,
topicId,
storyId,
explorationId,
readingTextSize
),
TAG_EXPLORATION_FRAGMENT
).commitNow()
Expand Down Expand Up @@ -211,7 +221,8 @@ class ExplorationActivityPresenter @Inject constructor(
}
R.id.action_help -> {
val intent = HelpActivity.createHelpActivityIntent(
activity, profileId.internalId,
activity,
profileId.internalId,
/* isFromNavigationDrawer= */false
)
fontScaleConfigurationUtil.adjustFontScale(activity, ReadingTextSize.MEDIUM_TEXT_SIZE)
Expand Down Expand Up @@ -266,7 +277,8 @@ class ExplorationActivityPresenter @Inject constructor(
// without deleting any checkpoints.
if (::oldestCheckpointExplorationId.isInitialized) {
explorationDataController.deleteExplorationProgressById(
profileId, oldestCheckpointExplorationId
profileId,
oldestCheckpointExplorationId
)
}
stopExploration(isCompletion = false)
Expand Down Expand Up @@ -340,7 +352,8 @@ class ExplorationActivityPresenter @Inject constructor(
explorationLiveData.observe(activity) {
explorationToolbarTitle.text =
translationController.extractString(
it.exploration.translatableTitle, it.writtenTranslationContext
it.exploration.translatableTitle,
it.writtenTranslationContext
)
}
}
Expand All @@ -363,7 +376,9 @@ class ExplorationActivityPresenter @Inject constructor(
return when (ephemeralExpResult) {
is AsyncResult.Failure -> {
oppiaLogger.e(
"ExplorationActivity", "Failed to retrieve answer outcome", ephemeralExpResult.error
"ExplorationActivity",
"Failed to retrieve answer outcome",
ephemeralExpResult.error
)
EphemeralExploration.getDefaultInstance()
}
Expand Down Expand Up @@ -472,7 +487,9 @@ class ExplorationActivityPresenter @Inject constructor(
}
is AsyncResult.Failure -> {
oppiaLogger.e(
"ExplorationActivity", "Failed to retrieve oldest saved checkpoint details.", it.error
"ExplorationActivity",
"Failed to retrieve oldest saved checkpoint details.",
it.error
)
}
is AsyncResult.Pending -> {} // Wait for an actual result.
Expand Down Expand Up @@ -512,41 +529,40 @@ class ExplorationActivityPresenter @Inject constructor(
private fun maybeShowSurveyDialog(profileId: ProfileId, topicId: String) {
surveyGatingController.maybeShowSurvey(profileId, topicId).toLiveData()
.observe(
activity,
{ gatingResult ->
when (gatingResult) {
is AsyncResult.Pending -> {
oppiaLogger.d("ExplorationActivity", "A gating decision is pending")
}
is AsyncResult.Failure -> {
oppiaLogger.e(
"ExplorationActivity",
"Failed to retrieve gating decision",
gatingResult.error
)
activity
) { gatingResult ->
when (gatingResult) {
is AsyncResult.Pending -> {
oppiaLogger.d("ExplorationActivity", "A gating decision is pending")
}
is AsyncResult.Failure -> {
oppiaLogger.e(
"ExplorationActivity",
"Failed to retrieve gating decision",
gatingResult.error
)
backPressActivitySelector()
}
is AsyncResult.Success -> {
if (gatingResult.value) {
val dialogFragment =
SurveyWelcomeDialogFragment.newInstance(
profileId,
topicId,
explorationId,
SURVEY_QUESTIONS
)
val transaction = activity.supportFragmentManager.beginTransaction()
transaction
.add(dialogFragment, TAG_SURVEY_WELCOME_DIALOG)
.addToBackStack(null)
.commit()
} else {
backPressActivitySelector()
}
is AsyncResult.Success -> {
if (gatingResult.value) {
val dialogFragment =
SurveyWelcomeDialogFragment.newInstance(
profileId,
topicId,
explorationId,
SURVEY_QUESTIONS
)
val transaction = activity.supportFragmentManager.beginTransaction()
transaction
.add(dialogFragment, TAG_SURVEY_WELCOME_DIALOG)
.addToBackStack(null)
.commit()
} else {
backPressActivitySelector()
}
}
}
}
)
}
}

companion object {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,16 +65,17 @@ class StoryFragmentPresenter @Inject constructor(
private lateinit var linearSmoothScroller: RecyclerView.SmoothScroller
private lateinit var profileId: ProfileId

@Inject lateinit var storyViewModel: StoryViewModel
@Inject lateinit var accessibilityService: AccessibilityService

@Inject lateinit var storyViewModel: StoryViewModel

fun handleCreateView(
inflater: LayoutInflater,
container: ViewGroup?,
internalProfileId: Int,
topicId: String,
storyId: String
): View? {
): View {
binding = StoryFragmentBinding.inflate(
inflater,
container,
Expand All @@ -89,9 +90,10 @@ class StoryFragmentPresenter @Inject constructor(
binding.storyToolbar.setNavigationOnClickListener {
(activity as StoryActivity).finish()
}

binding.storyToolbarTitle.setOnClickListener {
binding.storyToolbarTitle.isSelected = true
if (!accessibilityService.isScreenReaderEnabled()) {
binding.storyToolbarTitle.setOnClickListener {
binding.storyToolbarTitle.isSelected = true
}
}

linearLayoutManager = LinearLayoutManager(activity.applicationContext)
Expand Down Expand Up @@ -274,11 +276,17 @@ class StoryFragmentPresenter @Inject constructor(
// one.
val startPlayingProvider = if (canHavePartialProgressSaved) {
explorationDataController.startPlayingNewExploration(
profileId.internalId, topicId, storyId, explorationId
profileId.internalId,
topicId,
storyId,
explorationId
)
} else {
explorationDataController.replayExploration(
profileId.internalId, topicId, storyId, explorationId
profileId.internalId,
topicId,
storyId,
explorationId
)
}
startPlayingProvider.toLiveData().observe(fragment) { result ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import org.oppia.android.app.translation.AppLanguageResourceHandler
import org.oppia.android.databinding.TopicFragmentBinding
import org.oppia.android.domain.oppialogger.OppiaLogger
import org.oppia.android.domain.oppialogger.analytics.AnalyticsController
import org.oppia.android.util.accessibility.AccessibilityService
import org.oppia.android.util.platformparameter.EnableExtraTopicTabsUi
import org.oppia.android.util.platformparameter.PlatformParameterValue
import javax.inject.Inject
Expand All @@ -35,6 +36,9 @@ class TopicFragmentPresenter @Inject constructor(
@EnableExtraTopicTabsUi private val enableExtraTopicTabsUi: PlatformParameterValue<Boolean>,
private val resourceHandler: AppLanguageResourceHandler
) {
@Inject
lateinit var accessibilityService: AccessibilityService

private lateinit var tabLayout: TabLayout
private var internalProfileId: Int = -1
private lateinit var topicId: String
Expand All @@ -48,7 +52,7 @@ class TopicFragmentPresenter @Inject constructor(
topicId: String,
storyId: String,
isConfigChanged: Boolean
): View? {
): View {
val binding = TopicFragmentBinding.inflate(
inflater,
container,
Expand All @@ -64,11 +68,11 @@ class TopicFragmentPresenter @Inject constructor(
binding.topicToolbar.setNavigationOnClickListener {
(activity as TopicActivity).finish()
}

binding.topicToolbar.setOnClickListener {
binding.topicToolbarTitle.isSelected = true
if (!accessibilityService.isScreenReaderEnabled()) {
binding.topicToolbarTitle.setOnClickListener {
binding.topicToolbarTitle.isSelected = true
}
}

viewModel.setInternalProfileId(internalProfileId)
viewModel.setTopicId(topicId)
binding.viewModel = viewModel
Expand Down Expand Up @@ -152,7 +156,8 @@ class TopicFragmentPresenter @Inject constructor(
TopicTab.REVISION -> oppiaLogger.createOpenRevisionTabContext(topicId)
}
analyticsController.logImportantEvent(
eventContext, ProfileId.newBuilder().apply { internalId = internalProfileId }.build()
eventContext,
ProfileId.newBuilder().apply { internalId = internalProfileId }.build()
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import androidx.appcompat.app.AppCompatActivity
import androidx.appcompat.widget.Toolbar
import androidx.databinding.DataBindingUtil
import androidx.lifecycle.LiveData
import androidx.lifecycle.Observer
import androidx.lifecycle.Transformations
import org.oppia.android.R
import org.oppia.android.app.activity.ActivityScope
Expand All @@ -19,6 +18,7 @@ import org.oppia.android.domain.oppialogger.OppiaLogger
import org.oppia.android.domain.oppialogger.analytics.AnalyticsController
import org.oppia.android.domain.topic.TopicController
import org.oppia.android.domain.translation.TranslationController
import org.oppia.android.util.accessibility.AccessibilityService
import org.oppia.android.util.data.AsyncResult
import org.oppia.android.util.data.DataProviders.Companion.toLiveData
import javax.inject.Inject
Expand All @@ -32,6 +32,7 @@ class RevisionCardActivityPresenter @Inject constructor(
private val topicController: TopicController,
private val translationController: TranslationController
) {
@Inject lateinit var accessibilityService: AccessibilityService

private lateinit var revisionCardToolbar: Toolbar
private lateinit var revisionCardToolbarTitle: TextView
Expand Down Expand Up @@ -66,9 +67,12 @@ class RevisionCardActivityPresenter @Inject constructor(
binding.revisionCardToolbar.setNavigationOnClickListener {
(activity as ReturnToTopicClickListener).onReturnToTopicRequested()
}
binding.revisionCardToolbarTitle.setOnClickListener {
binding.revisionCardToolbarTitle.isSelected = true
if (!accessibilityService.isScreenReaderEnabled()) {
binding.revisionCardToolbarTitle.setOnClickListener {
binding.revisionCardToolbarTitle.isSelected = true
}
}

subscribeToSubtopicTitle()

binding.actionBottomSheetOptionsMenu.setOnClickListener {
Expand All @@ -89,14 +93,18 @@ class RevisionCardActivityPresenter @Inject constructor(
return when (itemId) {
R.id.action_options -> {
val intent = OptionsActivity.createOptionsActivity(
activity, profileId.internalId, isFromNavigationDrawer = false
activity,
profileId.internalId,
isFromNavigationDrawer = false
)
activity.startActivity(intent)
true
}
R.id.action_help -> {
val intent = HelpActivity.createHelpActivityIntent(
activity, profileId.internalId, isFromNavigationDrawer = false
activity,
profileId.internalId,
isFromNavigationDrawer = false
)
activity.startActivity(intent)
true
Expand All @@ -117,11 +125,10 @@ class RevisionCardActivityPresenter @Inject constructor(

private fun subscribeToSubtopicTitle() {
subtopicLiveData.observe(
activity,
Observer<String> {
revisionCardToolbarTitle.text = it
}
)
activity
) {
revisionCardToolbarTitle.text = it
}
}

private val subtopicLiveData: LiveData<String> by lazy {
Expand All @@ -143,7 +150,9 @@ class RevisionCardActivityPresenter @Inject constructor(
when (revisionCardResult) {
is AsyncResult.Failure -> {
oppiaLogger.e(
"RevisionCardActivity", "Failed to retrieve Revision Card", revisionCardResult.error
"RevisionCardActivity",
"Failed to retrieve Revision Card",
revisionCardResult.error
)
EphemeralRevisionCard.getDefaultInstance()
}
Expand Down
Loading

0 comments on commit 89e8032

Please sign in to comment.