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

[BUG]: Certain ARIA roles must contain particular children #17161

Closed
KolliAnitha opened this issue Jan 27, 2023 · 21 comments
Closed

[BUG]: Certain ARIA roles must contain particular children #17161

KolliAnitha opened this issue Jan 27, 2023 · 21 comments
Assignees
Labels
a11y bug Label to indicate an issue is a regression good first issue Impact: Low -- ONLY DO IF GOOD FIRST ISSUE Postponing for now, since it doesn't affect users much. Work: Low Solution is known and broken into good-first-issue-sized chunks.

Comments

@KolliAnitha
Copy link

Describe the bug

Ensures elements with an ARIA role that require child roles contain them

Steps To Reproduce

Login to www.oppiatestserver.org
Go to Contributor dashboard
In Review Questions tab, select any question and click on Review button
Run axe devtools
See errors

Expected Behavior

To solve this problem, you need to
Fix at least (1) of the following:
Required ARIA child role not present: tab
Element has no aria-busy="true" attribute

Screenshots/Videos

Screenshot 2023-01-27 11 20 15 AM
Screenshot 2023-01-27 11 19 50 AM

What device are you using?

Desktop

Operating System

Other

What browsers are you seeing the problem on?

Chrome

Browser version

108.0.5359.111

Additional context

No response

@sagangwee
Copy link
Contributor

For new contributors

@sagangwee sagangwee added Impact: Low -- ONLY DO IF GOOD FIRST ISSUE Postponing for now, since it doesn't affect users much. Work: Low Solution is known and broken into good-first-issue-sized chunks. a11y good first issue labels Feb 16, 2023
@github-actions
Copy link

Hi @sagangwee, thanks for proposing this as a good first issue. I am removing the label for now and looping in @DubeySandeep to approve the label. It will be added back if approved. Thanks!

@bhubneshmaharana
Copy link

and sir pls assign me admin role so I can fix this error and creat a PR

@seanlip
Copy link
Member

seanlip commented Feb 17, 2023

@bhubneshmaharana Per the guidance at https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#choosing-a-good-first-issue, please provide an explanation of what your PR will do (with names of files you're changing, what you plan to change in each file, etc.). If it looks good, we can assign you to this issue.

Please also follow the other instructions on that wiki page if you have not yet done so. Thanks!

@mansi1234567890
Copy link

hello sir , please assign me this task so , that I can clear the bug and get a good experience.

@seanlip
Copy link
Member

seanlip commented Feb 25, 2023

@mansi1234567890 See my previous comment in this thread, which has instructions on how to claim an issue.

@pawelborkar
Copy link
Contributor

Hi @seanlip,
I was checking the issues and saw that lot of people are interested in solving the bugs are asking for assigning the issues to them.

And you replying to them by

<-username-> Per the guidance at https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#choosing-a-good-first-issue, please provide an explanation of what your PR will do (with names of files you're changing, what you plan to change in each file, etc.). If it looks good, we can assign you to this issue.

Please also follow the other instructions on that wiki page if you have not yet done so. Thanks!

I think we can automate your reply by implementing a github-action-bot which will specifically target the assign keyword from the first time contributors and post this reply to them tagging their username.

What do you think?

@seanlip
Copy link
Member

seanlip commented Mar 5, 2023

Hi @pawelborkar, I'm not sure if we should do that, but thanks for your suggestion. Sometimes people do actually provide the necessary details and ask to be assigned, and in such cases I think it would not be great for them to receive that message.

The thing I am confused about is that this guidance is part of our standard onboarding flow and wiki pages, which I feel like many people might be skipping. If you have a suggestion for how to make this more prominent somehow, we'd be all ears!

@dante381
Copy link
Contributor

dante381 commented Mar 7, 2023

We can fix the issue by adding aria-busy=true attribute to the highlighted tag in file core/templates/components/state-editor/state-responses-editor/state-responses.component.html and core/templates/components/state-editor/state-solution-editor/state-solution-editor.component.html

image

@seanlip Can i be assigned to this issue?

@seanlip
Copy link
Member

seanlip commented Mar 7, 2023

