Skip to content

fix(radio): change touch target to fix blocked aria-hidden on an <input> element error #5700

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

Merged
merged 3 commits into from
Aug 29, 2024

Conversation

cotone-dev
Copy link

@cotone-dev cotone-dev commented Aug 23, 2024

Fixes #5699

Copy link

google-cla bot commented Aug 23, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@asyncLiz
Copy link
Collaborator

asyncLiz commented Aug 23, 2024

Mentioned in the PR, we still need a touch target that isn't inert. I think we can remove the input entirely and replace it with a touch target. Can you update this PR for that?

Replace the <input> here with:

<div class="touch-target"></div>

Replace the <input> touch target styles here with:

    .touch-target {
      height: 48px;
      position: absolute;
      width: 48px;
    }

    :host([touch-target='none']) .touch-target {
      width: 100%;
      height: 100%;
    }

And keep aria-hidden="true" on the <div class="container">

@cotone-dev
Copy link
Author

cotone-dev commented Aug 24, 2024

@asyncLiz

Thank you for your prompt feedback.

I will make adjustments based on the information you provided.

This is my first pull request, so please excuse my rudimentary question.

I looked at the package.json file to set up a development server, but there are no scripts such as dev or start set up, so I couldn't figure out how to start it.

It seems that npm run build will build the server, but will I have to load this script in a separate index.html?

We apologize for the inconvenience, but we would appreciate it if you could let us know.

@asyncLiz
Copy link
Collaborator

We don't have a great dev server setup for external contributors yet. The only option right now is to run the package's catalog server and navigate to /components/radio/stories/.

@cotone-dev
Copy link
Author

@asyncLiz
Thank you very much for your kind attention.
I was able to successfully set up the development server.

We will contact you again as soon as the adjustments are completed.

@cotone-dev
Copy link
Author

@asyncLiz

Please excuse me for repeating myself.

Is <md-radio> in radio/demo/stories.ts provided by unpkg?

I need to edit radio/internal/radio.ts this time, but I am having trouble getting the changes to take effect.

On the other hand, the elements in radio/demo/stories.ts reflect the changes (e.g. changing “Cats” to “Cat”).

Your wisdom would be greatly appreciated.
Thank you in advance.

@asyncLiz
Copy link
Collaborator

Is <md-radio> in radio/demo/stories.ts provided by unpkg?

It may be, that sounds familiar. I haven't tried running it locally for development :(

I'm not sure I have a good suggestion. I think @VandeurenGlenn mentioned they had a script in their fork of the repo for development.

@cotone-dev
Copy link
Author

@asyncLiz

Thank you for your response.
I understand the content.

So for now, I will only send the changes you proposed and he will be responsible for confirming the operation status, is that correct?

@asyncLiz
Copy link
Collaborator

You can make the changes and I'll verify it

@cotone-dev
Copy link
Author

@asyncLiz

Understood.

While I regret not being able to test it in my environment this time, I will send you the proposed changes.

I'm interested in Lit and Google Material Components, and would appreciate the opportunity to learn more about them in the future.

Please let me know when the development environment for external contributors is ready.

Thank you for your time.

Copy link
Collaborator

@asyncLiz asyncLiz left a comment

Choose a reason for hiding this comment

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

Thanks! I'll take a look at this and test it internally

@asyncLiz asyncLiz changed the title fix: Replace aria-hidden attribute with inert attribute in <md-radio> component fix(radio): change touch target to fix blocked aria-hidden on an <input> element error Aug 28, 2024
Comment on lines 99 to 100
width: 100%;
height: 100%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Things look good! The one thing I see to improve is we can display: none the touch target instead of making it fit the width/height, like we do with button's touch target. Mind making that change?

:host([touch-target='none']) .touch-target {
  display: none;
}

Copy link
Author

Choose a reason for hiding this comment

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

Understood. I will make the changes and get back to you shortly.

@copybara-service copybara-service bot merged commit 990d065 into material-components:main Aug 29, 2024
3 of 4 checks passed
@cotone-dev
Copy link
Author

@asyncLiz

Thank you very much for the series of guidance and reviews.

@cotone-dev cotone-dev deleted the use-inert-radio branch August 30, 2024 01:02
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.

Accessibility issue with aria-hidden="true" attribute in <md-radio> component
2 participants