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

Inaccessible input for link's URL #1098

Closed
Comandeer opened this issue Jun 21, 2018 · 35 comments Β· Fixed by #8267
Closed

Inaccessible input for link's URL #1098

Comandeer opened this issue Jun 21, 2018 · 35 comments Β· Fixed by #8267
Assignees
Labels
bc:major Resolving this issue will introduce a major breaking change. domain:accessibility This issue reports an accessibility problem. package:link package:ui squad:core Issue to be handled by the Core team. status:discussion type:bug This issue reports a buggy (incorrect) behavior.

Comments

@Comandeer
Copy link
Member

Is this a bug report or feature request? (choose one)

🐞 Bug report

πŸ’» Version of CKEditor

Version from docs.

πŸ“‹ Steps to reproduce

  1. Select some text.
  2. Click link button.

βœ… Expected result

Shown input has proper label that is announced by screen reader.

❎ Actual result

Only [placeholder] value is announced by VoiceOver, which is confusing as placeholder is set to a URL.

Chrome devtools Accessibility Inspector shows that current label is empty:

πŸ“ƒ Other details that might be useful

The issue is caused by the fact that label for the input is hidden with display: none. It should be hidden with more accessible technique, e.g. one proposed by H5BP.

In the ideal situation, link dialog would display both label and hint outside the input itself:

Super ugly visualisation of input with label above and hint below

If the hint is present, then it should be bound to the input with [aria-describedby] attribute.

Some context:

@Reinmar
Copy link
Member

Reinmar commented Jun 21, 2018

cc @oleq @dkonopka

@Reinmar Reinmar added type:bug This issue reports a buggy (incorrect) behavior. status:discussion package:link package:ui labels Jun 21, 2018
@oleq
Copy link
Member

oleq commented Jun 25, 2018

I check this out and simply adding aria-hidden="false" to the <label> fixed the issue in VoiceOver. Can you confirm this is an acceptable approach, @Comandeer? Because it's the simplest one I could think of.

@Comandeer
Copy link
Member Author

Well, it fixes issue for users of screen readers, but all other concerns (described in linked articles) are still there.

@oleq
Copy link
Member

oleq commented Jun 26, 2018

but all other concerns (described in linked articles) are still there.

If you mean concerns about discoverability (placeholder disappears when the user is typing), we discussed that during the design phase and dismissed it because there's no way this form could surprise anyone; it's either:

  • displayed after the user clicked the link button in the toolbar,
  • displayed after the user clicked a link, and then decided to edit it

Besides, even FB uses placeholders for their sign up form ;-)

Anyway, we decided to take some risks to gain a more compact and modern-looking UI. And mostly we were aware of them. Still if we

  • make sure the inputs placeholders pass WCAG AA (they do),
  • (TODO) unblock the labels,
  • (TODO) and provide an additional description using aria-describedby

then, I think, we're on the safe side, despite other potential concerns.

@Comandeer
Copy link
Member Author

Besides, even FB uses placeholders for their sign up form ;-)

And that's a part of why FB is not considered super accessible :P

make sure the inputs placeholders pass WCAG AA (they do)

In such case placeholder could be mistaken with filled field (I have issues with distinguishing between them).

But yes, making label visible for screen readers and adding the description (however this description will be available only for screen-readers users – and it could be helpful for anyone) should fix the biggest issue with this input.

@oleq
Copy link
Member

oleq commented Jun 27, 2018

I'll create a simple PR for this issue.

@oleq oleq self-assigned this Jun 27, 2018
@oleq oleq removed their assignment Nov 23, 2018
@Comandeer
Copy link
Member Author

The same situation (incorrect label) is present also for other inputs in the editor, e.g. media embed one.

@oleq oleq added the module:ux label Dec 11, 2018
@Comandeer
Copy link
Member Author

Some more context: https://codepen.io/stevef/post/placeholder-the-piss-take-label#show-us-the-fail-they-ask-1

This indicates that label should be visible the whole time.

@oleq oleq added this to the next milestone Jun 13, 2019
@mlewand
Copy link
Contributor

mlewand commented Jun 19, 2019

I believe ideal solution to that is a label, that is shown the way placeholders are (so inside the input but is distinguishable from a user provided text with styling) and as user provides input the label shrinks and move to the top of the input, kind of overlapping with its boundaries.

field label and no text provided by the user

field with both label and user provided text

@oleq
Copy link
Member

oleq commented Jun 24, 2019

@mlewand
Copy link
Contributor

mlewand commented Jul 15, 2019

@oleq yes, that's exactly what's on my mind.

@mlewand mlewand added the domain:accessibility This issue reports an accessibility problem. label Jul 15, 2019
@oleq
Copy link
Member

oleq commented Jul 16, 2019

I created a PoC with the "material approach". The only drawback (so far) I see now is that we're gonna need more padding around forms so balloons like link or text alternative will be bigger.

Kapture 2019-07-16 at 17 30 20

The code:

@mlewand
Copy link
Contributor

mlewand commented Jul 17, 2019

@oleq looks nice to me. Any reason why you dropped a thicker outline upon focus?

Now it looks inconsistent with buttons next to it, buttons in the toolbar etc.

@oleq
Copy link
Member

oleq commented Jul 17, 2019

@mlewand Doesn't look good with the notch IMO

image

But maybe it needs some polishing.

@Reinmar
Copy link
Member

Reinmar commented Jul 19, 2019

I had to compare your proposal with what we have right now, to realise that what we currently see is:

image

So, there will be one more change – the placeholder (usually – an URL) will be replaced with a label.

Which makes the description in media embed dialog unnecessary:

image

Which causes a problem with the fact that now it can be seamlessly replaced with this tip:

image

BTW, I think that the code that you pushed (the two branches) don't work:

image

image

I guess more changes are needed than just in the UI and theme packages.

@oleq
Copy link
Member

oleq commented Jul 19, 2019

The code is incomplete. You have to increase the padding of the balloons manually. And I didn't even consider the media balloon; it's a PoC after all.

@Reinmar
Copy link
Member

Reinmar commented Jul 19, 2019

Regarding issues that you were discussing here:

  • The bigger padding... I think we can't avoid that if we want that label somewhere. If we'll have the label above the input (classical approach), then we'd have a vertically unsymmetrical form for the link. Using the material design approach will allow us to keep it symmetrical, which is fine.

  • I'd consider a slightly bigger height of URL input – the text is a bit too close to the label. Perhaps something like (I added 3px there):

    image

    I think it composes well with the bigger balloon padding.

  • I'd do our best to try to keep our current focus ring. I personally really like it. And it'd be good if we could have it consistent with the rest of the buttons. Unfortunately, I can't test myself how would that work (your screenshot doesn't show the input border which is present on current master) because I can't find where it's defined πŸ˜…

@oleq
Copy link
Member

oleq commented Jul 22, 2019

  • I'd consider a slightly bigger height of URL input – the text is a bit too close to the label. Perhaps something like (I added 3px there):

It's a bummer then πŸ˜• The height of the input is in sync with the height of toolbar buttons. Increasing the height of the input will bloat toolbars. It's a weird and complex thing because it's a part of the UI scalability using variables system etc..

@Reinmar
Copy link
Member

Reinmar commented Oct 8, 2019

Agree. And perhaps we can count on some feedback if we link to this from the milestone ticket. Because we tried to gather some feedback, but there wasn't any so far.

@Reinmar
Copy link
Member

Reinmar commented Dec 10, 2019

@oleq Could you refresh the constellation of branches? I tried to merge master to those branches but there are some conflicts.

@oleq
Copy link
Member

oleq commented Dec 11, 2019

It's done. Check out https://github.com/ckeditor/ckeditor5/tree/t/ckeditor5/1098-poc for the latest version.

@oleq
Copy link
Member

oleq commented Aug 28, 2020

I revived the PoC in #7952 (migrated to monorepo, migrated table forms) and I think it's time to think it over, hence this RFC.

πŸ‘‰ Here you can check out the latest PoC with the new features live πŸ‘ˆ


Context and Problem Statement

As @Comandeer pointed out in his original issue, inputs displayed in the UI of the editor (e.g. link URL, etc.) are inaccessible. There are 2 problems with the current approach:

  1. screen readers don't read those fields properly because we hide their labels (display: none),
  2. when a field contains some text the placeholder disappears and because the label is hidden as well there is no way to tell the purpose of the field (check out the screenshot below: what does the balloon do?)

Back in 2019, I came up with a proposal that implements material design–like inputs in CKEditor 5 to address both problems.

The PoC was forgotten πŸ˜• because there were different priorities back then but now I decided to revisit it and see how it performs with new forms like table properties or table cell properties.

Decision Drivers

  • Editor UI fields are inaccessible to screen readers,
  • Editor UI fields are inaccessible to regular users, they might be confusing in some cases,
  • Editor UI fields look dated (debatable).

Considered Options

Do "next to nothing"

  • Use aria-hidden="false" for labels to address the issue with screen readers because it is a cheap fix (1st problem)
  • Ignore the second problem, we've been living like this for years.

Display labels next to fields.

We could enable the labels before fields (they are already in DOM, it's just a matter of removing display: none):

Note: It does not change the look of table forms as the labels are already there under the fields (or before them when there is enough space).

Adopt the material UI approach

Check out the live demo.Β 

The idea is to adopt the Material approach to text fields but in a slightly more compact way because space our UI can use is very limited. In this approach, labels are always visible but there is little to no sacrifice in terms of space or layout.

Pros and Cons of the Options

Do "next to nothing"

Pros

  • πŸ‘ very quick: it will take just a few hours to finish, it's all about the ARIA attribute,

Cons

  • πŸ‘Ž it solves only 50% of our problems, it does not solve the second issue (the label-less fields are confusing)

Display labels next to fields.

Pros

  • πŸ‘ it solves both problems,

Cons

  • πŸ‘Ž it mangles the layout and the symmetry of some forms, for instance, link form or text alternative form,
    • the whole idea was these forms remain compact and linear (buttons perfectly aligned with the input),
  • πŸ‘Ž since the label always precedes (or follows) the input, some forms will consume more space,
  • πŸ‘Ž this may mean some changes in the UI of CF (provided it uses the LabeledFormView component), either the project will also change the approach or it will need to add the display: none for labels back

Adopt the material UI approach

Pros

  • πŸ‘ it solves both problems,
  • πŸ‘ it looks modern (debatable),
  • πŸ‘ it requires minimal space overflow (when compared to the 2nd option),

Cons

  • πŸ‘Ž it brings a considerable amount of new code to the project,
  • πŸ‘Ž it increases the complexity of components (for instance, new data bindings were necessary to implement this, not just CSS),
  • πŸ‘Ž it may give an impression of chaos because there's a lot going on (animations, labels moving around)
  • πŸ‘Ž it may require some actions in CF (provided it uses the LabeledFormView component),
    • on the second though, whether it uses the component or not, it would be great if UIs of both projects were consistent so this may require some refactoring in CF after all

@niegowski
Copy link
Contributor

niegowski commented Aug 28, 2020

I really like the material UI approach with a few comments:

  • Table properties and Cell properties balloons:
    • Both are smaller in height but they give the first impression of being crowded.
    • "Background color" should not be grouped together with "Border". This way it looks inconsistent and if the group label would be there it would add some space for more readability. Also missing the "Background" group label makes rows of fields spread inconsistent.
  • Table properties balloon:
    • "Dimensions" and "Alignment" labels are vertically not aligned.
  • Cell properties balloon:
    • "Table cell text alignment" breaks vertical spacings (other group labels have bigger spacings from fields).
  • Some labels seem to be cropped (like "Height", "Padding" on the images below).
  • On some small forms transitions of label behave differently on form appearing (for the same form animation is visible but after reopening of the same form there is no animation). Also, it makes it feel "woo what just happened". I think that labels should not animate on the form appearing (only as of the transition for focus).

@mlewand
Copy link
Contributor

mlewand commented Aug 31, 2020

For me, I sad it once, I say it again: I really like the idea of introducing this material ui concept. I also like the way you implemented it πŸ‘πŸ‘πŸ‘

I'm all for it as it is IMHO the best of all the solutions. We also already invested quite some time, so it makes sense to convert this into a tangible value in CKE5.

I also recall that we explicitly asked for community feedback back in iteration 29. We did not get any objections to the proposal, so we can see that there are objections out there.

Few minor details:

  • The focus (border) animation is too slow. The text transition speed is fine πŸ‘Œ
  • Given recent changes in image plugin dropdown, it looks really messy and crowded now (especially with this change):

I think it make sense to extract such details as a followup not to stop this big change.

@Reinmar
Copy link
Member

Reinmar commented Oct 26, 2020

A ready PR with screenshots landed in #8267.

@oleq oleq modified the milestones: next, iteration 38 Oct 26, 2020
@Reinmar Reinmar added the bc:major Resolving this issue will introduce a major breaking change. label Oct 26, 2020
Reinmar added a commit that referenced this issue Nov 5, 2020
Feature: Made input labels accessible across the editor UI. Closes #1098. Closes #8242.

MAJOR BREAKING CHANGE (theme-lark): The look and behavior of the `LabeledFieldView` UI component (used for displaying fields across the project) have changed. This may require changes in your integration if it customizes the `.ck-labeled-field-view` selector (or its internals).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bc:major Resolving this issue will introduce a major breaking change. domain:accessibility This issue reports an accessibility problem. package:link package:ui squad:core Issue to be handled by the Core team. status:discussion type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants