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

[TECHNICAL] Changes in accessibility role management #4466

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Aitorbp
Copy link
Collaborator

@Aitorbp Aitorbp commented Sep 9, 2024

Related Issues

App:

  • Add changelog files for the fixed issues in folder changelog/unreleased. More info here
  • Add feature to Release Notes in ReleaseNotesViewModel.kt creating a new ReleaseNote() with String resources (if required)

QA

@Aitorbp Aitorbp self-assigned this Sep 9, 2024
@Aitorbp Aitorbp linked an issue Sep 9, 2024 that may be closed by this pull request
16 tasks
@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Sep 10, 2024

Report 1:
Include contentDescription as another parameter in the setAccessibilityRole extension function, so that we avoid implementing AccessibilityDelegateCompat again, like in SortOptionsView

As I said earlier in the pr: Assigning an AccessibilityDelegate to a MenuItem is not as straightforward as assigning it to a normal view because MenuItem is not a View itself. Instead, MenuItem has an associated View via actionView, which can sometimes be null if it hasn't been manually configured.

So if we try to access an itemMenu view following, for example, this code:

view?.findViewById<View>(R.id.action_see_details)?.setAccessibilityRole( contentDescription = "${getString(R.string.actionbar_see_details)} ${getString(R.string.button_role_accessibility)}" )

Not working. Therefore I would leave it as it is and until contentDescription had no more use I would not put it in the extension function.

Likewise, we cannot remove the AccessibilityDelegateCompat from the SortOptionsView because what this method does is detect the clicks that the talkback makes and the changes that have been made at that moment. If we put this functionality in the setAccessibilityRole extension function pointing to the corresponding view, it would not detect ascending/descending state changes.

Report 2:
Check if onCreateOptionsMenu can actually be moved to FileFragment and remove it from its subclasses

I don't see a problem that now the oncreateView is created in the parent and not in the children. To give more detail to the structure, leave here what the menu would look like. In FileFragment there would be all those options that are common to all and in the children their particular cases:

FileFragment:

  1. Open with item
  2. Send item
  3. Set as available offline item
  4. Unset as available offline item

FileFragment inheritances:

FileDetailsFragment:

  1. Rename item
  2. Remove item

PreviewAudioFragment

  1. Details item

PreviewTextFragment

  1. Details item

PreviewImageFragment

  1. Details item

FileDownloadFragment: is not affected.

@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Sep 12, 2024

Research passcode visibility when introducing numbers from the keyboard

First of all, we need to mention that the operating system has internal mechanisms to manage text input interactions, both from physical and virtual keyboards. In the case of physical keyboards, when typing into a password EditText, the character is briefly displayed and then hidden. This behavior can be restricted from the Android settings.

Therefore, in our code, two functionalities coexist: first, the system behavior is executed, followed by our own implementation.

The most optimal solution we have found is to set the EditText's focusable attribute to false. This way, the system will not briefly display the digit on the screen.

@Aitorbp Aitorbp force-pushed the technical/refactor_accessibility_role branch from 8c007fd to 9a74b81 Compare September 12, 2024 09:58
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments here for you to check @Aitorbp

Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more change, and check the previous CR comments

@Aitorbp Aitorbp force-pushed the technical/refactor_accessibility_role branch from 62de37c to 58d48cc Compare September 18, 2024 12:57
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's move this to QA 🚀

@JuancaG05
Copy link
Collaborator

Here the EditTexts management in the passcode view changed, so I recommend to put a special focus on the passcode feature: check that it is correctly introduced, even when we remove some characters, etc... FYI @jesmrec 😀

@jesmrec
Copy link
Collaborator

jesmrec commented Sep 18, 2024

This task is a refactor over other task in the same milestone that already has an entry in Calens file. That means, we don't need two entries for the same feature in the same release/milestone. Please, join all together the roles management stuff in just one entry in the Calens.

@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Sep 18, 2024

This task is a refactor over other task in the same milestone that already has an entry in Calens file. That means, we don't need two entries for the same feature in the same release/milestone. Please, join all together the roles management stuff in just one entry in the Calens.

Ok, maybe removing this one is ok because this issue handle two differents issues. What do you thing ? @jesmrec

@jesmrec
Copy link
Collaborator

jesmrec commented Sep 18, 2024

Ok, maybe removing this one is ok because this issue handle two differents issues. What do you thing ? @jesmrec

it depends. If the stuff in the current PR adds something new and relevant to the work in the other, it should be added. If it is just refactor, corrections and bugfixing you are right, we could get rid of the current entry.

@jesmrec
Copy link
Collaborator

jesmrec commented Sep 18, 2024

[WIP]

Some checks over navegability in passcode view:

Open create view and ...

  • click on tab + enter -> 1 clicked
  • click on down arrow key + enter -> 1 clicked
  • Click on enter -> moved to 1 and another enter submit 1

Open unlock view and ...

  • click on tab + enter -> 1 clicked
  • click on down arrow + enter -> 1 clicked
  • click on enter -> moved to 1 and another enter submit 1

On 1 and...

  • click on tab and enter -> 2 clicked, then 3, 4,... after 0, then delete and 1 again
  • click on arrow keys -> correct navigation

@jesmrec
Copy link
Collaborator

jesmrec commented Sep 19, 2024

Ok, maybe removing this one is ok because this issue handle two differents issues. What do you thing ? @jesmrec

And anyway, we can remove this one but adding the PR reference to the existing one @Aitorbp

@Aitorbp Aitorbp force-pushed the technical/refactor_accessibility_role branch from 58d48cc to ff822d5 Compare September 19, 2024 06:32
@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Sep 19, 2024

Ok, maybe removing this one is ok because this issue handle two differents issues. What do you thing ? @jesmrec

And anyway, we can remove this one but adding the PR reference to the existing one @Aitorbp

Ok I did it.

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.

[TECHNICAL] Changes in accessibility role management
3 participants