Feature/accessibility

[Zankel-Engineering]

What is it?

• [ x] Feature / enhancement
• Bug
• Docs / tests

Description

Adding accessibility-feature for screen readers to the popover trigger. Can be extended to other components and is not bound from typing where what is possible yet.

Use cases and why

• 1. New accessibility law in the EU
• 2. People with disabilities worldwide

Screenshots/Demo

See in the examples

Checklist:

• My code follows the developer guidelines of this project
• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• Added new tests to cover the fix / functionality

[github-actions]

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

---

I have read the CLA Document and I hereby sign the CLA

---

1 out of 2 committers have signed the CLA.
✅ @shairez
❌ @Zankel-Engineering
_{You can retrigger this bot by commenting recheck in this Pull Request}

[shairez]

Thanks @Zankel-Engineering for spending time on this PR.

First of all, headless component must apply all the correct ARIA roles and attributes in order to be considered READY for production. so our goal is to save the developer the need to explicitly mark any of the components with ARIA labels because it should be done for them by the library (and if not, it's probably a bug on our side)

Second, I've reviewed it and I think there's an eaiser way to handle this -

Qwik has a type called QwikIntrinsicElements which you can use by passing the specific component like QwikIntrinsicElements['button'] to get all the type safety for props.

Plus in Qwik, unlike React, we can use aria-label instead of ariaLabel so no need for the converting utils for these props.

What we can do is to either destructure the props from all the specific event listeners we don't want to override like onMouseOver$ or to spread them BEFORE the intended implementation.

I've written an example of a mix of both here:

import {
  $,
  component$,
  Slot,
  useVisibleTask$,
  useContext,
  useSignal,
  useStylesScoped$,
} from '@builder.io/qwik';
import { PopoverContext } from './popover-context';
import styles from './popover-trigger.css?inline';
import { QwikIntrinsicElements } from '@builder.io/qwik';

export type PopoverProps = QwikIntrinsicElements['span'];

export const PopoverTrigger = component$(({onMouseOver$, ...props}: PopoverProps) => {
  const ref = useSignal<HTMLElement>();
  const contextService = useContext(PopoverContext);
  useStylesScoped$(styles);

  useVisibleTask$(() => {
    contextService.setTriggerRef$(ref);
  });

  const mouseOverHandler = $(() => {
    contextService.isOpen = true;
  });

  return (
    <span
      ref={ref}
      {...props}
      class={`popover-trigger${props.class ? ` ${props.class}` : ''}`}
      role="button"
      onMouseOver$={
        contextService.triggerEvent === 'mouseOver'
          ? mouseOverHandler
          : undefined
      }
    >
      <Slot />
    </span>
  );
});

We need to make sure that each component has this behavior by default (some of the componets like tabs and accordion has this already)

If you can, please update your PR and also make sure you fill out the CLA following these insturctions to make the bot happy :)

Let me know if you need any assitance

Thanks again for your contribution!

[shairez]

Thanks @Zankel-Engineering !

About the CLA, if you manually udpated the file, you need to undo it and instead reply the following words here (so it'll do it automatically) -

Thanks

[shairez]

What are the benefits of using ExtendedPropsByAriaAttribute compared to QwikIntrinsicElements which alredy has aria attributes built in?

[shairez]

That's good thinking to cover the case, but I think it's more relevant to react code.
Wrote in my other answer why.

[shairez]

I'm not sure why do we need an extra store, we can spread the props over the element itself instead (like we do in the tabs or accordion) which will remove the unnecessary management code.

[shairez]

but in Qwik it's not an issue, people are not suppose to write their attributes with snake case or camel etc, only kebab, just like standard HTML

[shairez]

Thanks again @Zankel-Engineering !

I've added a few comments to illustrate better what I meant in my last reply, I think we can leverage the already existing aria attributes handling that Qwik has instead of duplicating it.

Let me know what you think
Thanks!

[shairez]

@all-contributors please add @Zankel-Engineering for code, tests, bug, and a11y

[allcontributors]

@shairez

I've put up a pull request to add @Zankel-Engineering! 🎉