#1098: Made input labels accessible across the editor UI

[oleq]

Suggested merge commit message (convention)

Feature: Made input labels accessible across the editor UI. Closes #1098. Closes #8242.

BREAKING CHANGE (theme-lark): The look and behavior of the LabeledFieldView UI component (used for displaying fields across the project) has changed. This may require changes in your integration if it customizes the .ck-labeled-field-view selector (or its internals).

---

Additional information

⚠️⚠️⚠️ This PR should be merged at the beginning of an iteration because the scope is so vast and it will take some time to detect possible issues by the team. ⚠️⚠️⚠️

Some notes for QA:

• Below there are screenshots of places affected by this PR.
• Make sure this PR is tested against other languages (e.g. German, which has longer labels).
• Make sure this PR does not break CF.

Before/after

[oleq]

@ckeditor/qa-team Can you take a look at this PR?

[oleq]

@Reinmar There's one architectural aspect of the PR I'd love to know your opinion about.

[FilipTokarski]

In table properties and cell properties balloons - border color and background color inputs have empty labels

[FilipTokarski]

I checked it with screen readers on mac an win, on different browsers and different languages. Apart from lack of color labels reported above, the rest seems to be fine 👍
In some languages the label's text is longer than the input, but it gets truncated with ... and it looks ok, so I guess it's not an issue.

[oleq]

In table properties and cell properties balloons - border color and background color inputs have empty labels

A nice catch! I'm not sure this is in the scope of this PR, though. In this PR we're talking about visual accessibility while this is screen reader accessibility and this problem also occurs on master. 

Anyway, I checked this in VoiceOver and the screen reader fails to read the label for the input. I'm not sure how to fix it, though, because the field is more complex than this. @Comandeer can you give us some thoughts on this?

[oleq]

A nice catch! I'm not sure this is in the scope of this PR, though. In this PR we're talking about visual accessibility while this is screen reader accessibility and this problem also occurs on master.

I meant: let's have a follow-up for this 😛

[Reinmar]

The contrast difference between a placeholder and filled values is IMO too low. I wonder if this makes it harder to understand whether a field is filled:

This is the limit value for WCAG AA:

IDK if reducing the contrast to 4.5 makes sense as the placeholder text in the input is very important (it tells you what that field is about). However, such a form is easier to understand to me.

Alternatively, we could make the default text a bit darker and keep the current placeholder color. However, we should then make the icons stop following this variable (they'd otherwise get darker too):

[Reinmar]

This is a blocker:

[Reinmar]

Not sure if it's supposed to work on http://localhost:8125/ckeditor5-theme-lark/tests/manual/theme.html where it says that it's supposed to be labeled but there's no label:

[Reinmar]

[Reinmar]

One more, less obvious thing about RTL - the label should be on the right:

The placeholder too:

[Reinmar]

As for disableCSSTransition:

It's unclear for me from the documentation when this is applicable. My guess is that when we display some form with one of the fields pre-focused, then we don't want to show the transition. Is that correct? If so, then, it'd be lovely to update this documentation with more info.

Other than that – disableCssTransitions().

And finally – the solution is far from perfect in this sense that it leaks presentational logic to JS. Unfortunately, I suppose there's no other way around this and thus:

• We can live with this because it is what it is and there's no point in lying about this.
• We can try to generalize it in some fancy way but, hey, leaky abstractions...
• We can try to combine it with LabeledInput's focus()? But what if we have some non-focus-related transition? OTOH, what if some non-focus-related transition should actually happen while the focus-related one should not? The current naming groups all them.
• We can try to find some slightly more "JSy" name but I can't think of any.

I can see it is used in 3 places, so it's not bad. If you want to keep it this way, I'd be fine with that.

If you'll decide to keep this solution, I think that classes that mix it (like TextAlternativeFormView) should either implement some additional interface?

[Reinmar]

As commented. The RTL issues are blockers. The test issues probably not but since we're not in a hurry and these might be smaller things, it'd be good to fix these too. Finally, as for the whole disabling transitions problem – we may KISS. The naming would need to be fixed and the interface thing.

[Reinmar]

And finally – the solution is far from perfect in this sense that it leaks presentational logic to JS. Unfortunately, I suppose there's no other way around this and thus:

There's another option, BTW, to move these methods to an object inside a view. Like:

this.transitionDisabler = injectTransitionDisabler( this );

And then use:

someView.transitionDisabler.turnOn();

The idea here would be to not have two additional methods and a property on a view, but have them grouped.

[oleq]

This is a blocker:

Addressed in 34d3a80 and ec85982.

---

One more, less obvious thing about RTL - the label should be on the right:

Addressed in 34d3a80.

---

Regarding the dark theme, I created a follow-up a few weeks ago #8275.

---

It's unclear for me from the documentation when this is applicable. My guess is that when we display some form with one of the fields pre-focused, then we don't want to show the transition. Is that correct? If so, then, it'd be lovely to update this documentation with more info.

 * A decorator that brings possibility to temporarily disable CSS transitions using
* {@link module:ui/view~View} methods. It is helpful when, for instance, the transitions should not happen
* when the view is first displayed but they should work normally in other cases.

It's pretty clear to me what's the purpose of this helper. It has nothing to do with inputs (although I use it for this purpose). It could be any transition on any DOM element.

And finally – the solution is far from perfect in this sense that it leaks presentational logic to JS. Unfortunately, I suppose there's no other way around this and thus:

IMO the only sane way to maybe improve this is to narrow the use-case to the inputs and their focus (instead of entire forms in my implementation) because currently, this is the only use case. The implementation of injectCSSTransitionDisabling would not change, though. So ¯\_(ツ)_/¯ .

There's another option, BTW, to move these methods to an object inside a view. Like:

I'd rather not make it available across all views in the editor when there are only a few use-cases. I like the decorator pattern.

[oleq]

Not sure if it's supposed to work on http://localhost:8125/ckeditor5-theme-lark/tests/manual/theme.html where it says that it's supposed to be labeled but there's no label:

It looks like it was broken before. Fixed in 243b0f5.

[oleq]

This is the limit value for WCAG AA:

Increased contrast in 9fad801.

Alternatively, we could make the default text a bit darker and keep the current placeholder color. However, we should then make the icons stop following this variable (they'd otherwise get darker too):

Can we discuss it in a follow-up? I think the change that I mentioned ☝️ makes it good enough and we can think about it later. Decoupling icon color and text color would be a serious breaking change because the whole UI/theme depends on this coupling.