Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: accessibility improvements #992

Conversation

eduardocorreas
Copy link

Foram realizados ajustes de acessibilidade para correção de erros e violações de diretrizes da WCAG.
Entre os ajustes realizados foram corrigidas violações de nível crítico como ausência de region, contraste de cor e descrição de labels em inputs.

Educardo Corrêa added 4 commits November 27, 2022 08:59
…de region no layout do site

A página precisa prover uma estrutura segmentada por regiões. Para isso o HTML se utiliza de tags
como main, nav, header, footer, aside, etc. Esta segmentação auxilia usuários que necessitam de
leitor de tela a navegar diretamente na área que desejam e entenderem a estrutura do site
É comum termos imagens ou ícones de Logo em site que, ao serem clicados nos redirecionam para uma
página específica seja interna ou externa. Para links que contenham apenas estes elementos e possuam
ausencia de texto complementar, é necessário inserirmos o title para ser lido e informado por
leitores de tela
…dição de aria-describedby

Ajustado o contraste de cor do botão de publicação para satisfazer as categorias AA e AAA da WCAG.
Foi realizado também a inserção de atributo aria-describedby para labels e inputs
…raste e aria-describedby

Ajustado o contraste de cor do botão de submit para estar de acordo com as categorias AA e AAA da
WCAG. Foi  adicionatdo tambem o atributo aria-describedby nos inputs para correção de erro de
acessibilidade
@vercel
Copy link

vercel bot commented Nov 27, 2022

Someone is attempting to deploy a commit to the TabNews Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Collaborator

@aprendendofelipe aprendendofelipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oi @eduardocorreas, muito importante essa contribuição!

O TabNews utiliza o Primer. Eles ainda não tem soluções para todos os problemas de acessibilidade que você encontrou, pois muita coisa ainda está em alpha, mas mesmo assim existem algumas maneiras mais compatíveis com a evolução dos componentes deles.

A solução proposta é sustentável com a evolução do Primer, ou seja, buscou na documentação deles sobre esses problemas que você corrigiu?

Pensando na manutenção do sistema, o ideal é priorizar o que eles oferecem.

Como exemplo do que consta na documentação, para mudar a tag html de um componente, basta passar a propriedade as para ele:

<Header as='header'>

Outro exemplo são as cores, onde devemos priorizar o que existe no tema deles. Isso vai facilitar mudanças já planejadas, como a adição do tema dark.

Compatibilizar o máximo possível com o Primer irá facilitar a revisão desse PR 🤝

Exclusão de estilização de cor inline para uso da cor padrão do Prime. Remoção de atributos nativos
HTML (header, main e nav) para utilização do atributo as do Prime
@eduardocorreas
Copy link
Author

@aprendendofelipe Muito obrigado por seu comentário, de fato não havia me atentado a parte da facilidade de manutenção e alinhamento com features futuras. Acabei de subir o ajuste visando os pontos que você mencionou.

@vercel
Copy link

vercel bot commented Nov 27, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
tabnews ✅ Ready (Inspect) Visit Preview Nov 27, 2022 at 5:56PM (UTC)

@aprendendofelipe
Copy link
Collaborator

@eduardocorreas, por que adicionou id nas labels se não está utilizando eles?

@eduardocorreas
Copy link
Author

eduardocorreas commented Nov 27, 2022

id

Inseri id para referenciar no atributo aria-describedby no input.
O aria-describedby estava sendo referenciado como um erro por estar sendo setado com valor vazio.
Por isso, atribui o id e o referenciei no input.

Documentação para este atributo:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-describedby

@@ -84,7 +85,7 @@ export default function ContentList({ contentList, pagination, paginationBasePat
{itemCount}.
</Text>
</Box>,
<Box as="article" key={contentObject.id} sx={{ overflow: 'auto' }}>
<Box as="article" key={contentObject.id} sx={{ overflow: 'auto' }} role={'listitem'}>
Copy link

@hitmain13 hitmain13 Nov 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

role="listitem"

@@ -18,7 +18,8 @@ export default function Footer(props) {
justifyContent: 'center',
flexWrap: 'wrap-reverse',
gap: 3,
}}>
}}
as={'nav'}>
Copy link

@hitmain13 hitmain13 Nov 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as="nav"

@hitmain13
Copy link

hitmain13 commented Nov 30, 2022

Opa, como vai @eduardocorreas?

Muito boa iniciativa abordar sobre acessibilidade, parabéns!

Vale como observação: a premissa de um ID é que elas sejam unicas. No entanto, acredito que seja interessante considerarmos que as IDs passadas nos elementos há possibilidade de se repetirem.

Como sugestão, poderá atribuir prefix dos IDs com base às páginas. Ex.: id="login__password-label", "signup__email-label"

@eduardocorreas
Copy link
Author

eduardocorreas commented Nov 30, 2022

@hitmain13 , tudo bem por aqui e com você?
Muito obrigado por sua contribuição e review do PR.
Acabei de subir estes ajustes que você apontou, poderia validá-los?

