feat(NcActionButton): Allow pressed state on NcActionButton - similar to NcButton #4744

susnux · 2023-11-01T23:44:35Z

☑️ Resolves

Fix Actions: "active" style doesn't work #425

NcButton already allows a checked state so this introduces this for the NcActionButton.
In checked state the aria-pressed attribute will be added and a primary border will indicate it visually.

🖼️ Screenshots

vokoscreenNG-2023-11-02_00-37-54.mp4

🏁 Checklist

⛑️ Tests are included or are not applicable
📘 Component documentation has been extended, updated or is not applicable

raimund-schluessler · 2023-11-02T07:35:04Z

I think we went a bit away from showing the active state with a border at the left, no? But if it's approved, fine with me.

jancborchardt

Yep, that’s correct @raimund-schluessler – the specs in the issue are quite old and from a time where we used that style.

Since we are generally close to Material Design, what they do is show a checkmark on the right – see "list item trailing icon" at https://m3.material.io/components/menus/specs
Then we also do not have any issues with contrast, regardless of the color.

Also, not sure if related, but in that example there is a bit too much whitespace below the last entry. It should be exactly the same as on the left and right (4px likely), but seems more.

susnux · 2023-11-02T12:47:01Z

Also, not sure if related, but in that example there is a bit too much whitespace below the last entry. It should be exactly the same as on the left and right (4px likely), but seems more.

No this is unrelated and a different issue.

Since we are generally close to Material Design, what they do is show a checkmark on the right

I am not sure about this one this will either be very "flashy" as the checkmark will disappear on disable and then the menu resizes or we add a quite huge margin for all button on the right side.
Maybe we can go with "activated" state of material design? This would be even somewhat consistent with NcButton which is primary on "activated", ref: https://m3.material.io/foundations/interaction/states/applying-states#f7145f12-7c29-4870-b587-b638833b2061

Example:

vokoscreenNG-2023-11-02_13-43-42.mp4

This also works for this corner case (groups):

vokoscreenNG-2023-11-02_14-02-27.mp4

susnux · 2023-11-02T12:57:25Z

With an icon but without margin it would look like this:

vokoscreenNG-2023-11-02_13-53-58.mp4

And with additional margin on "un-pressed":

vokoscreenNG-2023-11-02_13-55-03.mp4

jancborchardt · 2023-11-02T15:49:16Z

Thanks for the examples @susnux! :) I think the variant with the checkmark and the additional marking of course (so it doesn’t jump) is the best one.

The background color change wouldn’t pass contrast requirements unless we change it to primary-element background with white text – which we could also do, just like in Nextcloud Office or Text.

susnux · 2023-11-02T16:44:03Z

The background color change wouldn’t pass contrast requirements unless we change it to primary-element background

@jancborchardt no this is fine, I tested that combination (the PR on server were we adjusted the colors so that common combinations work find (here it is primary-element-light + primary-element-light-text)

I am fine with the icon, the only down side is it can not be used in the button group example (see video)

raimund-schluessler · 2023-11-02T16:49:50Z

We could do both. An icon and the background change.

JuliaKirschenheuter · 2023-11-03T08:22:08Z

Thank you @susnux!

My impression regarding contrasts on selected / pressed button: if there is no other indicator (like check icon) then contrast have to be 3:1 (focused <-> unfocused, focused <-> surrounding background) like it was here nextcloud/text#3860

Could we use another outline or background color?

susnux · 2023-11-07T10:48:50Z

What is the suggested active state? Border does not work because it will be overridden by the focus state, as focus uses the border instead of outline. So only the fill color or icon is possible.
I think primary will be a bit too much?

JuliaKirschenheuter · 2023-11-07T15:50:16Z

Ok, we can move with a "checked" icon. But then we have to notify a screen reader that something happened (checked, unchecked) via aria-label i think.

susnux · 2023-11-07T15:55:55Z

But then we have to notify a screen reader that something happened (checked, unchecked) via aria-label i think.

This is already done by aria-pressed. This PR is about the visual representation of it.

JuliaKirschenheuter · 2023-11-07T16:01:34Z

This is already done by aria-pressed. This PR is about the visual representation of it.

Adding aria-pressed to an element with a role of button turns the button into a toggle button. The aria-pressed attribute is only relevant for toggle buttons.

not sure that aria-pressed is ok for this case. Do we have a real toggle button inn that case?

susnux · 2023-11-07T16:38:35Z

Yes this is about toggle button, like toggle between full screen view and windowed. Or toggle left align etc.

JuliaKirschenheuter · 2023-11-07T16:42:33Z

Yes this is about toggle button, like toggle between full screen view and windowed. Or toggle left align etc.

I've just asked myself because it is like a normal button inside a "normal" menu. Let's leave it like toggle button 👍

JuliaKirschenheuter

fine to me!

susnux · 2023-11-08T00:40:37Z

So currently it looks like this:

vokoscreenNG-2023-11-08_01-39-27.mp4

ShGKme · 2023-11-09T19:34:06Z

What should it be used instead of NcActionCheckbox?

ShGKme · 2024-01-10T16:35:22Z

I think, this can be useful for NcActions but in a bit different way to cover all the cases and a11y requirements. @susnux what do you think?

Current usage

NcActionButton can be used in 3 ways:

An inline button like a normal button.
- This could be a toggle button, so pressed state makes sense with aria-pressed
- It is rendered as NcButton that already have a pressed feature.
  We only need a way to set NcButton.pressed via NcActionButton
- Example - toolbar in text`. Bold and Italic are toggle buttons
In a menu as a menu item button with radio behavior
- We have NcActionRadio already that represents a role="menuitemradio" and manages aria-cheched
- But we want to have it in a button design, like in this PR
- ? A problem here - how to determine a group ?
- Examples:
  
  Personal Settings: visibility scope Text menu bar: callout, heading options
In a menu as a menu item button with checkbox behavior
- Same as p.2 but with a menuitemcheckbox role
- No issues with making it a group
- Examples: none
In a NcActionButtonGroup
- Same as p.2 and p.3 but in a group
- No problem with grouping a radio
- Example - Table cell alignment in text:
In a popup as a button in a dialog
- Same as p.2 and p.3 but when it is not in a role="menu"
- Examples: none

The most problematic part in my opinion - menu item with a radio behavior. Because there is no way to specify a group. My idea - allow a menu to have no more than one radio group (use NcActionButtonGroup to create a group). In future we can add horizontal layout to NcActionButtonGroup).

Proposal

Add a new prop that represents pressed/checked state. For example, selected to be more universal and not associated with a specific type nor HTML attribute. Or modelValue =D
Add a new prop that defines if it is a radio or checkbox (= toggle button). Like type in NcCheckboxRadioSwitch. For example, behavior: 'toggle' | 'radio'.
- Should it has also value prop to act like a real radio?
Use it to define:
- NcButton's pressed prop in inline buttons
- role="menuitemcheckbox", role="menuitemradio" with aria-checked in menus with this new design
- aria-pressed when used not in a menu with the same new design

Alternative solution

Add button-like design to NcActionCheckbox and NcActionRadio, but it seems more complicated to me.
Add NcActionButtonCheckbox and NcActionButtonRadio to wrap NcActionButton instead of a new prop.

susnux · 2024-01-19T17:47:43Z

@ShGKme I fully agree with your points and I think your proposal is the best way to implement this.
I decided to also support a real value for the radio semantics, this has some benefits like making it easier to interact with the component :modelValue.sync instead of @click="state = 'x'".

Please feel free to review :)

src/components/NcActionButton/NcActionButton.vue

ShGKme

Everything looks cool, seems to work and covers all the use-cases having good a11y, thanks!

Just some nitpicks/thoughts

src/components/NcActionButton/NcActionButton.vue

src/components/NcActions/NcActions.vue

src/components/NcActionButtonGroup/NcActionButtonGroup.vue

src/components/NcActionButton/NcActionButton.vue

Changed design

susnux · 2024-01-23T14:41:15Z

@ShGKme could you give this a try for the 8.5.0 release?

ShGKme · 2024-01-23T15:59:17Z

src/components/NcActionButton/NcActionButton.vue

+		/**
+		 * The buttons state if `type` is 'checkbox' or 'radio' (meaning if it is pressed / selected)
+		 * Either boolean for checkbox and toggle button behavior or `value` for radio behavior
+		 */
+		modelValue: {
+			type: [Boolean, String],
+			default: null,
+		},


It's said "if type is 'checkbox' or 'radio'" but it also changes attributes on other types.

And I think it's worth mentioning that null (the default value) in a specific case (in a menu with type checkbox) actually means mixed, not false.

Do we actually need this mixed value? It is not supported by NcButton, and there is no UI for this value. If feels a bit complicated to me...

I added more documentation on this behavior

ShGKme · 2024-01-23T16:00:20Z

@ShGKme could you give this a try for the 8.5.0 release?

Yes, but it also requires #5119

Otherwise it is not a11y for inline buttons...

… to NcButton * Use trailing icon for pressed state Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

…elected button Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

…ked states * This introduces new props `modelValue`, `modelBehavior` and `radioValue` Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

…`NcButton` for inline actions Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

susnux added enhancement New feature or request 3. to review Waiting for reviews feature: actions Related to the actions components labels Nov 1, 2023

susnux requested review from jancborchardt, raimund-schluessler, JuliaKirschenheuter, skjnldsv and Pytal November 1, 2023 23:44

jancborchardt previously requested changes Nov 2, 2023

View reviewed changes

skjnldsv removed their request for review November 7, 2023 10:53

susnux mentioned this pull request Nov 7, 2023

chore: Prepare release of stable version 8.0.0 #4765

Merged

16 tasks

JuliaKirschenheuter approved these changes Nov 7, 2023

View reviewed changes

susnux force-pushed the feat/action-button-active branch from c0b04f1 to d8701b7 Compare November 8, 2023 00:39

susnux requested a review from jancborchardt November 8, 2023 00:40

susnux added this to the 8.1.0 milestone Nov 8, 2023

ShGKme added this to the 8.5.0 milestone Dec 27, 2023

susnux force-pushed the feat/action-button-active branch from d8701b7 to 5ff8f0e Compare January 19, 2024 16:06

susnux force-pushed the feat/action-button-active branch from c91b8a3 to cc6ce59 Compare January 19, 2024 17:48

susnux commented Jan 19, 2024

View reviewed changes

src/components/NcActionButton/NcActionButton.vue Outdated Show resolved Hide resolved

susnux force-pushed the feat/action-button-active branch 3 times, most recently from e09abbb to 7836434 Compare January 19, 2024 19:40

ShGKme reviewed Jan 19, 2024

View reviewed changes

susnux force-pushed the feat/action-button-active branch 4 times, most recently from 36f1311 to 2c795b9 Compare January 23, 2024 00:41

susnux requested a review from ShGKme January 23, 2024 00:42

susnux mentioned this pull request Jan 23, 2024

chore(release): v8.5.0 #5113

Merged

ShGKme reviewed Jan 23, 2024

View reviewed changes

ShGKme approved these changes Jan 23, 2024

View reviewed changes

susnux added 5 commits January 23, 2024 18:00

feat(NcActionButton): Allow pressed state on NcActionButton - similar…

557629c

… to NcButton * Use trailing icon for pressed state Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

fix(NcActionButtonGroup): Do not use icon but primary color to mark s…

88e82be

…elected button Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

enh(NcActionButton): Fully support sematic roles for pressed and chec…

b1c392f

…ked states * This introduces new props `modelValue`, `modelBehavior` and `radioValue` Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

fix(NcActionButton): Provide type prop for behavior and forward to …

9e15e67

…`NcButton` for inline actions Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

fix(NcActionButton): Add some more documentation

00b1c74

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

susnux force-pushed the feat/action-button-active branch from e1816e4 to 00b1c74 Compare January 23, 2024 17:00

susnux merged commit 01e1067 into master Jan 23, 2024
15 checks passed

susnux deleted the feat/action-button-active branch January 23, 2024 17:04

ShGKme mentioned this pull request Jan 24, 2024

fix(settings): federation scope menu a11y nextcloud/server#43098

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NcActionButton): Allow pressed state on NcActionButton - similar to NcButton #4744

feat(NcActionButton): Allow pressed state on NcActionButton - similar to NcButton #4744

susnux commented Nov 1, 2023

raimund-schluessler commented Nov 2, 2023

jancborchardt left a comment •

edited

susnux commented Nov 2, 2023 •

edited

susnux commented Nov 2, 2023

jancborchardt commented Nov 2, 2023

susnux commented Nov 2, 2023

raimund-schluessler commented Nov 2, 2023

JuliaKirschenheuter commented Nov 3, 2023

susnux commented Nov 7, 2023

JuliaKirschenheuter commented Nov 7, 2023

susnux commented Nov 7, 2023

JuliaKirschenheuter commented Nov 7, 2023

susnux commented Nov 7, 2023

JuliaKirschenheuter commented Nov 7, 2023

JuliaKirschenheuter left a comment

susnux commented Nov 8, 2023

ShGKme commented Nov 9, 2023

ShGKme commented Jan 10, 2024 •

edited

susnux commented Jan 19, 2024 •

edited

ShGKme left a comment

susnux commented Jan 23, 2024

ShGKme Jan 23, 2024

susnux Jan 23, 2024

ShGKme commented Jan 23, 2024

feat(NcActionButton): Allow pressed state on NcActionButton - similar to NcButton #4744

feat(NcActionButton): Allow pressed state on NcActionButton - similar to NcButton #4744

Conversation

susnux commented Nov 1, 2023

☑️ Resolves

🖼️ Screenshots

🏁 Checklist

raimund-schluessler commented Nov 2, 2023

jancborchardt left a comment • edited

Choose a reason for hiding this comment

susnux commented Nov 2, 2023 • edited

susnux commented Nov 2, 2023

jancborchardt commented Nov 2, 2023

susnux commented Nov 2, 2023

raimund-schluessler commented Nov 2, 2023

JuliaKirschenheuter commented Nov 3, 2023

susnux commented Nov 7, 2023

JuliaKirschenheuter commented Nov 7, 2023

susnux commented Nov 7, 2023

JuliaKirschenheuter commented Nov 7, 2023

susnux commented Nov 7, 2023

JuliaKirschenheuter commented Nov 7, 2023

JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

susnux commented Nov 8, 2023

ShGKme commented Nov 9, 2023

ShGKme commented Jan 10, 2024 • edited

Current usage

Proposal

Alternative solution

susnux commented Jan 19, 2024 • edited

ShGKme left a comment

Choose a reason for hiding this comment

susnux commented Jan 23, 2024

ShGKme Jan 23, 2024

Choose a reason for hiding this comment

susnux Jan 23, 2024

Choose a reason for hiding this comment

ShGKme commented Jan 23, 2024

jancborchardt left a comment •

edited

susnux commented Nov 2, 2023 •

edited

ShGKme commented Jan 10, 2024 •

edited

susnux commented Jan 19, 2024 •

edited