Carousel #6295
Carousel #6295
Conversation
| @@ -467,8 +467,12 @@ export class AmpSlideScroll extends BaseSlides { | |||
| if (showIndex == newIndex) { | |||
| this.scheduleLayout(this.slides_[showIndex]); | |||
| this.scheduleResume(this.slides_[showIndex]); | |||
| this.slides_[showIndex] && | |||
There was a problem hiding this comment.
you dont have to check for this. Show index is promised to exist at this point
| } else { | ||
| this.schedulePreload(this.slides_[showIndex]); | ||
| this.slides_[showIndex] && |
| @@ -515,6 +519,8 @@ export class AmpSlideScroll extends BaseSlides { | |||
| setStyle(this.slideWrappers_[i], 'order', ''); | |||
| } | |||
| this.slideWrappers_[i].classList.remove(SHOWN_CSS_CLASS); | |||
| this.slides_[i] && | |||
| this.slides_[i].setAttribute('aria-hidden', 'true'); | |||
There was a problem hiding this comment.
these are display none so aria-hidden is not needed here.
There was a problem hiding this comment.
@camelburrito We need this here otherwise we can get to a conflicting state where display:none but aria-hidden=false (this can happens if one jumps more than 2 slides, although not something that UI supports yet, it is something that can happen when we do amp-bind)
There was a problem hiding this comment.
+1 to what @aghassemi said.
| @@ -515,6 +517,7 @@ export class AmpSlideScroll extends BaseSlides { | |||
| setStyle(this.slideWrappers_[i], 'order', ''); | |||
| } | |||
| this.slideWrappers_[i].classList.remove(SHOWN_CSS_CLASS); | |||
| this.slides_[i].setAttribute('aria-hidden', 'true'); | |||
There was a problem hiding this comment.
should we removeAttribute here and let the browser take care of this?
There was a problem hiding this comment.
@aghassemi - WDYT?
There was a problem hiding this comment.
SGTM. End result is the same,removeAttribute is a faster DOM operation than setAttribute anyway.
* revert change 4089 * set ariane-hidden to visible elements out of view * remove checks * removing aria-hidden
* revert change 4089 * set ariane-hidden to visible elements out of view * remove checks * removing aria-hidden
Visible slides that are out of view need an aria-hidden.
close #5484