Make the `buttons` accessible. #79

XhmikosR · 2016-03-29T15:14:32Z

Also switch to × instead of a simple X for the fallback close button.

Tested things with https://github.com/squizlabs/HTML_CodeSniffer, but I think we hit squizlabs/HTML_CodeSniffer#160 for the remaining ones.

feimosi · 2016-04-02T11:16:02Z

Just a question, why do you use .setAttribute(); for one attribute and .innerHTML = '<span aria-hidden="true"> for the second one instead of being consistent? And don't you think that we should manipulate aria-hidden depending whether the buttons are shown or not?

XhmikosR · 2016-04-02T11:20:51Z

I just went with the solution that was simpler for me. Do you want to use your create function instead?

As for if we should change the aria attributes, I don't see the need. We only hide the icons. The label is OK I believe.

feimosi · 2016-04-02T11:39:24Z

Yeah, actually I'd use create and implement a new method for adding attributes (the reason behind this is to keep minification more effective).

I'm not really familiar with aria spec, I just thought that aria-hidden should reflect the current visibility state, correct me if I'm wrong.

XhmikosR · 2016-04-02T11:40:23Z

IIRC it's OK. We do the same in Bootstrap.

How about you merge this and refactor it later as you wish?

feimosi · 2016-04-02T12:38:29Z

Following the W3 ARIA spec it should be manipualted dynamically:

When the element is presented, authors MUST set the aria-hidden attribute to false or remove the attribute, indicating that the element is visible.

XhmikosR · 2016-04-02T12:43:37Z

Hmm, not sure how I can keep this simple then.

I'll try to check it out later and let you know.

feimosi · 2016-04-02T12:46:05Z

Ok, thanks.

XhmikosR · 2016-04-02T16:15:58Z

An example of grunt-accessibility in action https://travis-ci.org/mpc-hc/mpc-hc.org/builds/120297353

Now, any hints how to proceed with this? In which function should I handle the addition/removal of aria-hidden?

feimosi · 2016-04-02T21:22:14Z

Here's the line responsible for buttons visibility:
https://github.com/feimosi/baguetteBox.js/blob/master/src/baguetteBox.js#L316
but actually you can set aria-hidden to false by default and switch it to true in this if:
https://github.com/feimosi/baguetteBox.js/blob/master/src/baguetteBox.js#L312

XhmikosR · 2016-04-04T08:17:54Z

@feimosi: so I tried this approach, but I'm having issues with inserting the new element. appendChild or insertBefore don't do what we need.

Check the last patch to see what I've been trying to do.

XhmikosR · 2016-04-04T12:23:04Z

@feimosi: I think it's better now, please check the branch and let me know so that I squash before merging.

feimosi · 2016-04-04T18:28:34Z

@XhmikosR I just don't understand, why you have created new elements [xxx]Img ?

XhmikosR · 2016-04-04T18:30:40Z

You asked me to use create.

feimosi · 2016-04-04T18:35:35Z

Ah, you're right! The name just misguided me. I'm not sure about the Img suffix.Do you have any other ideas? Maybe Content ?

XhmikosR · 2016-04-04T18:53:26Z

You mean previousButtonContent?

feimosi · 2016-04-04T19:03:16Z

Yeah, exactly.

XhmikosR · 2016-04-04T19:06:06Z

Done.

I'm still not sure if we are doing the opposite with aria-hidden. Perhaps we should have it set to true by default and set it to false when the buttons are visible.

feimosi · 2016-04-04T19:13:47Z

src/baguetteBox.js

@@ -316,6 +325,8 @@
            options.animation === 'slideIn' ? '' : 'none');
        // Hide buttons if necessary
        if (options.buttons === 'auto' && ('ontouchstart' in window || imagesMap[currentGallery].length === 1)) {
+            previousButtonContent.setAttribute('aria-hidden', 'true');
+            nextButtonContent.setAttribute('aria-hidden', 'true');
            options.buttons = false;
        }
        // Set buttons style to hide or display them


Actually, I've overlooked the case when a user sets options.buttons to false himself. The aria-hidden logic should better be on line 333. Could you insert there something like:

