ckeditor / ckeditor5

Powerful rich text editor framework with a modular architecture, modern integrations, and features like collaborative editing.
https://ckeditor.com/ckeditor-5
Other
9.56k stars 3.7k forks source link

Inaccessible input for link's URL #1098

Closed Comandeer closed 4 years ago

Comandeer commented 6 years ago

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 commented 6 years ago

cc @oleq @dkonopka

oleq commented 6 years ago

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 commented 6 years ago

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

oleq commented 6 years ago

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:

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

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

Comandeer commented 6 years ago

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 commented 6 years ago

I'll create a simple PR for this issue.

Comandeer commented 5 years ago

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

Comandeer commented 5 years ago

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.

mlewand commented 5 years ago

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 commented 5 years ago

As in https://material.io/design/components/text-fields.html#outlined-text-field?

mlewand commented 5 years ago

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

oleq commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@mlewand Doesn't look good with the notch IMO

image

But maybe it needs some polishing.

Reinmar commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Regarding issues that you were discussing here:

oleq commented 5 years ago
  • 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 commented 5 years ago

Doh, I didn't think about this. So, the only way around would be to move the label a few px higher, so it leaves enough space for the text inside the input. Which means that the balloon's padding would need to be increased (perhaps even more than you proposed).

IDK... it's hard to be happy about such a change because visually it's definitely for worse :D Maybe we just need a more concrete proposal (something that we can really test), agree to what we can improve in it and just accept it as it is.

Unless anyone have a better idea?

oleq commented 5 years ago

What exactly is wrong with https://github.com/ckeditor/ckeditor5/issues/1098#issuecomment-511868090? I don't think the distance between the label (inside the notch) and the input text is an issue here.

Reinmar commented 5 years ago

A little crowdy for me. And I find the inconsistency in input vs button's outlines disturbing. The former is not a blocker for sure, but IDK about the latter. In fact, is such a focus style accessible? Because we only change the color of the border. We also move the label, but IDK if that can be considered an "enough" solution.

oleq commented 5 years ago

πŸ‘‰ Live demo with the PoC πŸ‘ˆ

It includes the latest code with changes in ckeditor5-link, ckeditor5-media-embed, and ckeditor5-image.

If you'd like this PoC, add a πŸ‘ reaction to this post. If you find any issues or suggestions, please add a comment with your feedback in this issue.


Concerns and TODOs

  1. If we went with the design in the PoC (or similar), what impact would it have on Letters/Cloud features? cc @pjasiun @oskarwrobel
  2. Maybe the new design should be a separate ckeditor5-ui component?
    1. OTOH, what's the point of making things accessible when we leave an inaccessible component in our codebase?
    2. OTTH, this will definitely be a breaking change and it could seriously affect 3rd–party integrations. Can we do anything about it?
  3. I removed input placeholders to have a large label when the user uses the feature for the first time (e.g. inserting a new link). Was that a good decision?
  4. Related to the above, we should probably figure new helper text for the media embed panel.
  5. We need to double-check if the contrast of labels in all their states matches WCAG standards.
  6. What does the small label in the notch look like in LoDPI?

If you want to play with the code, use the "constellation branch" I prepared.

Reinmar commented 5 years ago

I think I like it, but I looked too many times and saw too many versions of it so I'm not sure about anything right now.

Some minor things:

oskarwrobel commented 5 years ago

If we went with the design in the PoC (or similar), what impact would it have on Letters/Cloud features? cc @pjasiun @oskarwrobel

No worries, In the worst scenario, we will need just to adjust some styles.

dkonopka commented 5 years ago

Just to add, I'd consider a slower label/placeholder movement animation (from .1s to .2s) - it's simply too fast. Additionally, it will be unified with buttons where we are using 200ms in the focus state. Besides that, it looks cool, gj!

mlewand commented 5 years ago

IMHO the proposal looks great. The thick border looks fine.

  • It doesn't look like a placeholder. I think it should be greyish.

πŸ‘

@dkonopka when reading slower movement proposal I sounded like I want to second to slower transition. But once I applied it, .2 and .1 make a perceivable difference. Actually .1 feels much more zippy and smooth. .2 while allows you to better see what's happening, it might be annoying in a long run. And let's not forget this is just an eye-candy, the most important thing is that the end user can effectively use the field and finish the job.

dkonopka commented 5 years ago

@mlewand Ok, TBH I didn't check material design guides and as I can see they are recommending 100ms for small UI elements ( switches, checkboxes ) - https://material.io/design/motion/speed.html#duration.

However, what's interesting they are using 150ms for text field label: https://material.io/design/components/text-fields.html.

In my opinion, if we are implementing material.io based design, we should follow the animation property too - I mean 150ms.

mlewand commented 5 years ago

Since we have already put some real work into this issue, it would be nice to add finishing touches and close the feature. So I'm moving this into next milestone, hopefully we'll be able to tackle it in future iterations.

Reinmar commented 5 years ago

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 commented 4 years ago

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

oleq commented 4 years ago

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

oleq commented 4 years ago

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

Considered Options

Do "next to nothing"

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

Cons

Display labels next to fields.

Pros

Cons

Adopt the material UI approach

Pros

Cons

niegowski commented 4 years ago

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

mlewand commented 4 years ago

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:

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

Reinmar commented 4 years ago

A ready PR with screenshots landed in #8267.