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

[a11y] 11.4.1.2 Name-role value #4454

Merged
merged 17 commits into from
Aug 27, 2024
Merged

Conversation

Aitorbp
Copy link
Collaborator

@Aitorbp Aitorbp commented Aug 14, 2024

Related Issues

App:#4373

  • 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

QA checks: #4454 (comment)

Reports:

@Aitorbp Aitorbp self-assigned this Aug 14, 2024
@Aitorbp Aitorbp linked an issue Aug 14, 2024 that may be closed by this pull request
11 tasks
@Aitorbp Aitorbp force-pushed the feature/name_role_value_accessibility branch from 7495de5 to c0c62be Compare August 19, 2024 12:53
@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Aug 20, 2024

Some technical stuffs to take in account:

Menu

  • The link "...-Portal" was not correctly marked as a link
    correctly. The role "Link" is missing, which should be used for the semantically correct
    should be used for the semantically correct labeling of links.
  • The logo (see screenshot) was erroneously labeled as a button
    although clicking on the element opens a new page in the browser.
    opens a new page in the browser. Instead, the logo should be provided with a link element
    in order to be semantically correct.
    image
  • All elements of the menu are missing the correct role, either
    "Button" or "Link".
    image

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.

The problem is that you need to associate an actionview to the item, but this returns null. Searching on the internet it is recommended to manually assign a view to the item. But that hasn't worked either.
Also we have try to set the role in the contentDescription of the menuItem in the XML, but it isn't work because of MenuItem itself is not a direct view.
The solution we have seen is to access to contentDescription programmatically and use the attribute on the MenuItem component and not on its actionview. Here we have one restriction because this code only works for API level versions greater than 25 (it will work from Android 8 onwards).

  • The button with the account data does not have a corresponding role.
    In addition, the labeling is too long and difficult to understand. It is
    It is also unclear which view of the menu users are currently in.
    image

This element is not going to be changed due to the current behaviour is the following: when you click in this element the talkback said Header, so it is enough information to know we are in the header of the drawer. In any case, this element doesn't have a functionality, it is just information that the user can see.

Edit shared Link

  • The "Expiry date" and "Date" buttons have no role.
    image

These are not buttons, they work as informational text. In any case, these elements have been labeled with the corresponding switches in this pr #4448 related to this issue #4363

Spaces

  • Both the "Spaces" heading (button) and the "Space" buttons
    do not have a role.
    image

About the Spaces heading, at the moment we are going to set the talkback in this element like "Search button" in all toolbars.

This report is contradictory with another accessibility report #4362 we had where we were asked to put the Space title as a heading and now they tell us to give it the role of a button. I think both labels should not coexist, either it is a button or it is a heading.

But you have to know there is a issue #4452 opened to discuss about the behavior of the Toolbar and the SearchView. The proposal is divided the title and the search icon in differents elements, so when only when the search icon is clicked the search view will be opened.

Image Viewer

  • None of the items in the "More options" menu have a role.
    image

For the menu that opens from the toolbar when clicking on the three dots, we have the following cases:

FileFragment:
The onCreateOptionsMenu method has been overridden so that its subclasses don’t have to call this method themselves. Within it, a contentDescription has been added to the items that are common to all: "open with," "send," "set available offline," and "unset available offline."

Then, in these three classes (PreviewImageFragment, PreviewAudioFragment, PreviewTextFragment), which are subclasses of FileFragment, the following contentDescription has been added: "Details."

Finally, in FileDetailsFragment, the following contentDescriptions have been added: "rename" and "remove."

MainFileListFragment:
It was not possible to use the override onCreateOptionsMenu from FileFragment because MainFileListFragment does not extend FileFragment, and incorporating it into this class is not planned. Therefore, all contentDescriptions that appear in this menu had to be added to this class: "select all," "select inverse," "open with," "rename," "move," "copy," "send," "set available offline," "unset available offline," "details," and "remove."

On the other hand, we have cases where the itemMenu is executed from an activity:

FileDisplayActivity:
A contentDescription has been added to the "select all" item.

PreviewVideoActivity:
This activity is special because it does not use the onCreateOptionsMenu method but rather the MenuProvider interface, a more modern way of handling menus in the application (source). Initially, I considered removing this interface and taking an approach similar to the other activities and fragments, using onCreateOptionsMenu. This would allow managing the contentDescription from the onCreateOptionsMenu method in ToolbarActivity. However, in the cases we have (PreviewVideoActivity and FileDisplayActivity), each one uses a different itemMenu, so there is no common case for both. Therefore, it was decided to keep the MenuProvider and add the necessary contentDescriptions there.

You'll notice that we haven't consistently used the same attribute to define a role. For example:

className:
We've used className because it's the most commonly used attribute for assigning a role, and it also supports multilingual roles. We use className when we need to specify or override the type of widget that a view represents. This is especially useful if we're creating a custom component or if a non-standard view needs to be identified as a specific type of widget.

roleDescription:
We've used this attribute when it's necessary to describe the purpose or function of a view that isn't clear from its class type alone. This is particularly helpful for custom elements or when you want to provide additional context to assistive technology users. Specifically, we've used it to define the "Link" role. This role does not appear by default in the Android list, so we have to create it ourselves.

contentDescription:
Finally, we've used this option when, for technical reasons, it wasn't possible to work with className or roleDescription. This applies to cases like MenuItems, which we have already discussed in the issue comment.

Copy link

@mplazaspalacio mplazaspalacio left a comment

Choose a reason for hiding this comment

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

LGTM

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 21, 2024

QA checks:

Personal Space

  • Several buttons have been detected on the page that do not have a role assigned.

Fixed

  • If the elements on the page are sorted, the status of the sorting is only visually sorting status is only visually represented by arrows. However, this visual representation visual representation is a limitation for non-visual users, as they do not receive any information about the current sorting.

Fixed

  • The button with the "+" does have an accessible name, but this is not meaningful. It would make sense to use the word "Add" as an accessible the word "Add", for example, to give users a clear description of the function. clear description of the function. There is also a problem for non-visual users, as opening the menu is not opening the menu is not clearly signaled. It is important that the status of the menu is accessible to all users.

Fixed

Menu

  • The link "...-Portal" was not correctly marked as a link correctly. The role "Link" is missing, which should be used for the semantically correct should be used for the semantically correct labeling of links.

Fixed. But, the clickable area is too small, difficult to click

  • The logo (see screenshot) was erroneously labeled as a button although clicking on the element opens a new page in the browser. opens a new page in the browser. Instead, the logo should be provided with a link element in order to be semantically correct.

Fixed. But, the clickable area is too small, difficult to click

  • All elements of the menu are missing the correct role, either "Button" or "Link".

Fixed

  • The button with the account data does not have a corresponding role. In addition, the labeling is too long and difficult to understand. It is It is also unclear which view of the menu users are currently in.

Fixed

Edit shared Link

  • The "Expiry date" and "Date" buttons have no role.

Fixed

Spaces

  • Both the "Spaces" heading (button) and the "Space" buttons do not have a role.

Fixed, but the role is dictated before the name of the item. No way to reverse
#4454 (comment)

Manage Account

Fixed, but the role is dictated before the name of the item.

Image Viewer

  • None of the items in the "More options" menu have a role.

Fixed

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 21, 2024

(1) [FIXED]

The "Sort By" menu in the list of files is always dictated by talkback as "Sort by name", no matter if the selection is "Sort by date" or "Sort by size"

Pixel 2, Android 11
c0c62bed

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 21, 2024

(2) [FIXED]

Not sure if this one is part of the current PR, but probably a quick one to fix:

The mini FAB option for Upload is dictated as New folder

Let me know if it should be moved to a different issue.

Pixel 2, Android 11
c0c62bed

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 21, 2024

(3) [FIXED]

The elements on the list of items in the drawer is dictated as "x of 7", as 7 elements are in the list. But, there are only 5. Any reason? something fixable?

Pixel 2, Android 11
c0c62bed

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 21, 2024

(4) [FIXED]

About the expiration date, you stated:

These are not buttons, they work as informational text. In any case, these elements have been labeled with the corresponding switches in this pr #4448 related to this issue #4363

the "expiration" text is not a button, so that's correct

but, the expiration date is a button used to open the date picker in case of updating the expiration date. It should have a role.

Pixel 2, Android 11
c0c62bed

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 21, 2024

(5) [WONT FIX]

About the space cards. Talkback dictates something like: "Button ..."

Is that intended? in any other element, the role (Button) is dictated after the name of the element, not before. Is that right?

Pixel 2, Android 11
c0c62bed

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 21, 2024

(6) [WONT FIX]

Same as (5), but for items in the list of accounts inside the Manage accounts view.

Pixel 2, Android 11
c0c62bed

@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Aug 22, 2024

(3)

The elements on the list of items in the drawer is dictated as "x of 7", as 7 elements are in the list. But, there are only 5. Any reason? something fixable?

Pixel 2, Android 11 c0c62bed

Here the problem is there are two items that never use. I don't know why. The specific items are: footer_spacer_1 and footer_spacer_2. This cause there are 7 items. I have added a new attribute android:visible="false" in the items to avoid them from accessibility.

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 22, 2024

Here the problem is there are two items that never use. I don't know why. The specific items are: footer_spacer_1 and footer_spacer_2. This cause there are 7 items. I have added a new attribute android:visible="false" in the items to avoid them from accessibility.

These two footer_space seem to be free slots to fill up if more options are required for any reason. If there is no branding option affecting, i'd get rid of them. If new options are required at any moment, they could be added again, right?

@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Aug 22, 2024

Here the problem is there are two items that never use. I don't know why. The specific items are: footer_spacer_1 and footer_spacer_2. This cause there are 7 items. I have added a new attribute android:visible="false" in the items to avoid them from accessibility.

These two footer_space seem to be free slots to fill up if more options are required for any reason. If there is no branding option affecting, i'd get rid of them. If new options are required at any moment, they could be added again, right?

Ok, let's go with this option yes.

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 22, 2024

Ok, let's go with this option yes.

Please, be sure first that no branding option is affected.

@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Aug 22, 2024

(5)

About the space cards. Talkback dictates something like: "Button ..."

Is that intended? in any other element, the role (Button) is dictated after the name of the element, not before. Is that right?

Pixel 2, Android 11 c0c62bed

Related to report 5 and 6, it is how className works by default with items in the Adapter. Another solution could be add a contentDescription, but at the moment we will leave the className because it's more appropriate.

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 22, 2024

About report (1). Now the correct method of sorting is dictated correctly. But, i realised that an ownCloud word is dictated when a sorting method is selected from the bottom sheet, then it says the method and the asc/desc. @Aitorbp

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 22, 2024

#4454 (comment)

is that checked and correct?

@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Aug 22, 2024

#4454 (comment)

is that checked and correct?

Yes, it is checked, drawer_menu.xml is the only place where these items are referenced.

@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Aug 23, 2024

About report (1). Now the correct method of sorting is dictated correctly. But, i realised that an ownCloud word is dictated when a sorting method is selected from the bottom sheet, then it says the method and the asc/desc. @Aitorbp

The problem is that when you click on one of the sortOptionsMenu options and go back to the main screen, Talkback thinks you've started the app again and it says Owncloud.
This same test can be done if you delete the app from recent views and open it again, it will also say "ownCloud".
It also happens, for example, if you're debugging, a snackbar appears saying: ownCloud isn't responding. And click on Wait. The talkback will say Owncloud again.

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 23, 2024

The problem is that when you click on one of the sortOptionsMenu options and go back to the main screen, Talkback thinks you've started the app again and it says Owncloud.

is there any doc or something that proves that? or just a guess?

This same test can be done if you delete the app from recent views and open it again, it will also say "ownCloud".
It also happens, for example, if you're debugging, a snackbar appears saying: ownCloud isn't responding. And click on Wait. The talkback will say Owncloud again.

this is a completely different scenario. And also, saying "ownCloud" when you click on "Wait" does not sounds as something correct.

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 23, 2024

The android:label in the AndroidManifest.xml file is causing that. Talkback says the labelled name of the activity every time the focus changes. It could be prevented but the solution might not be a good one.

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 26, 2024

The android:label in the AndroidManifest.xml file is causing that. Talkback says the labelled name of the activity every time the focus changes. It could be prevented but the solution might not be a good one.

I noticed that android:label is available for application entry as a "generical" label. It can be attached to every single activity, so that, when the activity is loaded, the specific label of the activity is dictated instead of a generical "ownCloud". The point here is, how to handle fragments. Any idea?

@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Aug 26, 2024

The android:label in the AndroidManifest.xml file is causing that. Talkback says the labelled name of the activity every time the focus changes. It could be prevented but the solution might not be a good one.

I noticed that android:label is available for application entry as a "generical" label. It can be attached to every single activity, so that, when the activity is loaded, the specific label of the activity is dictated instead of a generical "ownCloud". The point here is, how to handle fragments. Any idea?

We could try to handle the fragments using for example this code: view.importantForAccessibility = View.IMPORTANT_FOR_ACCESSIBILITY_NO. But it is not working, I think it has to be manage from the Activity which has the responsibility for the fragments.

I have tried several things. For example, using the dispatchPopulateAccessibilityEvent override function. But it is not working neither, I think it was something Android used some years ago. I don't see current documentation about that and accessibility could be changed the criterial.

So, the solution that is working is to modify the label, either in the Activity or in the Manifest itself. But for me it is not a good solution.

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 26, 2024

So, the solution that is working is to modify the label, either in the Activity or in the Manifest itself. But for me it is not a good solution.

why? the label will help the visual impaired person to know where he/she is after an activity change. If the part of the fragments work fine (i don't know at this point), it could be useful.

@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Aug 26, 2024

The android:label in the AndroidManifest.xml file is causing that. Talkback says the labelled name of the activity every time the focus changes. It could be prevented but the solution might not be a good one.

I noticed that android:label is available for application entry as a "generical" label. It can be attached to every single activity, so that, when the activity is loaded, the specific label of the activity is dictated instead of a generical "ownCloud". The point here is, how to handle fragments. Any idea?

We could try to handle the fragments using for example this code: view.importantForAccessibility = View.IMPORTANT_FOR_ACCESSIBILITY_NO. But it is not working, I think it has to be manage from the Activity which has the responsibility for the fragments.

I have tried several things. For example, using the dispatchPopulateAccessibilityEvent override function. But it is not working neither, I think it was something Android used some years ago. I don't see current documentation about that and accessibility could be changed the criterial.

So, the solution that is working is to modify the label, either in the Activity or in the Manifest itself. But for me it is not a good solution.

Everything relate to the label in activities and fragments will be done in this issue #4458

@Aitorbp Aitorbp force-pushed the feature/name_role_value_accessibility branch from 5459d1d to 2c3a1c9 Compare August 26, 2024 13:11
@jesmrec
Copy link
Collaborator

jesmrec commented Aug 27, 2024

After latest checks, ready to go.

@Aitorbp Aitorbp force-pushed the feature/name_role_value_accessibility branch from 2c3a1c9 to 4e99414 Compare August 27, 2024 07:02
@Aitorbp Aitorbp requested a review from jesmrec August 27, 2024 07:43
@Aitorbp Aitorbp merged commit 3541973 into master Aug 27, 2024
7 checks passed
@Aitorbp Aitorbp deleted the feature/name_role_value_accessibility branch August 27, 2024 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[a11y] 11.4.1.2 Name-role value
3 participants