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

Fix for accessibility: Modal Dialog Button #3400

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@vdeshpande
Contributor

vdeshpande commented Jun 27, 2016

Description

This fix is to make the screen reader read the Modal Dialog Close button better.

Specific Changes proposed

Please list the specific changes involved in this pull request.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors
@vdeshpande

This comment has been minimized.

Show comment
Hide comment
@vdeshpande

vdeshpande Jun 27, 2016

Contributor

@gkatsev , @OwenEdwards How do the above changes look ?

Contributor

vdeshpande commented Jun 27, 2016

@gkatsev , @OwenEdwards How do the above changes look ?

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jun 27, 2016

Member

close-button still needs a default controlText similar to clickable-component's

Member

gkatsev commented Jun 27, 2016

close-button still needs a default controlText similar to clickable-component's

@mister-ben

This comment has been minimized.

Show comment
Hide comment
@mister-ben

mister-ben Jun 27, 2016

Contributor

Are the translations from Google Translate? The German and Dutch at least aren't right.

Contributor

mister-ben commented Jun 27, 2016

Are the translations from Google Translate? The German and Dutch at least aren't right.

@OwenEdwards

This comment has been minimized.

Show comment
Hide comment
@OwenEdwards

OwenEdwards Jun 28, 2016

Member

@gkatsev I think the close-button does (still) have a default controlText:

https://github.com/videojs/video.js/blob/master/src/js/close-button.js#L15

  constructor(player, options) {
    super(player, options);
    this.controlText(options && options.controlText || this.localize('Close'));
  }

@mister-ben is there a specific procedure for adding new text which needs translating? Should it just be added into the en.json file, and then left for others to translate?

Member

OwenEdwards commented Jun 28, 2016

@gkatsev I think the close-button does (still) have a default controlText:

https://github.com/videojs/video.js/blob/master/src/js/close-button.js#L15

  constructor(player, options) {
    super(player, options);
    this.controlText(options && options.controlText || this.localize('Close'));
  }

@mister-ben is there a specific procedure for adding new text which needs translating? Should it just be added into the en.json file, and then left for others to translate?

@mister-ben

This comment has been minimized.

Show comment
Hide comment
@mister-ben

mister-ben Jun 28, 2016

Contributor

I think just updating the en.json "template" is better than spending the time to add low quality translations that will need to be updated anyway. We could put up a page showing which language files have missing keys.

Contributor

mister-ben commented Jun 28, 2016

I think just updating the en.json "template" is better than spending the time to add low quality translations that will need to be updated anyway. We could put up a page showing which language files have missing keys.

@vdeshpande

This comment has been minimized.

Show comment
Hide comment
@vdeshpande

vdeshpande Jun 28, 2016

Contributor

@mister-ben Sure, we could update the template.
However the list of localized files may increase over time and we can afford to have missing keys.
Missing values for a key is fine as the screen reader will read the key in that case ( Voice Over read the key in English)

Also I was using Google translate to translate the text as I do not know many of the languages.
Feel free to translate the text back ;)

Contributor

vdeshpande commented Jun 28, 2016

@mister-ben Sure, we could update the template.
However the list of localized files may increase over time and we can afford to have missing keys.
Missing values for a key is fine as the screen reader will read the key in that case ( Voice Over read the key in English)

Also I was using Google translate to translate the text as I do not know many of the languages.
Feel free to translate the text back ;)

@mister-ben

This comment has been minimized.

Show comment
Hide comment
@mister-ben

mister-ben Jun 28, 2016

Contributor

Google translate really isn't that good, and missing keys won't break anything. localize() returns the original string if there's not a key for the language in use. Some of the files here have empty strings as the value, so that makes things even worse.

I'd let a native speaker add a good translation when they discover there are missing keys, which has happened fairly well organically so far, rather than add poor translations.

Again, I think it's worth putting something together that will check for missing translations (again much simpler to check for missing keys) so contributors speaking a language can know what needs addressing. I can see if I can figure something out for that.

Contributor

mister-ben commented Jun 28, 2016

Google translate really isn't that good, and missing keys won't break anything. localize() returns the original string if there's not a key for the language in use. Some of the files here have empty strings as the value, so that makes things even worse.

I'd let a native speaker add a good translation when they discover there are missing keys, which has happened fairly well organically so far, rather than add poor translations.

Again, I think it's worth putting something together that will check for missing translations (again much simpler to check for missing keys) so contributors speaking a language can know what needs addressing. I can see if I can figure something out for that.

@vdeshpande

This comment has been minimized.

Show comment
Hide comment
@vdeshpande

vdeshpande Jun 28, 2016

Contributor

@mister-ben localize() takes in the key , so not having a key would be an issue.
Not having a value to a key is ok I think.

I'm fine with letting native translators translate text. What do you guys think @gkatsev , @OwenEdwards ?

Contributor

vdeshpande commented Jun 28, 2016

@mister-ben localize() takes in the key , so not having a key would be an issue.
Not having a value to a key is ok I think.

I'm fine with letting native translators translate text. What do you guys think @gkatsev , @OwenEdwards ?

@OwenEdwards

This comment has been minimized.

Show comment
Hide comment
@OwenEdwards

OwenEdwards Jun 30, 2016

Member

I've looked at how localize() works, and tested it, and it seems like it can handle both a missing key and "key": "", which both just return the key value.

So this is more of a style decision than a programmatic decision. Personally, I think having null values in the language files (the second pattern) is confusing, rather than having "key": "key", and the absence of a key indicates that the language file is incomplete, but I'm open to other decisions.

@mister-ben it would certainly be useful if something could detect whether/which language files are incomplete, and maybe also enforce what you decide about whether null keys are allowed?

@vdeshpande can you undo the changes in everything except modal-dialog.js and en.json?

Member

OwenEdwards commented Jun 30, 2016

I've looked at how localize() works, and tested it, and it seems like it can handle both a missing key and "key": "", which both just return the key value.

So this is more of a style decision than a programmatic decision. Personally, I think having null values in the language files (the second pattern) is confusing, rather than having "key": "key", and the absence of a key indicates that the language file is incomplete, but I'm open to other decisions.

@mister-ben it would certainly be useful if something could detect whether/which language files are incomplete, and maybe also enforce what you decide about whether null keys are allowed?

@vdeshpande can you undo the changes in everything except modal-dialog.js and en.json?

@vdeshpande

This comment has been minimized.

Show comment
Hide comment
@vdeshpande

vdeshpande Jun 30, 2016

Contributor

@gkatsev Changes undone , done!

Contributor

vdeshpande commented Jun 30, 2016

@gkatsev Changes undone , done!

@OwenEdwards

This comment has been minimized.

Show comment
Hide comment
@OwenEdwards

OwenEdwards Jul 1, 2016

Member

@mister-ben maybe this tool could be part of the automated checking/maintaining translations? https://github.com/edenspiekermann/translations. @gkatsev should I open a separate issue on "translations maintenance"? Seems like we could add some guidance in CONTRIBUTING.md?

Member

OwenEdwards commented Jul 1, 2016

@mister-ben maybe this tool could be part of the automated checking/maintaining translations? https://github.com/edenspiekermann/translations. @gkatsev should I open a separate issue on "translations maintenance"? Seems like we could add some guidance in CONTRIBUTING.md?

Show outdated Hide outdated src/js/modal-dialog.js
@@ -256,6 +256,7 @@ class ModalDialog extends Component {
let temp = this.contentEl_;
this.contentEl_ = this.el_;
close = this.addChild('closeButton');
close.controlText('Close Modal Dialog');

This comment has been minimized.

@misteroneill

misteroneill Jul 7, 2016

Member

I think this would be better like this:

close = this.addChild('closeButton', {controlText: 'Close Modal Dialog'});

It avoids multiple internal calls to controlText().

@misteroneill

misteroneill Jul 7, 2016

Member

I think this would be better like this:

close = this.addChild('closeButton', {controlText: 'Close Modal Dialog'});

It avoids multiple internal calls to controlText().

@misteroneill

This comment has been minimized.

Show comment
Hide comment
@misteroneill

misteroneill Jul 18, 2016

Member

LGTM

@OwenEdwards I like the idea of adding some translations maintenance to the CONTRIBUTING.md file.

Member

misteroneill commented Jul 18, 2016

LGTM

@OwenEdwards I like the idea of adding some translations maintenance to the CONTRIBUTING.md file.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jul 18, 2016

Member

yeah, CONTRIBUTING.md needs to be rewritten. It's probably the next thing I'm going to pick up after doing merging a bunch of stuff and making a release.

Member

gkatsev commented Jul 18, 2016

yeah, CONTRIBUTING.md needs to be rewritten. It's probably the next thing I'm going to pick up after doing merging a bunch of stuff and making a release.

@gkatsev gkatsev added confirmed and removed needs: LGTM labels Jul 18, 2016

@gkatsev gkatsev closed this in c51c19a Jul 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment