Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Accessibility with overlaying popups #136

Merged
merged 3 commits into from
Sep 15, 2024

Conversation

IeuanWalker
Copy link
Contributor

@IeuanWalker IeuanWalker commented Sep 12, 2024

This PR fixes 2 issues -

  • Views list constantly growing

    • When I first implemented accessibility, I didn't realise that AndroidMopups is essentially a singleton/ static class. So currently the Views list (used to keep track of pages accessibility was applied to), is constantly growing every time a popup is opened. I thought each popup would have its own class/ list. And when the popup was removed the class/ list would go with it, but that is not the case.
    • Meaning currently memory usage of the app would/ could grow every time a popup is opened. I doubt it's much of an issue as this would have been the case with the XF.
    • It also means the performance of opening/ closing a popup constantly gets worse, as each time it loops the ever-growing list of views.
  • The current implementation of accessibility only works for 1 popup open at any one time. This is because whenever a popup is removed it resets the accessibility handling on all the pages in the views list.

    • f.e. I have a settings page, that opens a popup where the user can enter some settings, on that popup it does an API call, which I then popup a loading popup. When the loading popup is removed, it resets the accessibility handling on all the pages it has been applied to. Meaning even though the settings popup is still visible, screenreaders/ keyboard users can interact with the settings page behind it.
  • Underlying popups are never treated for accessibility like pages. Meaning if a popup is open on top of another popup, then screenreader/ keyboard users will be able to interact with both.

Fix

The fix applied fixes both issues. Rather than storing a single list of views, it now creates a dictionary where the Key is the popup and the value is a list of views it altered when it opened. Also added a check to add any underlying popups to the view list to be treated for accessibility.

When the popup is closed/removed it only loops the views associated with its key and then removes itself from the dictionary.

To avoid duplicate page handling, it now checks to see if accessibility has already been handled for the page, if it has then it's ignored.

@LuckyDucko LuckyDucko merged commit 6e556f4 into LuckyDucko:master Sep 15, 2024
1 check passed
@IeuanWalker IeuanWalker deleted the Fix/Accessibility branch September 16, 2024 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants