Repository navigation
a11y: homepage sweep #4177
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,16 +43,16 @@ export default class Showcase { | |
| start () { | ||
| for (let i = 0; i < this.slides.length; i++) { | ||
| const n = this.slides[i] | ||
| const $iChoice = $("[aria-controls='" + n + "']", this.container) | ||
| const $iContainer = $('#' + n, this.container) | ||
|
|
||
| $iContainer.prepend('<button type="button" class="showcase-back" aria-controls="showcase-index" aria-expanded="false" aria-label="Back" tabindex="-1"></button>') | ||
|
Contributor
There was a problem hiding this comment. Why exclude this from the tab index? Shouldn't you want to naturallly focus it instead so that you can get back out of the showcase?
Author
There was a problem hiding this comment. by default actually no. we set the tabindex to 0 here: https://github.com/elementary/website/pull/4177/changes#diff-a375a752112dc75ce153214314f0ad86ca13deddee1be26df741aebae00ea20aR133 only for the relevant tab, so once you enter one tab's contents your next action is to go back. this is technically a bit of a holdover from before i got the it might be able to be removed now? but it doesn't do any harm being this way i don't think
Contributor
There was a problem hiding this comment. Not sure that link is taking me to the right spot in my browser (grrr GH) but should I be focused on this back button when after I hit enter and select a "tab"? |
||
|
|
||
| // each choice button | ||
| const that = this | ||
| $iChoice.on('click', function (e) { | ||
| e.preventDefault() | ||
| that.slideTo($(this).attr('aria-controls')) // slide on click of button | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -98,10 +98,68 @@ export default class Showcase { | |
| return console.error("could not find requested slide '" + rSlide + "'") // log an error | ||
| } | ||
|
|
||
| const $container = $(this.container) | ||
|
|
||
| // update aria-expanded to alert which slide is active | ||
| $container.find('button[aria-controls]').each(function() { | ||
| const $button = $(this) | ||
| if ($button.attr('aria-controls') === rSlide) { | ||
| $button.attr('aria-expanded', true) | ||
| } | ||
| else { | ||
| $button.attr('aria-expanded', false) | ||
| } | ||
| }) | ||
|
|
||
| // we want to only mark them as hidden after some time has elapsed | ||
| // this allows us to keep the slide height animation | ||
| let $targetsToHide = []; | ||
| let $targetToFocus = false; | ||
|
|
||
| /** | ||
| * unhide a specific jQuery element | ||
| * | ||
| * we first set the hidden attribute to false, so that the DOM tree | ||
| * can update. we then set the active class on the next frame to allow | ||
| * the animation to play nicely | ||
| * | ||
| * @param {*} $target | ||
| */ | ||
| const unhide = function($target) { | ||
| $target.attr('hidden', false) | ||
| requestAnimationFrame(function() { | ||
| $target.addClass('active') | ||
| }) | ||
| $target.find('.showcase-back').attr('tabindex', 0) | ||
| } | ||
|
|
||
| /** | ||
| * hide a specific jQuery element | ||
| * | ||
| * we don't need to worry about any animation frames here! | ||
| * | ||
| * @param {*} $target | ||
| */ | ||
| const hide = function($target) { | ||
|
Contributor
There was a problem hiding this comment. Inspecting this in FF when you click a button and go to the screenshot the entire list goes away in the a11y tree. Is that what we really want? I would think that we still want to mention that the list is there somehow and don't change the entire tree out from under a user. (I have NOT tested this yet with a screenreader)
Author
There was a problem hiding this comment. 1st point: imo, it makes sense? 2nd point: it's out of the viewport in a way that you can only see it again is by going back, so why wouldn't it be what we want? in theory, the only "valid" action at the point of entering a tab is to go back, because there is no "go forwards". the current site has each link out of the viewport, so the only way you can navigate it is by seeing the links in the bottom change or just remembering the order. since we remove the if we want to keep the tree, then IMO the section needs some kind of redesign to keep each button visible and within the viewport at all times. i'm not sure what a preferred solution there would look like, especially when considering mobile.
Author
There was a problem hiding this comment. just got back from a run, apologies if i came off as rude, not my intention :) i largely agree that this should be seen as an accordion, and i think the convention would be that the title of the current item would remain focused even when expanded. but you also have to consider your available actions & what's visible. let's use this MDN playground as an example. if you press 
you then have 2 options:
with my current showcase section, we don't have (1) as you're focusing the div itself, so the next hope this makes a little more sense wrt how i'm thinking! ^^ |
||
| $target.removeClass('active') | ||
| $target.find('.showcase-back').attr('tabindex', -1) | ||
| $targetsToHide.push($target) | ||
| } | ||
|
|
||
| const $indexSlide = $(this.index, this.container) | ||
|
|
||
| if (rSlide === 'index') { | ||
| unhide($indexSlide) | ||
|
|
||
| if (this.current) { | ||
| // focus the previous button when returning to the index | ||
| $targetToFocus = $('button[aria-controls="' + this.current + '"]') | ||
| } | ||
| else { | ||
| $targetToFocus = $(this.index) | ||
| } | ||
| } else { | ||
| hide($indexSlide) | ||
| } | ||
|
|
||
| // iterates through slides based on this.slides | ||
|
|
@@ -110,24 +168,33 @@ export default class Showcase { | |
| const $n = $('#' + n, this.container) // current iterated slide | ||
|
|
||
| if (n === rSlide) { // if correct slide | ||
| unhide($n) | ||
| $targetToFocus = $n | ||
| } else { | ||
| hide($n) | ||
| } | ||
| } | ||
|
|
||
| this.current = rSlide | ||
| $container.trigger('change', { | ||
| active: rSlide | ||
| }) | ||
|
|
||
| if (this.current !== 'index') { | ||
| $('body').animate({ | ||
| scrollTop: $container.offset().top | ||
| }, 100) | ||
| } | ||
|
|
||
| this.resize() // resize the container | ||
|
|
||
| // hide and focus roughly after animations are complete | ||
| $container.one('transitionend', function() { | ||
| for (const $target of $targetsToHide) { | ||
| $target.attr('hidden', true) | ||
| } | ||
| $targetToFocus.focus() | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.