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

Show number of characters when typing a commit message #5207

Merged
merged 4 commits into from Aug 12, 2019

Conversation

@jmcphers
Copy link
Member

commented Aug 9, 2019

This change adds a small character counter to the commit message entry box. It is visible only if you've entered a commit message, and intended to help keep commit messages shorter than some limit if desired.

image

Closes #5192.

@jmcphers jmcphers requested a review from gtritchie Aug 9, 2019
Copy link
Contributor

left a comment

See comment on accessibility concerns--naturally :-)

<rs_widget:FormLabel ui:field="lblCommit_" text="Commit message"/>
</g:cell>
<g:cell horizontalAlignment="ALIGN_RIGHT">
<g:Label ui:field="lblCharCount_"/>

This comment has been minimized.

Copy link
@gtritchie

gtritchie Aug 10, 2019

Contributor

As you might expect, this would be considered inaccessible for both blind and limited-vision users.

For a "sighted user", they can reasonably be expected to notice the counter changing as they type, so will understand what the number represents.

For limited vision, a user may be zoomed way in on the input field and not notice the counter updating, thus wouldn't necessary deduce what it means even if they happen to pan over to it at some point. So, it needs to provide more context, e.g. "# characters in message" instead of just "#". This also partially helps the screen reader scenario (next).

For a screen reader, this field has a couple problems. First, like limited vision, even if the user moves the screen reader cursor to it, all they'll get is a number, without any context (i.e. they won't "see" it updating while they type), so more detailed wording is strongly recommended.

More advanced would be something that announces the message length via a visually-hidden live region, probably role="status", per https://www.w3.org/TR/wai-aria-1.1/#status. Because a screen reader will be reading what they type, you don't want this message being read over every keystroke, so would require some debouncing, such as not updating the message for some delay without keyboard input. The visible label shouldn't exhibit this delay, so can't use it as the live-region (hence need for visually-hidden live region). There's an example of this at https://github.com/rikschennink/short-and-sweet

I'd consider updating the label as a must-fix for accessibility. The announced-counter would be very nice to have, especially if we have other places that will use it, but not a show stopper.

This comment has been minimized.

Copy link
@jmcphers

jmcphers Aug 12, 2019

Author Member

Very much appreciate the detailed breakdown! I've taken a shot at doing this with full-bore accessibility support: there are now two labels, one exposed only to screen readers (with a debounced update) and one hidden from screen readers.

@@ -152,4 +152,13 @@

.ignoreWhitespace label {
margin-left: 4px;
}

.charCountPanel {

This comment has been minimized.

Copy link
@gtritchie

gtritchie Aug 12, 2019

Contributor

For consistency, use .visuallyHidden from themeStyles.css. There are a few different techniques for this, might as well be using the same one everywhere.

This comment has been minimized.

Copy link
@jmcphers

jmcphers Aug 12, 2019

Author Member

Done!

@coatless

This comment has been minimized.

Copy link

commented Aug 12, 2019

Would it be possible to "grey" text past the limit?

@jmcphers

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

@coatless Not as part of this change; this change doesn't add a limit, really, just tells you the # of characters so you're aware if you've hit one. It would indeed be nicer to have a configurable limit with more feedback!

Copy link
Contributor

left a comment

Looks good! Once it's in I'll play with it in a couple screen reader/browser combos and see if it works as hoped.

@jmcphers jmcphers merged commit 1e39805 into master Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.