Add visually-hidden class

[JeffersonBledsoe]

This PR adds a visually-hidden class which is useful in situations where you want to provide additional information to Assistive Technology and an aria-label may not be appropriate.

Questions

• Should this be 'CMS' only? Or should we provide this utility class for users of the Volto view-mode to rely on too.

Todo

• Add to @plone/components
• Documentation!

[netlify]

✅ Deploy Preview for plone-components canceled.

Name	Link
🔨 Latest commit	24fbf83
🔍 Latest deploy log	https://app.netlify.com/sites/plone-components/deploys/6749a906d3d67500084bd47b

[sneridagh]

@JeffersonBledsoe @ichim-david if we do this, we need that also in @plone/components (for now) to have it also in the future theming.

[sneridagh]

@JeffersonBledsoe it seems you pushed more changes than the needed in this PR. Can you take a look, please? you deleted things and files, that we cannot remove.

[ichim-david]

@sneridagh @JeffersonBledsoe I've taken a deeper look and it seems that bootstrap 5 renamed sr-only to visually-hidden.
barceloneta is now based on bootstrap 5 as I see
https://getbootstrap.com/docs/5.3/migration/#helpers-2
https://github.com/plone/plonetheme.barceloneta/tree/master/scss
If we stick to the visually-hidden class, we have the same class name in both systems.

This is such a standard nowadays that I doubt that it could lead to class conflicts on account of modifying such class.
As such I don't think we need to rename it to sr-only unless @sneridagh you have another opinion.

[Wagner3UB]

@plone/volto-accessibility I'll try to run some tests within our theme to see if I can find any conflicts.

[ichim-david]

I think this is fine from my point of view, waiting also for @Wagner3UB review if there would be any conflict with the bootstrap theme

[Wagner3UB]

All good here for me.

[Wagner3UB]

@JeffersonBledsoe @ichim-david Can we merge this one?

[JeffersonBledsoe]

@Wagner3UB I still need to add this to @plone/components. Should get that done this week

[github-actions]

Hi There! 👋

We haven't seen any activity on this pull request in a while 😴, and we want to make sure that it's still relevant. Please let us know by:

• adding a comment about what needs to be done next 💬
• updating its status and other labels 🏷️

Otherwise close this pull request. 🧹

[giuliaghisini]

@JeffersonBledsoe @ichim-david what about this? Is it ready to merge? we need this fixes..

[wesleybl]

I think it requires a small note in the upgrade guide though, it can be seen as a breaking change.

@pnicolli @Wagner3UB I find it strange that this is considered a breaking change. Is this because the class name visually-hidden is common in CSS frameworks? Can't we have a name like visually-hidden-volto? That way we would avoid name conflicts. Otherwise, how can we have this functionality in Volto 18?

[nileshgulia1]

I think it requires a small note in the upgrade guide though, it can be seen as a breaking change.

@pnicolli @Wagner3UB I find it strange that this is considered a breaking change. Is this because the class name visually-hidden is common in CSS frameworks? Can't we have a name like visually-hidden-volto? That way we would avoid name conflicts. Otherwise, how can we have this functionality in Volto 18?

I guess yes, this a11y change would silently changes behavior of any custom class with the same name. I see it as a behaviour change not a developer error.

[nileshgulia1]

Maybe we can add some code examples in the upgrade guide to show the exact class name. Otherwise lgtm as well.

[Wagner3UB]

I think it requires a small note in the upgrade guide though, it can be seen as a breaking change.

@pnicolli @Wagner3UB I find it strange that this is considered a breaking change. Is this because the class name visually-hidden is common in CSS frameworks? Can't we have a name like visually-hidden-volto? That way we would avoid name conflicts. Otherwise, how can we have this functionality in Volto 18?

@wesleybl @JeffersonBledsoe @pnicolli This can be discussed, but it creates room for duplicate classes to remain in the code. I’m not sure if that would be acceptable. If it is, we can certainly create a specific class to reference this kind of behavior. Keep in mind that there are other older classes from different libraries that might have been included, such as sr-only, an old class from Bootstrap 4.

[Wagner3UB]

Maybe we can add some code examples in the upgrade guide to show the exact class name. Otherwise lgtm as well.

@nileshgulia1 Sure, add info is never a problem!

[pnicolli]

I find it strange that this is considered a breaking change. Is this because the class name visually-hidden is common in CSS frameworks?

@wesleybl I personally usually consider anything that could change the end user website appearance as a breaking change. Adding a global css class with a name that is commonly already used elsewhere, even if it usually has the same goal, can fall into these breaking change category. Again, this is just my personal opinion, and I understand it might look a little too strict. @sneridagh you got any opinion about this matter?

Can't we have a name like visually-hidden-volto? That way we would avoid name conflicts. Otherwise, how can we have this functionality in Volto 18?

I am not sure, Plone used to have these "branded" classes in Plone 5 and I found them confusing eventually. I would not be 100% against it but hoestly I would just put the class in your custom projects when you need it if those are Volto 18 still, it's a super quick addition.

[sneridagh]

@pnicolli @wesleybl luckily we have this written down:

https://6.docs.plone.org/volto/release-management-notes/index.html#dom-structure-and-default-styling

Yes, it is a breaking change and should be documented as one properly. Also, this makes it non-backportable to 18. As @pnicolli said, a quick addition to any 18 project.

Regarding the @plone/components, yes it should be backported, since the "canonical" version that we use is in the seven branch.

[Wagner3UB]

@JeffersonBledsoe I think we are done here! Only thing to do is the backport to 18.x.x

@pnicolli @stevepiercy We have your blessing?

[pnicolli]

I'm so sorry to bring up more problems, but I realized that we are adding this to @plone/components but actually react-aria has its own way of doing this: https://react-spectrum.adobe.com/react-aria/VisuallyHidden.html
We should probable then avoid adding this to @plone/components, or is there a reason that I don't see at the moment? @sneridagh

Also since this is a breaking change, we cannot backport it to Volto 18.

[wesleybl]

but hoestly I would just put the class in your custom projects when you need it if those are Volto 18 still, it's a super quick addition.

@pnicolli @sneridagh @Wagner3UB What I'm trying to say is this: We have an accessibility issue in both Volto 19 and Volto 18. If the fix in Volto 19 is a breaking change, fine. But we need to find a way to make it work in Volto 18 as well. Volto 18 is the current version of Volto. We need to fix accessibility issues in it. This is a bug. We can't just say "fix it in your project."

[pnicolli]

What I'm trying to say is this: We have an accessibility issue in both Volto 19 and Volto 18. If the fix in Volto 19 is a breaking change, fine. But we need to find a way to make it work in Volto 18 as well. Volto 18 is the current version of Volto. We need to fix accessibility issues in it. This is a bug. We can't just say "fix it in your project."

Just want to make sure here that I explain my thoughts in a clear way: we are only talking about adding the visually-hidden class here in this PR, not fixing any bug. Adding this class as a global class is the breaking change, and I am willing to have this breaking change because I find this useful for the future and for some of the currently open bugs that we have.

If you are thinking about the bugfix in #7500 then yes, I agree we should have a fix for that in Volto 18 as well. That PR is out of scope here, even if it is what triggered the discussion on this PR, and thank you again for bringing this up over there.

We should try to stick to our policy about breaking changes at all times unless really unavoidable. In the specific case of #7500 we should finish and merge this PR and then use the class that we are adding here to fix that issue in Volto 19. Then we should fix the issue in Volto 18, absolutely, but I would simply change the fix to make it non-breaking. For example, for Volto 18, we should not use this class and instead apply these styles directly to the affected item. I would say let's continue the discussion about that other PR in its specific thread and not here, otherwise we risk mixing things up too much. Am i making sense here? Always happy to discuss!

[sneridagh]

@pnicolli you are right, let's remove the addition to @plone/components for now. I didn't realise of this fact.
Could someone take care of it?

[Wagner3UB]

@pnicolli Ok, completely understood. Thank you.

[Wagner3UB]

@pnicolli @sneridagh Done, I have removed the plone/components part. Now we are good to go.