demtario / hes-gallery

Light, dependency free, responsive gallery script
https://demtario.github.io/hes-gallery/demo/
MIT License
30 stars 2 forks source link

fix(accessibility): solve WAVE problems #11

Closed Kristinita closed 4 years ago

Kristinita commented 4 years ago

1. Summary

I fixed hes-gallery errors, that shows web accessibility tools.

2. MCVE

2.1. Data

We can check hes-gallery demo page online use WAVE — web accessibility evaluation tool.

Report.

2.2. Behavior

2.2.1. Before

Behavior before

2.2.2. After

Behavior after

No “Missing alternative text” and “Empty button” errors.

I didn’t find any regressions after my changes.

3. Changes

3.1. Alternative text

I added alt attribute with value Default alternative text to img#hg-pic.

3.2. Buttons

I replaced <button> tag to <a>. Possibly, <button> isn’t the best solution if it isn’t used for submissions in forms. See “When To Use The Button Element” article.

4. Environment

4.1. Minification

I minified hes-gallery.js and hes-gallery.css on these online services:

  1. UglifyES 3.3.9
  2. CSSO 4.1.0

4.2. Testing

  1. Windows 10.0.19041.508 Pro N for Workstations 64-bit EN
  2. Browsers:

    1. Firefox 82.0.1 (64-bit)
    2. Chromium 85.0.4183.102 (Official Build) (64-bit)

Thanks.

demtario commented 4 years ago

Hi! First of all, thanks for your contribution!

But, I'm a bit concerned about changing <button> into <a> tag. <a> tag without href attribute is considered as a placeholder, and I think shouldn't be a replacement for a <button>, which is allowed to be used outside the forms as an action button.

I agree, that empty buttons are a bad idea, but I think, this can be solved by moving icon from CSS directly into button (in html structure), and by adding aria-label and title tag with "Next" or "Previous" text in them [source]. Current solution does probably remove error, but does not solve accessibility issue.

So to summarize, you can change:

<button id='hg-prev'></button>
<button id='hg-next'></button>

into:

<button id="hg-prev" title="Previous" aria-label="Next"> <img src="iconsource" /> </button>
<button id="hg-next" title="Next" aria-label="Previous"> <img src="iconsource" /> </button>

Again, thanks for your contribution!

Kristinita commented 4 years ago

Type: Objection 💬

1. Summary

Possibly, in our case div, not button or a is the most semantic way. What do you think about it?

2. Argumentation

<a> vs <button> vs <div> vs <input type='button"> question.

Answers:

Thanks.

demtario commented 4 years ago

I would rather stay with <button> option. Element about which we discuss is an arrow to change image in the gallery, so it is a clickable one. In my opinion div is just a container (shouldn't hold any action), and in this case button should do this.

Kristinita commented 4 years ago

@demtario ,

1. Remark on a secondary argument

can also be focused by keyboard, which is not simply possible for divs

Possibly, tabindex attribute should help in solving this problem.

2. Pull request status

According to the arguments presented above I think that the button element is not the best idea in this case. I will not add it.

But hes-gallery is your plugin, you can use whatever elements you want.

Thanks.

demtario commented 4 years ago

@Kristinita Yea, I know that this could be done by tabindex, but this is not a solution. There is no point of using <div> if we can use <button>.

As you said in your arguments:

<button> tag: according with W3, this is the standard tag to create buttons on the page, so use it when you need an action like onClick().

and in

<div> | Can be used for clickable area which is not button or link by definition

Moving to next/prev button is not a area, it is a button by definition

Kristinita commented 4 years ago

@demtario , OK, we are waiting for it to be implemented).

Thanks.

demtario commented 4 years ago

Ok, so I'm closing this PR and moving our conclusions to the new issue: https://github.com/demtario/hes-gallery/issues/13