if (options.buttons === false) { // set aria-hidden to true previousButton.style.display = nextButton.style.display = 'none'; } else { // we may also move setAttribute('aria-hidden', 'false') here // ... .display = ''; }

I agree.

But shouldn't we have aria-hidden=true by default and aria-hidden=false if the buttons are not hidden?

We can set aria-hidden value only in this if {} else {} without any defaults before. What do you think?

Can you check the last patch I pushed here?

BTW, the close button isn't hidden so I left it out from aria-hidden=true.

I think we need to have aria-hidden=true by default. So, we probably need to ditch the last approach.

Okay, you're right. The last patch doesn't look so good. We could move to aria-hidden=true approach, but what about the Close button? It's always visible, so does it make sense to switch aria-hidden for him?

I guess for consistency, yes.

I pushed a 4th patch, can you look and let me know?

feimosi · 2016-04-04T19:15:20Z

Does my latest code note resolve your doubts? Then we'll have 'aria logic' in one place.

feimosi · 2016-04-04T19:49:01Z

Still, I've got some mixed feelings about keeping it to true by default. That creates unnecessary code bloat for a such a small lib.
Let's start from the beginning. If we kept ('aria-hidden', 'false') by default for every button, then we could on the if checking options.buttons === false switch previous and next to'aria-hidden', 'true'. Just 5 setAttribute() invokations and all cases are covered.

Also, why don't we use aria-hidden on button elements instead of that inner span

XhmikosR · 2016-04-04T19:54:43Z

I'm having second thoughts too :/

https://www.w3.org/TR/wai-aria/states_and_properties#aria-hidden

The button is visible, the img is hidden because we have a label for assistive technologies. Which I believe should be aria-labelledby by reading the specs.

Please look at the specs too, and we'll get to it :)

XhmikosR · 2016-04-04T19:55:51Z

/CC @patrickhlauke in case you have some time to chime in.

feimosi · 2016-04-04T19:58:38Z

Ok, that gets quite complicated. We have to postpone it then. I can come back to this issue during the weekend too.

patrickhlauke · 2016-04-04T20:04:57Z

src/baguetteBox.js

@@ -313,8 +327,19 @@
        if (options.buttons === 'auto' && ('ontouchstart' in window || imagesMap[currentGallery].length === 1)) {


completely unrelated, but: am i right in thinking this test for ontouchstart is used to determine whether or not buttons should be shown? if so, this will fall foul of touchscreen-enabled laptops/desktops and mobile/tablet devices with paired mouse/keyboard - see http://patrickhlauke.github.io/getting-touchy-presentation/

Yeah, that is what this test is used for.

/CC @feimosi: maybe make a new issue to track this.

patrickhlauke · 2016-04-04T20:07:37Z

hate to ask, but...do you have a nice isolated test case showing the old and new (after applying this patch) behavior?

patrickhlauke · 2016-04-04T20:24:50Z

having had a quick look, i think all you really want to do in this PR is to set explicit aria-label on the buttons. that should take care of making the buttons themselves announce properly (e.g. "Close button")

XhmikosR · 2016-04-04T20:26:00Z

Cool, I'll get to it then.

It baffles me that HTML_Codesniffer complains though. I'll need to check if there's any issue open upstream.

XhmikosR · 2016-04-04T20:27:09Z

Correction, it must be squizlabs/HTML_CodeSniffer#160.

That is why I always thought I need aria-hidden too for the child image :/

patrickhlauke · 2016-04-04T20:28:14Z

Code analyzers are dumb in many cases, with many false positives/negatives depending on how complex your stuff is. To be clear, aria-hidden is not incorrect per se, just completely unnecessary in this situation.

XhmikosR · 2016-04-04T20:30:40Z

I know, but we need something to check things automatically too :)

OK, so I'll rebase this branch and only add aria-labels on the buttons.

Thanks for all the help!

patrickhlauke · 2016-04-04T20:32:21Z

no probs. there may be more things to fix (e.g. not checked focus handling when a lightbox is opened etc), but leave that for another day ;)

Also switch to `×` instead of a simple `X` for the fallback close button.

XhmikosR · 2016-04-04T20:36:04Z

@feimosi @patrickhlauke: please check for one hopefully last time.

I have also added type=button for clarity.

XhmikosR · 2016-04-04T20:39:52Z

On a side note, it seems we need an alt for the imgs but that's for another time. :)

patrickhlauke · 2016-04-04T20:46:12Z

LGTM at this stage

feimosi · 2016-04-08T15:49:02Z

LGTM too.
@XhmikosR could you rebase on top of master? I want to start keeping the log history clean, unfortunately GitHub doesn't allow me to do it myself.

XhmikosR · 2016-04-08T15:57:43Z

@feimosi: not sure what you see, but the branch is already rebased.

feimosi · 2016-04-08T17:36:52Z

@XhmikosR sorry, GitHub has confused me. It shows the last commit is 10 days old and on master the last one is 6 days old. So looks like it's based on author date instead of commit date? I just want the commits to be in a chronological order aligned with PRs, so it's easier to reason about the changes.

XhmikosR · 2016-04-08T18:17:47Z

That is completely normal. Git is like that, and the date has no importance.

feimosi · 2016-04-08T18:27:12Z

I know how Git works. When you rebase, the commit date is updated (actually it's a new commit). That's useful to keep linear log history because after recent PRs I've noticed the log became quite a mess.

XhmikosR · 2016-04-08T18:29:46Z

No, the rebase alone isn't enough. I'm sorry but this is just insane. That is how things work.

feimosi · 2016-04-08T18:37:26Z

Sorry, I don't understand what you mean, what isn't enough? Correct me where I'm wrong.

XhmikosR · 2016-04-08T18:56:58Z

A rebase does not change the commit date. There are 2 dates, (push) date and commit date.

feimosi · 2016-04-08T19:07:57Z

Actually, there are AuthorDate and CommitDate. It's a normal practice to do a rebase before merging in feature branches workflow. I don't understand why you've resented.

XhmikosR · 2016-04-08T19:10:31Z

Because that is the Author Date which I don't want to change. That is when I made the patch.

feimosi · 2016-04-08T19:18:59Z

I'm just trying to say that Commit Date is more important, as it's used most of the time. You can't change Author Data and I've never asked for that. Just Github's confused me.

feimosi reviewed Apr 4, 2016
View reviewed changes

patrickhlauke reviewed Apr 4, 2016
View reviewed changes

Make the buttons accessible.

004e6e2

Also switch to `×` instead of a simple `X` for the fallback close button.

feimosi merged commit 58063f3 into feimosi:master Apr 8, 2016

XhmikosR deleted the accessibility branch April 8, 2016 18:17

feimosi mentioned this pull request Apr 9, 2016

Touch detection can fail #95

Open

		@@ -313,8 +327,19 @@
		if (options.buttons === 'auto' && ('ontouchstart' in window \|\| imagesMap[currentGallery].length === 1)) {

Make the buttons accessible. #79

Make the buttons accessible. #79

Conversation

XhmikosR commented Mar 29, 2016

feimosi commented Apr 2, 2016

XhmikosR commented Apr 2, 2016

feimosi commented Apr 2, 2016

XhmikosR commented Apr 2, 2016

feimosi commented Apr 2, 2016

XhmikosR commented Apr 2, 2016

feimosi commented Apr 2, 2016

XhmikosR commented Apr 2, 2016

feimosi commented Apr 2, 2016

XhmikosR commented Apr 4, 2016

XhmikosR commented Apr 4, 2016

feimosi commented Apr 4, 2016

XhmikosR commented Apr 4, 2016

feimosi commented Apr 4, 2016

XhmikosR commented Apr 4, 2016

feimosi commented Apr 4, 2016

XhmikosR commented Apr 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feimosi commented Apr 4, 2016

feimosi commented Apr 4, 2016

XhmikosR commented Apr 4, 2016

XhmikosR commented Apr 4, 2016

feimosi commented Apr 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhlauke commented Apr 4, 2016

patrickhlauke commented Apr 4, 2016

XhmikosR commented Apr 4, 2016

XhmikosR commented Apr 4, 2016

patrickhlauke commented Apr 4, 2016

XhmikosR commented Apr 4, 2016

patrickhlauke commented Apr 4, 2016

XhmikosR commented Apr 4, 2016

XhmikosR commented Apr 4, 2016

patrickhlauke commented Apr 4, 2016

feimosi commented Apr 8, 2016

XhmikosR commented Apr 8, 2016

feimosi commented Apr 8, 2016

XhmikosR commented Apr 8, 2016

feimosi commented Apr 8, 2016

XhmikosR commented Apr 8, 2016

feimosi commented Apr 8, 2016

XhmikosR commented Apr 8, 2016

feimosi commented Apr 8, 2016

XhmikosR commented Apr 8, 2016

feimosi commented Apr 8, 2016

Make the `buttons` accessible. #79

Make the `buttons` accessible. #79