Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

[Bug] Enabling accessibility feature (example talkback) causes app to crash on loading tabs tray #14540

Closed
abhijitvalluri opened this issue Aug 31, 2020 · 5 comments
Assignees
Labels
access Accessibility: Talkback, HW keyboard/mouse, braile display etc. b:crash Crashes Fenix: should link to Sentry, Crash-Stats or GPlay info 🐞 bug Crashes, Something isn't working, .. E3 Estimation Point: average, 2 - 3 days eng:qa:verified QA Verified

Comments

@abhijitvalluri
Copy link
Contributor

abhijitvalluri commented Aug 31, 2020

Steps to reproduce

  1. Enable TalkBack accessibility feature in Android settings.
  2. Open firefox app.
  3. Open the tabs tray by clicking on the tab count on the URL bar
  4. App crashes!

Expected behavior

I expect the app to not crash when accessibility features are enabled and you use the tabs tray in the app.

Actual behavior

The app crashes when the tabs tray is opened with the accessibilty feature, such as TalkBack, enabled.
It appears that the culprit for this crash is this chunk of code: https://github.com/mozilla-mobile/fenix/blob/master/app/src/main/java/org/mozilla/fenix/tabtray/TabTrayView.kt#L188

if (hasAccessibilityEnabled) {
    tabsAdapter.notifyDataSetChanged()
}

Somehow when notifyDataSetChanged() is called, I get an IndexOutOfBounds exception. When I remove that code, there is no crash. I am not sure what accessibility feature notify data set changed method call provides.

The following is the error:

2020-08-31 17:50:34.173 28972-28972/network.novak.fenix.debug E/AndroidRuntime: FATAL EXCEPTION: main
    Process: network.novak.fenix.debug, PID: 28972
    java.lang.IndexOutOfBoundsException: Index: 3, Size: 3
        at java.util.ArrayList.get(ArrayList.java:437)
        at mozilla.components.browser.tabstray.TabsAdapter.onBindViewHolder(TabsAdapter.kt:52)
        at org.mozilla.fenix.tabtray.FenixTabsAdapter.onBindViewHolder(FenixTabsAdapter.kt:67)
        at org.mozilla.fenix.tabtray.FenixTabsAdapter.onBindViewHolder(FenixTabsAdapter.kt:59)
        at org.mozilla.fenix.tabtray.FenixTabsAdapter.onBindViewHolder(FenixTabsAdapter.kt:22)
        at androidx.recyclerview.widget.RecyclerView$Adapter.bindViewHolder(RecyclerView.java:7308)
... (truncated)

Device information

  • Android device: Android 10, Pixel 2
  • Fenix version: Issue still exists on latest Firefox production and latest Firefox Nightly (Nightly 200831 06:01 (Build #2015761139)) and also on latest master (commit 4a00131)

┆Issue is synchronized with this Jira Task

@abhijitvalluri abhijitvalluri added the 🐞 bug Crashes, Something isn't working, .. label Aug 31, 2020
@github-actions github-actions bot added the needs:triage Issue needs triage label Aug 31, 2020
@liuche liuche removed the needs:triage Issue needs triage label Aug 31, 2020
@liuche
Copy link
Contributor

liuche commented Aug 31, 2020

Could you take a look at this @mcarare? I'm wondering if this is some fallout from the a11y changes we were making - I was also seeing these show up in the Google Play pre-launch report https://sentry.prod.mozaws.net/operations/fenix-fennec/?query=is%3Aunresolved+sealed

@kbrosnan kbrosnan added access Accessibility: Talkback, HW keyboard/mouse, braile display etc. b:crash Crashes Fenix: should link to Sentry, Crash-Stats or GPlay info labels Aug 31, 2020
@abhijitvalluri
Copy link
Contributor Author

As per this webpage on Android Developers Documentation: https://developer.android.com/reference/androidx/recyclerview/widget/ConcatAdapter

When an added adapter calls one of the notify methods, ConcatAdapter properly offsets values before reporting it back to the RecyclerView. If an adapter calls RecyclerView.Adapter.notifyDataSetChanged(), ConcatAdapter also calls RecyclerView.Adapter.notifyDataSetChanged() as calling RecyclerView.Adapter.notifyItemRangeChanged(int, int) will confuse the RecyclerView. You are highly encouraged to to use SortedList or ListAdapter to avoid calling RecyclerView.Adapter.notifyDataSetChanged().

So, it looks like calling notifyDataSetChanged is not a good idea with ConcatAdapter perhaps. For some reason doing this is causing a call in FenixTabAdapter's onBindViewHolder with a position value that is more than the number of tabs. What I am noticing is that it is calling this method with a position value that is upto 2 times the number of tabs available. So, if I have 3 tabs open, then this method is called with position = 0 to 5, and if I have 2 tabs open, position is 0 to 3 and so on. Hope that helps.

@liuche liuche added the eng:qa:needed QA Needed label Sep 1, 2020
@mcarare
Copy link
Contributor

mcarare commented Sep 1, 2020

I will take a look at this. It was working just fine , but there were a great deal of changes in Tab Tray since I have implemented the fix for this: #11542.
Including adding the ConcatAdapter. According to documentation, using this adapter breaks a11y functionality:

CollectionInfo and CollectionItemInfo will no longer be populated by default.

The question is if we want to populate this info manually or completely remove code. It will mean screen readers will never announce tab position in list .

@mcarare mcarare self-assigned this Sep 1, 2020
mcarare added a commit to mcarare/fenix that referenced this issue Sep 1, 2020
@Diana-Rus
Copy link

Hi, I can reproduce the issue on Nightly 9/1 with:

Note:

  • Didn't encounter the issue when there was no tab in tabs tray
  • I've encounter the issue by having at least one tab opened in tabs tray and than following the STR.
  • It can be encountered as well when you follow the STR, with an extra step, after step2 you add a tab and then continue with step 3:
    STR
  1. Enable TalkBack accessibility feature in Android settings.
  2. Open firefox app.
  3. Make a search after a site.
  4. Open the tabs tray by clicking on the tab count on the URL bar
    Actual Result: App crashes!
Check GIF

20200901-172345-2

Removing QA-needed until issue is fixed.

@sflorean
Copy link
Contributor

Verified as fixed on Nightly 9/10 with the following devices:

  • Samsung Galaxy Note 10 (Android 10)
  • Google Pixel 3 (Android 11)

Note: I was able to reproduce the crash on Beta and Release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
access Accessibility: Talkback, HW keyboard/mouse, braile display etc. b:crash Crashes Fenix: should link to Sentry, Crash-Stats or GPlay info 🐞 bug Crashes, Something isn't working, .. E3 Estimation Point: average, 2 - 3 days eng:qa:verified QA Verified
Projects
None yet
Development

No branches or pull requests

8 participants