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 Improvements -- Render buttons as buttons #154

Merged
merged 6 commits into from Oct 6, 2022
Merged

A11Y Improvements -- Render buttons as buttons #154

merged 6 commits into from Oct 6, 2022

Conversation

ghost
Copy link

@ghost ghost commented Oct 3, 2022

Per #148, I have been playing with the application tonight and the first thing that leapt out to me was that the form on the "New Project" modal was not able to be submitted by keyboard. The root cause of this that this PR fixes is rendering buttons as buttons rather than a div/span.

This can cause multiple accessibility challenges both to keyboard navigation and control and also screen reader identification. Forms throughout the application are now able to be submitted by keyboard so long as they utilize a real <form> element and an onSubmit property.

@ghost ghost mentioned this pull request Oct 3, 2022
Copy link
Owner

@zachhannum zachhannum left a comment

Choose a reason for hiding this comment

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

Thanks for this change! I just had one minor comment to address styling that will likely carry forward into future a11y changes.

app/renderer/controls/Button.tsx Show resolved Hide resolved
app/renderer/controls/Button.tsx Outdated Show resolved Hide resolved
Copy link
Owner

@zachhannum zachhannum left a comment

Choose a reason for hiding this comment

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

Sorry, just noticed one more minor thing!

We should be passing disabled={disabled} to StyledButton so that form submissions with the button are not possible if we've disabled the button. Minor thing, but it currently we can use Generate Book button even if it's disabled!

@ghost
Copy link
Author

ghost commented Oct 4, 2022

Sorry, just noticed one more minor thing!

We should be passing disabled={disabled} to StyledButton so that form submissions with the button are not possible if we've disabled the button. Minor thing, but it currently we can use Generate Book button even if it's disabled!

No problem, but I want to make sure I am looking at the right thing. In NewBookModel there does not seem to be disabled value or any sort of toggle, but in GenerateBookModel, the prop isDisabled is being passed in and then used on StyledButton, but I'm assuming this needs to be changed from isDisabled to disabled to match the correct prop for a standard HTML button?

@zachhannum
Copy link
Owner

No problem, but I want to make sure I am looking at the right thing. In NewBookModel there does not seem to be disabled value or any sort of toggle, but in GenerateBookModel, the prop isDisabled is being passed in and then used on StyledButton, but I'm assuming this needs to be changed from isDisabled to disabled to match the correct prop for a standard HTML button?

Sorry, the GitHub reviewer doesn't let you make comments on any line in the code yet.

Yep, the change should be made in Button.tsx and should be exactly what you said, pass the disabled prop down to the standard HTML button. I think you're right, you can also remove the isDisabled prop from StyledButtonProps and use p.disabled directly in the css.

@ghost
Copy link
Author

ghost commented Oct 5, 2022

I think this is good to go now. I changed it to disabled on the button itself, but maintained isDisabled in higher elements to create distinction between the abstraction and the element, but I can make it disabled all the way up if you would prefer that.

I also noticed in testing that the button maintained the click cursor if the button has been disabled, so I fixed that also.

app/renderer/controls/Button.tsx Outdated Show resolved Hide resolved
app/renderer/modals/GenerateBookModal.tsx Outdated Show resolved Hide resolved
@zachhannum
Copy link
Owner

Looking good, these should be the last few comments!

@ghost
Copy link
Author

ghost commented Oct 6, 2022

I think we are good to go now, sorry you've had to go over it so many times!

Copy link
Owner

@zachhannum zachhannum left a comment

Choose a reason for hiding this comment

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

Everything looks great now!

@zachhannum zachhannum merged commit 98a8f2e into zachhannum:main Oct 6, 2022
@zachhannum zachhannum linked an issue Oct 7, 2022 that may be closed by this pull request
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.

Accessibility
2 participants