Improve the functionality of aria-pressed attribute in button elements

[msamsel]

Suggested merge commit message (convention)

Fix: Improve the functionality of aria-pressed attribute in button elements. Closes ckeditor/ckeditor5#1403.

---

Additional information

sub-pr:

• Set UI buttons as toggleable ckeditor5-alignment#44
• Set UI buttons as toggleable ckeditor5-basic-styles#95
• Set UI button as toggleable ckeditor5-block-quote#40
• Set UI buttons as toggleable ckeditor5-heading#128
• Set UI buttons as toggleable ckeditor5-highlight#43
• Set UI buttons as toggleable ckeditor5-image#303
• Set UI buttons as toggleable ckeditor5-link#246
• Set UI buttons as toggleable ckeditor5-list#135
• Set UI button as toggleable ckeditor5-paragraph#46

[mlewand]

By adding aria-pressed attribute you mark the button as a toggle button.

Your proposal adds this attribute to every button and not all buttons are toggle buttons.

Buttons that are toggle buttons: bold, highlight, list, link.

Buttons that are not toggle buttons: undo, image, remove format.

[coveralls]

Coverage decreased (-0.3%) to 99.733% when pulling 5edce86 on t/ckeditor5/1403 into 1635784 on master.

[msamsel]

Your proposal adds this attribute to every button and not all buttons are toggle buttons.

I've modified existing behaviour, which has added aria-pressed="true" to every button. So as I understand this attribute should be added only for toggleable buttons.

I see 2 ways how to achieve that:

• provide configuration object passed to the constructor which define button as a toggle
• create child class ToggleButton which will extend Button

Both approaches require multiple changes in many repositories, that's why I'd like to have sure that the chosen solution will be better.

As the first one seems to be a little bit better for me, that's why I've prepared some sample solution:
5edce86

@oleq can you give me feedback related to this case? Which approach seems to be better for this case or maybe there is some other way?

// cc @mlewand

[mlewand]

BTW we already have a SwitchButtonView class. Which acts like toggle button but looks differently. Maybe we could reuse SwitchButtonView and allow it to take different visual forms?

The only issue is that IMHO switch button implies a slightly different semantics than toggle button. Toggle button for me is more of a boolean state, always on/off, while switch button sounds like a choice between two values, e.g. "wide/narrow", "black/white", "black/pink" etc.

[msamsel]

Maybe we could reuse SwitchButtonView and allow it to take different visual forms?

This sounds like overkill for me. To force the switch button, which inheritance from the button, to have a different visual form which will look like a regullar button but with defined aria-pressed attribute.

I think better could be to have the Button class with configuration option toggle and the SwitchButton class which inheritance from Button and always call super.constructor() with toggle configuration option set to true.

[oleq]

KISS. Cake recipe:

1. Let's create an observable property ButtonView#isToggleable = false. Document what impact it has on the button.
2. Let's bind

   'aria-pressed': bind.to( 'isOn', value => this.isToggleable ? String( value ) : false )

3. Let's set isToggleable in places like BoldUI, LinkUI, etc. (but not UndoUI)
4. Let's set this.isToggleable = true in SwitchButtonView#constructor()

We don't need new constructor arguments. We incline towards observables in the project because one can change them during the life cycle of the component.

We don't need additional clas chains like ButtonView -> ToggleButtonView -> SwitchButtonView. There's no need to create a separate class (ToggleButtonView) just to add an attribute to the template. The class would be almost empty. This is not the right way to OOP.

[msamsel]

Ad 2.
If I understand correctly previous comment: #517 (review) we should not add aria-pressed attribute for non-toggleable buttons. @mlewand please correct me if I wrong.

[oleq]

When the bind.to() callback returns false (Boolean, not String), the attribute will not be present in DOM.

[mlewand]

we should not add aria-pressed attribute for non-toggleable buttons

That's correct, we must not add aria-pressed attr for non-toggle buttons.

'aria-pressed': bind.to( 'isOn', value => this.isToggleable ? String( value ) : false )

I'm not proficient yet with this API but my understanding is that if bind.to gets false it does not add an attribute, which is fine.

We don't need additional clas chains like ButtonView -> ToggleButtonView -> SwitchButtonView.

My suggestion was not necessarily add another class here. Nonetheless additional property is ok.

[oleq]

Just to make things clear, buttons that are not toggleable (like undo, redo, etc.) should not have aria-pressed at all, right @Comandeer?

If the attribute is omitted or set to its default value of aria-pressed="undefined", the element does not support being pressed.

via https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role.

---

It looks like the "Print page" button in this example confirms my assumptions.

[mlewand]

should not have aria-pressed at all

Yes, regular buttons must not have aria-pressed attr. That's the cause of R- in #517 (review)

The "undefined" value is the default value of this attribute, so there's no benefit in setting it (https://www.w3.org/TR/wai-aria-1.1/#aria-readonly).

[msamsel]

I add observable isToggleable property to ButtonView and create fixes in other repositories to use it.