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

Fix Incorrect buttons labels in High Contrast mode #4436

Merged
merged 8 commits into from
Dec 21, 2020
Merged

Fix Incorrect buttons labels in High Contrast mode #4436

merged 8 commits into from
Dec 21, 2020

Conversation

sculpt0r
Copy link
Contributor

@sculpt0r sculpt0r commented Dec 14, 2020

Add padding and space in text.
Space alone doesn't give any results. But it could be helpfull for screen readers.

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches that change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

  • Unit tests
  • Manual tests

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

  • PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#4422](https://github.com/ckeditor/ckeditor4/issues/4422): Added extra space before shortcut label in HC mode.

What changes did you make?

Give an overview…

Added padding left for shortcut description span element in High Contrast Mode.
Added this in moono, moono-lisa and kama.

Also added an empty character before shortcut, but it doesn't give any effect.
I keep this empty character, because of conversation with @Comandeer . He says that it's better for readers - to see separate words.

Which issues does your PR resolve?

Closes #4422.

Add padding and space in text.
Space alone doesn't give any results. But it could be helpfull for screen readers.
@f1ames f1ames self-requested a review December 15, 2020 08:41
@f1ames f1ames self-assigned this Dec 15, 2020
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

It's a bug fix so changelog entry should not be skipped here.

Also added an empty character before shortcut, but it doesn't give any effect.
I keep this empty character, because of conversation with @Comandeer . He says that it's better for readers - to see separate words.

The space doesn't have any effect since it's a space on the beginning/end of an inline-block element and is trimmed by the browser.

Added padding left for shortcut description span element in High Contrast Mode.
Added this in moono, moono-lisa and kama.

I'm not sure if this is the best approach for this issue since:

  • There are more skins available and it won't work for them.
  • Adding spacing with padding is fine for layout purposes. Here, I have some difficulties deciding if we should treat it as layouting or just text formatting - if the later, using padding looks like not a valid approach.

Based on the above we could simply use non breaking space here ( ) which will be preserved and rendered correctly and also is handled same as space by screen readers AFAIR (cc @Comandeer?).

Also it could be added directly in the template (instead of ariaShortcut label) because it is specific for this one usage (and ariaShortcut could be reused somewhere else).

skins/moono-lisa/toolbar.css Outdated Show resolved Hide resolved
@sculpt0r
Copy link
Contributor Author

sculpt0r commented Dec 16, 2020

I revert a previous proposal.

I added   conditionally rendered if there is a shortcut to render.

As I check with JAWS in FF - the reader doesn't read the second span since the button points it as aria-describedby.
Together with @f1ames we decide to treat this issue purely visually.
@Comandeer also will take a look - so I create a separate issue for this.

Also, add a changelog entry.

@sculpt0r sculpt0r requested a review from f1ames December 16, 2020 09:21
@f1ames f1ames self-assigned this Dec 16, 2020
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@sculpt0r could you add manual test here (which forces CKEDITOR.env.hc=true so it can be easily tested on all OSes)?

@sculpt0r sculpt0r self-assigned this Dec 21, 2020
@sculpt0r
Copy link
Contributor Author

Looks good

@sculpt0r could you add manual test here (which forces CKEDITOR.env.hc=true so it can be easily tested on all OSes)?

I added the manual test labelhc.
Finally I was able to force HC mode, but instead of switching CKEDITOR.env.hc I use CKEDITOR.env.cssClass += ' cke_hc';

@sculpt0r sculpt0r removed their assignment Dec 21, 2020
@sculpt0r sculpt0r requested a review from f1ames December 21, 2020 09:40
@f1ames f1ames self-assigned this Dec 21, 2020
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

Looks good 👍


That's interesting in fact how to force HC mode. Normally, it would be quite easy - one can listen to CKEDITOR.loaded event which is fired right after HC detection logic or even better - force same color for div borders to trick our detection mechanism.

However... all this logic (from _bootstrap.js) is done before bender loads any manual tests content - this means it's not possible to alter the above with anything placed in manual test files. And there is no other place in fact to hook into without modifying bender or CKEditor 4 files.

Still it can be done a little more elegant than it was proposed with:

CKEDITOR.on( 'instanceCreated', function( evt ) {
	CKEDITOR.env.hc = true;
	CKEDITOR.env.cssClass += ' cke_hc';
} );

as this code is executed before plugins code. Also CKEDITOR.env.hc should be correctly overwritten because some stuff in plugins code depends on it too.

@f1ames f1ames merged commit 1f7743e into master Dec 21, 2020
@f1ames f1ames deleted the t/4422 branch December 21, 2020 13:09
@sculpt0r sculpt0r assigned sculpt0r and unassigned sculpt0r Dec 21, 2020
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.

2 participants