Thanks @dante381. One question: conceptually, what is going on here? What is the aria role that's problematic, and why does it need to "contain particular children"? What is the significance of putting aria-busy where you're putting it?

(In order to assess whether an auto-recommended fix makes sense, it's necessary to understand a bit more about why we're doing the fix. If you can explain this and confirm that your fix is the right thing to do conceptually, I'd be happy to assign you the issue.)

@dante381
Copy link
Contributor

dante381 commented Mar 7, 2023

Aria role is required as to define the purpose of the element. As developers try to understand the code the aria role allows to understand the role of the tag and its purpose. The aria-busy attribute set to true indicates that region is loaded with new content.

This will be helpful for people who use a screen reader.

@seanlip I hope this explains the fix.

@seanlip
Copy link
Member

seanlip commented Mar 7, 2023

Yup, I know what aria roles are -- my question is why we are making this specific change for these specific elements. For example, could you please explain which element exactly needs to contain particular children, and why? And then, conceptually, what are you adding the aria-busy to and why is it the right fix?

@PVenkatArvind
Copy link
Contributor

Greetings! @seanlip can you please assign me this issue

will make changes to state-responses.component.html

adding aria-busy = "true"

before
before17161

after
after_17161

Ran an Axe dev tool scan
To solve this problem, you need to fix at least (1) of the following:
Required ARIA child role not present: tab
Element has no aria-busy="true" attribute-->
Adding the aria-busy="true" element indicates and informs the users that the content is still loading or is in progress /updating
next why are we making changes to these specific elements or div elements is because role is tab so it is an tab interface just like keyboard focus where use tab to check its also a tab component and hence we require to switch contents and components which provides the user better feature to use oppia interface and navigate the interface better and this aria - busy = " true" tells the user that its loading and not ready for interaction for a certain time or immediate interaction

@dante381
Copy link
Contributor

dante381 commented Mar 7, 2023

Sorry, I misunderstood the question. I am adding the aria-busy attribute to the following tags ,

image
image
the above code is from core/templates/components/state-editor/state-responses-editor/state-responses.component.html.

image
the above code is from core/templates/components/state-editor/state-solution-editor/state-solution-editor.component.html

But after going through the code again, I missed that the parent containers of these elements contain a role attribute tablist.
So I changed the aria-busy=true to role=tab. This provides a clearer understanding of the element.

image
image
the above codes belong to core/templates/components/state-editor/state-responses-editor/state-responses.component.html.

image
the above code belongs to core/templates/components/state-editor/state-solution-editor/state-solution-editor.component.html.

@seanlip
Copy link
Member

seanlip commented Mar 7, 2023

This seems sensible to me conceptually, thanks @dante381. Assigning you.

(@PVenkatArvind For the record, your explanation is a bit too generic and I can't understand its relevance to the specific code that is under consideration. So, even though you wrote it first, I can't assign you to this issue.)

@PVenkatArvind
Copy link
Contributor

Greetings! @seanlip , Noted ! with thanks !

@dante381
Copy link
Contributor

dante381 commented Mar 8, 2023

@seanlip Could you please review the changes?

@seanlip
Copy link
Member

seanlip commented Mar 8, 2023

Hi @dante381, your PR reviewer is @nithinrdy. I'll defer to him to take a pass. Thanks!

@Ash-2k3 Ash-2k3 moved this from Todo to Done in Contributor Experience Team Mar 21, 2023
@Ash-2k3
Copy link
Member

Ash-2k3 commented Mar 21, 2023

Closing as fixed in #17627

@Ash-2k3 Ash-2k3 closed this as completed Mar 21, 2023
@satendrakumar50
Copy link

I want join your organization, and I know react,javaScript,Tailwind for css,Normal Css,Html

@Shahbaz898414
Copy link

I want join your organization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y bug Label to indicate an issue is a regression good first issue Impact: Low -- ONLY DO IF GOOD FIRST ISSUE Postponing for now, since it doesn't affect users much. Work: Low Solution is known and broken into good-first-issue-sized chunks.
Projects
Archived in project
Development

No branches or pull requests