@eduardocorreas eduardocorreas requested review from hitmain13 and removed request for aprendendofelipe November 30, 2022 11:41
@@ -131,7 +132,7 @@ function SignUpForm() {
)}
</FormControl>
<FormControl id="email">
<FormControl.Label>Email</FormControl.Label>
<FormControl.Label id="signup__label-email">Email</FormControl.Label>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dando uma olhada na documentação do MDN, que tal usarmos o atributo for na label ao invés de id e aria-describedby para vincular a label com o input?

No site a11y-101 tem um caso de uso melhor para o atributo aria-describedby.

Não sou especialista nesses pontos de acessibilidade, então se eu estiver enganado, me avise 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E aí, @Rafatcb ! Tudo bem? Concordo com o que você disse sobre a troca.
No caso, eu havia atribuído o aria-describedby via ID, pois verifiquei que este atributo está sendo gerado automaticamente com valor vazio. (Fiz essa inspeção via plugin WAVE). Ao invés de deixá-lo vazio, atribuí a ele o valor da label.

Entretanto você trouxe um ponto bem pertinente que, de fato, não é este o objetivo dele. Então em relação a vínculo, farei a troca para o FOR como você sugeriu. Obrigado.

@@ -108,7 +108,7 @@ function RecoverPasswordForm() {
{globalErrorMessage && <Flash variant="danger">{globalErrorMessage}</Flash>}

<FormControl id="password">
<FormControl.Label>Senha</FormControl.Label>
<FormControl.Label id="recover-token__label-password">Senha</FormControl.Label>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Que tal criar um componente wrapper para o TextInput e o Label para não ter o risco de criar um input sem label vinculado, e também evitar a repetição de código? Eu nunca usei o Primer, então não tenho certeza se faz sentido ter o FormControl.Label dentro do wrapper. Talvez levar todo o FormControl para o componente seja mais interessante.

De qualquer forma, recomendo esperar mais algum comentário antes de implementar o que eu sugeri aqui, caso concorde. Talvez o @aprendendofelipe possa trazer a opinião dele quando estiver com tempo disponível 👍

Fiz uma sugestão parecida no PR #926, porém mais simples e com outra intenção, sem colocar o FormControl.Label no wrapper. Se essa sugestão fizer sentido aqui, o outro PR será afetado também pois usará o componente criado aqui.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Faz sentido para mim sua proposta. O que você acha @aprendendofelipe ?

@Rafatcb
Copy link
Collaborator

Rafatcb commented Nov 30, 2022

Opa, como vai @eduardocorreas?

Muito boa iniciativa abordar sobre acessibilidade, parabéns!

Vale como observação: a premissa de um ID é que elas sejam unicas. No entanto, acredito que seja interessante considerarmos que as IDs passadas nos elementos há possibilidade de se repetirem.

Como sugestão, poderá atribuir prefix dos IDs com base às páginas. Ex.: id="login__password-label", "signup__email-label"

O id precisa ser único no documento (nesse caso, a página), não tem necessidade de ser tão específico assim. Caso tenham dois campos de senha, por exemplo, aí faz sentido um ter o id="password" e o outro id="password-confirm", por exemplo, mas aí já é o mesmo cuidado a ser tomado com os labels, que não serão iguais para não confundir o usuário.

PS: Ótimo PR, como já mencionado, já que a acessibilidade é crucial para o projeto 🤝

variant="primary"
type="submit"
disabled={isPosting}
aria-label="Publicar"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Não precisamos de um aria-label aqui, já que o botão tem um texto descritivo.

Referências: a11y-101, MDN

@@ -21,7 +21,8 @@ export default function ContentList({ contentList, pagination, paginationBasePat
display: 'grid',
gap: '0.5rem',
gridTemplateColumns: 'auto 1fr',
}}>
}}
role={'list'}>
Copy link
Collaborator

@Rafatcb Rafatcb Nov 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conforme comentários do hitmain13:

Suggested change
role={'list'}>
role="list">

@@ -28,7 +29,7 @@ export default function Footer(props) {
color: 'fg.subtle',
}}>
<Link href="/" sx={{ color: 'fg.subtle' }}>
<CgTab size={26} />
<CgTab size={26} title="Acessar a página inicial" />
Copy link
Collaborator

@Rafatcb Rafatcb Nov 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 O title não parece ser amigável para tecnologias assistivas. Eu não sabia disso, descobri na documentação do MDN. Vendo o link de referência, algumas sugestões são dadas para melhorar isso. De forma geral, acho que temos três alternativas:

  1. Colocar texto ao lado do ícone;
  2. Usar uma Tooltip, como nas TabCoins/TabCash;
  3. Usar o atributo aria-label.

A 3 me parece fazer mais sentido, mas não posso afirmar isso com certeza.

Edit: no caso da 3, pensei em ficar com ambos atributos: o title e o aria-label.

@Rafatcb Rafatcb added the pendente Aguardando ação do autor do Pull Request label Dec 16, 2023
@Rafatcb
Copy link
Collaborator

Rafatcb commented Dec 23, 2023

Eduardo, obrigado por trazer essas sugestões aqui! Por causa do seu PR eu decidi ir mais a fundo e resolvi os problemas que ficaram pendentes dos comentários, além de ter descoberto mais coisas. Compartilhei tudo no PR que abri: #1580.

Então, estou fechando este aqui.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acessibilidade Melhoria de acessibilidade pendente Aguardando ação do autor do Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants