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

core(listitem): mention li can be contained by a menu #13927

Merged
merged 5 commits into from
May 3, 2022

Conversation

FMJansen
Copy link
Contributor

Summary
Axe-core has been updated to allow listitems to exist inside menu elements, in accordance with the HTML spec. So, this text should be updated since axe-core has been updated to 4.4.x (37a1ceb)

Related to GoogleChrome/web.dev#7802 and GoogleChrome/web.dev#7803 in the web.dev repo.

Menu spec: https://html.spec.whatwg.org/multipage/grouping-content.html#the-menu-element
Menu on MDN: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/menu

Related Issues/PRs
#13926

Axe-core has been updated to allow listitems to exist inside menu elements, in accordance with the HTML spec. So, this text should be updated since axe-core has been updated to 4.4.x (GoogleChrome@37a1ceb)
@FMJansen FMJansen requested a review from a team as a code owner April 27, 2022 10:49
@FMJansen FMJansen requested review from connorjclark and removed request for a team April 27, 2022 10:49
@adamraine adamraine changed the title allow li to live inside menu core(listitem): allow li to live inside menu Apr 27, 2022
@adamraine
Copy link
Member

You will need to run yarn update:sample-json and commit the changes since this modifies one of our translated UI strings.

@FMJansen
Copy link
Contributor Author

Ah, got it, missed that before!

@connorjclark connorjclark changed the title core(listitem): allow li to live inside menu core(listitem): update strings to mention li can be contained by a menu Apr 27, 2022
@connorjclark
Copy link
Collaborator

This is our fault, but we have a newly extra step to deal with too: yarn update:flow-sample-json

@adamraine wdyt of having yarn update:sample-json just do both? I always forget too.

@FMJansen
Copy link
Contributor Author

At least the error said it, too, haha, done! :D

@adamraine
Copy link
Member

@adamraine wdyt of having yarn update:sample-json just do both? I always forget too.

SGTM

@FMJansen
Copy link
Contributor Author

I see some tests are failing but this is a little over my head, do I need to fix something on my end?

@connorjclark
Copy link
Collaborator

You won't believe it but... we also have to run yarn update:test-devtools to update some other tests. I'll take care of it from here, though, just a minute.

@FMJansen
Copy link
Contributor Author

Haha I found that command in one of the failed test messages, but when I ran it nothing changed so I was already confused. Thanks for picking it up :)

Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

Thanks!

@connorjclark connorjclark changed the title core(listitem): update strings to mention li can be contained by a menu core(listitem): mention li can be contained by a menu May 3, 2022
@connorjclark connorjclark merged commit 26fd5aa into GoogleChrome:master May 3, 2022
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.

4 participants