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

Improve accessibility for price fields of the type select by adding the price field label to the placeholder. #20927

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

alifrumin
Copy link
Contributor

@alifrumin alifrumin commented Jul 21, 2021

Overview

Due to limitations in Select2, screen readers don't read the labels of Select2 fields. As a stopgap, this adds the price field label to the placeholder text for price fields of the type select. The placeholder text is read by the screenreader.

Before

When using a price field of the type select on an event registration or contribution form the placeholder text was "- select -".

For example if the price field label was "Shirt Size" the place holder would be "- select -".

After

When using a price field of the type select on an event registration or contribution form the placeholder text is "- select $priceFieldLabel -".

For example if the price field label is "Shirt Size" the place holder would be "- select Shirt Size -".

Comments

Continues work outlined in #17675

@civibot
Copy link

civibot bot commented Jul 21, 2021

(Standard links)

@civibot civibot bot added the master label Jul 21, 2021
@eileenmcnaughton eileenmcnaughton added the merge ready PR will be merged after a few days if there are no objections label Jul 21, 2021
@eileenmcnaughton
Copy link
Contributor

Looks good - just added merge ready in case @colemanw or @mlutfy have thoughts on the translation side

@mlutfy
Copy link
Member

mlutfy commented Jul 21, 2021

Unfortunately it does make assumptions about how strings are concatenated. In French and most languages, it would look bad to have "sélectionner Taille de chandail" (incorrect capitalization and grammar).

Edit: maybe we can translate as "- Sélectionner : Taille de chandail -"

However, accessiblity is important, so I maybe minor bad grammar might be acceptable.

What about #20825 ?

@seamuslee001
Copy link
Contributor

@mlutfy I think your reference was meant to be #20926

@mlutfy
Copy link
Member

mlutfy commented Jul 21, 2021

No i really meant the select2 upgrade :)
Doesn't it have better accessibility?

But I guess it might take a while, so maybe not a good fix.

@aydun
Copy link
Contributor

aydun commented Jul 22, 2021

Edit: maybe we can translate as "- Sélectionner : Taille de chandail -"
However, accessiblity is important, so I maybe minor bad grammar might be acceptable.

Anything to help screenreaders is good! Maybe we can accept some dodgy grammar in translation if it communicates usefully.

@shaneonabike
Copy link
Contributor

Disclosure: I'm not someone who has visual impairments so I can only comment and would prioritize the needs of those who are commenting and have this accessibility need

In my experience with design/integration usually the text simply displays what the action is. So in this case it would be Shirt sizes rather than Select Shirt Sizes.

I did a simple test with a Screen Reader on a Civi form and it does indicate that it is a "Menu". So for example, if it were Shirt Sizes then it indicated "Shirt Sizes combobox push button to open menu"

@mlutfy
Copy link
Member

mlutfy commented Jul 23, 2021

@alifrumin Any thoughts on displaying just the field label, without "select -"?

@alifrumin
Copy link
Contributor Author

That seems like a reasonable solution to me. @agh1 any thoughts?

Should we then update the place holder text for custom field selects as well (#17675)?

@agh1
Copy link
Contributor

agh1 commented Jul 26, 2021

@mlutfy I'm with @alifrumin in that the - select Shirt Size - problem of capitalization is the same for all custom fields and it would be reasonable to standardize in a way that sidesteps it. I don't have any real opposition to dropping the "select" but we really should be consistent throughout CiviCRM. I'm sorry I missed this in all the prior work.

This PR, as it stands, resolves that by bringing price fields in line with everything else. I'd suggest that the best approach would be to merge it as-is and then engage more people in the much larger question--and task--of updating all select boxes to be consistently something better.

Besides the obvious consideration that there are a lot of - select Something - placeholders throughout the code, making it a fair amount of work to change it all, there's also the question of what to do with certain placeholders that need to indicate that the blank field is actually any option rather than no option.

I'd also suggest that select boxes for price fields seem relatively less common than either select boxes for other fields or other input types for price fields. I don't think that many people will be impacted by the change on this PR, so it is much lower in impact than a potential later one that changes how nearly every drop-down everywhere works.

@mlutfy
Copy link
Member

mlutfy commented Jul 28, 2021

OK - makes sense.

I did a quick test on the demo site, it looks like this in French:

priceset-2021-07-28_13-48

(I created a 'fruit' field with apples and oranges)

I still think it looks weird, but accessibility over design, and folks can override the translation if not happy. As @agh1 mentions, CiviCRM is already behaving like that in other places, so I'm late to the party :)

Thank you @alifrumin !

@mlutfy mlutfy merged commit af65ee6 into civicrm:master Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants