A11y Addons control panel #6345

deodorhunter · 2024-09-27T11:03:48Z

This PR overhauls current Addons Control panel UI using react-aria-components and solves #5142.
All accordions removed, UI is now akin to Plone Classic.
Aria pattern used: ARIA grid pattern through GridList component

Changelog:

Written in Typescript leveraging @plone/types
Component UI overhauled and code splitted, really minor changes to existing logic (types mostly)
Updated tests and added new tests and related mocks/fixtures/snapshots for Cypress and Jest/Testing Library.
Updated some Addons related types in @plone/types package
Added stylesheet and imported it directly in the component, css splitting and lazy loading works as expected. Will move styles in less theme files if deemed better/necessary
Translations

UI Preview

netlify · 2024-09-27T11:04:11Z

✅ Deploy Preview for plone-components canceled.

Name	Link
🔨 Latest commit	`7cdd967`
🔍 Latest deploy log	https://app.netlify.com/sites/plone-components/deploys/67223d4e3d6b7a000816b829

ichim-david · 2024-09-29T10:05:22Z

packages/volto/src/components/manage/Controlpanels/Addons/index.css

+  padding: 2rem 1.5rem;
+}
+
+#page-addons .btn-primary {


I am not a fan of this styling of button variants where we introduce css overrides.
Since we are starting over and making use of css vars we should have a button class
and variant classes that where possible just modifies the vars.
Have a look at this article where this designer shows how they made their button
component.
https://piccalil.li/blog/how-i-build-a-button-component/#variants

I feel like we still need to adopt a styling methodology and try to use it in the new
components if we are now at the totally breaking future phase.
Will have another look at the plone.contents efforts and plone.components and
come back with further observations for the future.

@ichim-david it's out of the scope of this PR to go in the direction of the new theming system. Since we don't know how that will be yet, it sounds like a waste of time for me to make it up in this specific PR. Moreover, Victor and I specifically told everybody working on reimplementing these panels and other views to avoid adding plone/components and such because that's not coming in volto 18 while these fixes should be there if possible.

@ichim-david I understand your concerns, and this kind of "custom temporary styling" is not my preferred style either.
I would have gone with a cva/cva like solution (https://cva.style/docs) if we had a theming convention (at least on paper), but alas that's still a long way in the future from what I understand.
As @pnicolli said, we had a talk about this during the sprint, and decided to avoid using plone/components and unified theming like quanta styles.

After some thinking, a possible edge case issue with my initial implementation of selector naming came to mind: people using custom cms-ui styles, like injecting bootstrap or a custom stylesheet with the same selector naming convention. It would have caused inconsistencies and hassles, so I changed the button selector names to reflect the scope of action of that button (install/install upgrade and uninstall).

deodorhunter · 2024-09-30T12:46:46Z

Update
This pr is now complete with all missing translations in all supported languages. Got some help from our well known GPT buddy, as I would have hated leaving blanks for other languages.
Haven't seen strange/unusual translations during a quick cross check with Translate and Deepl, but you never know...

ichim-david · 2024-10-29T12:03:43Z

@pnicolli @sneridagh I've looked at the bundle implication of loading react-aria-components.
It seems that it's not bundled in the default bundle and only used on the addons

I wonder it react-aria doesn't have better imports to avoid loading the entire library if you only use a couple of components.
Given the fact that we don't load it in the default bundle I'm not against this although I find it a bit strange that we have a dependency on react-aria-components within plone.components and now we load it also in volto core directly.
Whatever you guys decide I'm fine given that we don't fatten the default bundle :).

pnicolli · 2024-10-29T12:18:07Z

@ichim-david yeah Victor and I discussed this briefly with the people doing stuff with react-aria-components during Salamina Sprint and we said that we wouldn't use plone/components here because it's still alpha and kind of wip officially, but we can use react-aria-components for now, since moving from this to plone/components should remain easy enough.
Regarding a more precise import, we will need to look into it while working on plone/components so I think it's safe to say it's not a big problemi for now.

ichim-david

I see the following a11y issues:

Verify the warning given by IBM Equal Access Accessibility Checker plugin
I don't like that the screenreader says just listbox and you can only navigate with the arrows. Here it's strange if you use up and down arrow it will jump to the next listbox heading. If you use left or right keys it will read the heading, the install or uninstall button and the description such as "Installs the collective.volto.stickyblocks add-on." When you think of a listbox you think of up and down only so I don't know how many would think to use left or right to get to the button. Luckily it shows up in the form controls rotary so it could be worse.
The headings could use a bit of aria-label for instance what is available, if you just navigate through the headings you might not know what available means.

deodorhunter · 2024-10-29T14:12:38Z

Thanks for your review, I'll try to look at it asap!

I see the following a11y issues:

Verify the warning given by IBM Equal Access Accessibility Checker plugin

I run checks with Accessible Web and Axe and no warning were shown, I'll try this addon too. For future reference, is this what the community usually uses as browser addon for a11y testing? I might have missed this piece of information, sorry if that's the case.

I don't like that the screenreader says just listbox and you can only navigate with the arrows. Here it's strange if you use up and down arrow it will jump to the next listbox heading. If you use left or right keys it will read the heading, the install or uninstall button and the description such as "Installs the collective.volto.stickyblocks add-on." When you think of a listbox you think of up and down only so I don't know how many would think to use left or right to get to the button. Luckily it shows up in the form controls rotary so it could be worse.

I admit this is not the output I get/remember with VoiceOver.
On Chrome I get all the info as a screen reader user

I'll be checking Safari next, then Firefox.

AFAIK Firefox, is... a bit of a special child and at 2% usage nowadays for many reasons.
If it ends up as a Firefox specific issue, we will have to make a decision about that, given such a low and declining worldwide usage ratio (Edge is at 5%, and that's food for thought for Mozilla, given that usually for Windows non business users, Edge is used once only to download Chrome on a fresh Windows installation 😄 ).
Additionally, if it stems from react-aria-components or it's an os+browser combo accessibility bug, I cannot do much here, but I'll take a look for sure.

The headings could use a bit of aria-label for instance what is available, if you just navigate through the headings you might not know what available means.

On it

stevepiercy · 2024-10-29T20:32:07Z

I run checks with Accessible Web and Axe and no warning were shown, I'll try this addon too. For future reference, is this what the community usually uses as browser addon for a11y testing? I might have missed this piece of information, sorry if that's the case.

We don't document the tools we use, but we sure ought to.

https://6.docs.plone.org/volto/contributing/accessibility-guidelines.html

I'll update the open PR with the a11y tools that contributors should use (currently only cypress-axe is included), but please help me out by providing a list of their names (I've started that below), what you use them for, and from where to download or install them. Thank you!

cypress-axe. Automate accessibility testing with axe-core in Cypress.
IBM Equal Access Accessibility Checker plugin.
Accessible Web.
Voiceover.
moar tools

deodorhunter · 2024-10-30T12:12:53Z

I run checks with Accessible Web and Axe and no warning were shown, I'll try this addon too. For future reference, is this what the community usually uses as browser addon for a11y testing? I might have missed this piece of information, sorry if that's the case.

We don't document the tools we use, but we sure ought to.

https://6.docs.plone.org/volto/contributing/accessibility-guidelines.html

I'll update the open PR with the a11y tools that contributors should use (currently only cypress-axe is included), but please help me out by providing a list of their names (I've started that below), what you use them for, and from where to download or install them. Thank you!

cypress-axe. Automate accessibility testing with axe-core in Cypress.

IBM Equal Access Accessibility Checker plugin.

Accessible Web.

Voiceover.

moar tools

Sure thing, a couple that come to mind and usually use:

Axe devTools
voiceover kb shortcuts because I never remember these
A11Y-Color-Blindness-Empathy-Test
WAI-ARIA - Screen reader compatibility: Shows how different WAI-ARIA attributes behave in commonly used screen readers. The results include two types of test:

Expected to work - these tests show support when accessibility features are used correctly
Expected to fail - these tests show what happens when accessibility features are used incorrectly

The last resource helped me in coming to a conclusion on the differences between Chrome and Firefox and Safari with VoiceOver we were talking about yesterday.
Long story short: it is browser, OS AND Screen Reader dependant. Firefox/Safari + VoiceOver fails with aria-describedby, works if we use aria-label instead.
Which means that as it stands, having only a Mac to work on, I cannot guarantee the result on NVDA/Jaws as they are Windows only platforms. It should work (even better than on Mac looking at stats), but I simply don't know.

On the brighter side, I did a deep dive in rac docs and internals again, and came up with a solution using a specific prop that under the hood displays the passed value as aria-label (I can now guess why).
Contextually, I added another feature: Enter now executes the selected/focused GridListItem action, and updated assistive labels. This is to address your previous point

When you think of a listbox you think of up and down only so I don't know how many would think to use left or right to get to the button. Luckily it shows up in the form controls rotary so it could be worse.

The only downside is that dbl click on GridListItem now executes the same action, I cannot do anything about it due to it being a feature of rac. For some power users it could even be a feature, but we can surely think about that more thoroughly.
I updated the code with these changes, let me know what you think about it

ichim-david · 2024-10-30T12:33:20Z

packages/volto/src/components/manage/Controlpanels/Addons/AddonItem.tsx

@@ -83,7 +112,8 @@ const AvailableItem: React.FC<AvailableAddonProps> = ({ addon, onInstall }) => {
    <GridListItem
      key={addon['@id']}
      className="addon-item"
-      aria-describedby={`addon-desc-${addon.id}`}
+      textValue={`${addon.title} ${addon.description}. ${intl.formatMessage({ id: 'Press Enter to install this addon' })}`}


@deodorhunter you add the period which shows up strange if you don't have an addon. See the test result here 416059d#diff-023c7e608404538c6858332e1915002b4c248049367176e7d0a52f69097794a2R65

I don't get why a test on a slate block widget inside a page should be broken by a period in a controlpanel component aria-label which is not even rendered during said test, but if I remove the period, the test passes...
Oh well, fix incoming.
I saw a coresandbox test breaking randomly too, also on blocks, line 125 of blocks.js, is it cypress hooks and locators/selectors failing or is the test outdated/flaky/wrong?
In cy preview, I'm not seeing anything similar to what the selector is looking for

What is going on here?
I admit my Cypress skills are rusty, I've been using Playwright lately.

…etadata.js somehow

pnicolli · 2024-10-31T10:10:27Z

@ichim-david Not sure I understand the status today. Are the review comments still open or are they already settled? Are there any blockers among those that are still open? Just trying to figure out if this will make it into v18 or not and how to plan for the future.

ichim-david · 2024-10-31T10:35:57Z

@ichim-david Not sure I understand the status today. Are the review comments still open or are they already settled? Are there any blockers among those that are still open? Just trying to figure out if this will make it into v18 or not and how to plan for the future.

I have a few more comments that I will add soon. This is an improvement over the previous add-ons control panel so I am not against having it for Volto 18. Overall it's just minor issues left that can be handled. @sneridagh should also give his input on this work.

ichim-david

@deodorhunter @pnicolli there is no Upgradable, it's Upgradeable. Please rename everywhere in order for it to sound proper when using a screenreader. This also means every import and file name that has Upgradable.

Also please use add-ons instead of addons as it will be read again better by screen readers

ichim-david · 2024-10-31T10:48:30Z

@deodorhunter @pnicolli there is no Upgradable, it's Upgradeable. Please rename everywhere in order for it to sound proper when using a screenreader. This also means every import and file name that has Upgradable.

Also please use add-ons instead of addons as it will be read again better by screen readers

@deodorhunter @pnicolli ok I was incorrect to mention that there is no upgradable, there is https://en.wiktionary.org/wiki/upgradable#English. However I use American english and not british english and it sounded better for me with upgradeable. I don't know if this is reason enough to change it though, @stevepiercy do you have anything to add on this?

ichim-david · 2024-10-31T11:06:50Z

@deodorhunter @pnicolli I would also do the following fixes:

align the buttons to the top
add a flex-wrap on the header
add a class to the div that has the install, update or delete button

ichim-david · 2024-10-31T11:54:20Z

@deodorhunter I don't like the behavior of double-clicking on the section which will trigger an install or uninstall. it's not at all obvious and is in fact totally unexpected. I might want to copy some of the text from that section and install I get an install or uninstall

double-click-to-install-or-uninstall.mp4

I see that you mention something about double-clicking performing an action #6345 (comment).
I find this change worse than what we previously had with the left and right arrows. If you can't quickly do something about it I would still disable the double click to perform action.

EDIT:
For that matter, the section isn't a submit button, so I would not allow it to press enter to install but have it install or update or uninstall only with the button action that is clear from the label plus it's semantic which is a button to perform an action.

stevepiercy

In my review the changes in volto.pot are authoritative and should be propagated to the other files.

stevepiercy · 2024-10-31T11:52:57Z

packages/types/news/6345.bugfix

@@ -0,0 +1 @@
+Updated incomplete types for addons service response. @deodorhunter


Suggested change

Updated incomplete types for addons service response. @deodorhunter

Updated incomplete types for add-on service response. @deodorhunter

stevepiercy · 2024-10-31T12:04:50Z