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

Add carousel example #958

Merged
merged 66 commits into from
Jan 15, 2019
Merged

Add carousel example #958

merged 66 commits into from
Jan 15, 2019

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Dec 20, 2018

This adds the basic carousel implementation called for by issue #458.

Preview Link

jongund and others added 30 commits March 18, 2017 07:07
initial draft of design pattern for carousel for issue #43.
Updated Carousel Design Pattern and Examples
Merge branch 'master' into carousel-v2
@mcking65
Copy link
Contributor

@jongund commented:

@mcking65 I have pushed the following changes:

  • Changed h2s to h3s in the slides

Looks good.

  • Changed using button.disabled property to aria-disabled

Ditto

  • Changed pause button labels to "Start automatic slide rotation" and "Stop automatic slide rotation"

Good, although still waiting for input from others on the mailing list. This is easy to adjust at any time if the group decides on something different.

  • Added disabled property to documentation

Thank you. I'll now finish up my part of the documentation work.

@mcking65 mcking65 changed the title WIP: Add carousel example Add carousel example Jan 11, 2019
changed <code> tags on line 459 so just "disabled" is wrapped; moved some punctuation inside ending quotation marks, changed "unles" to "unless" on line 232.
@annabbott
Copy link

annabbott commented Jan 12, 2019

I can't figure out how to make edit suggestions to the text - when I go to Files, all I see is carousel code.

Second sentence of second paragraph following h1
For instance, Rotation stops when users move focus...
Lower case "r" on "rotation".

The rest looks good to me.
NOTE: I did NOT test with an AT.

@jnurthen
Copy link
Member

jnurthen commented Jan 12, 2019

Second sentence of second paragraph following h1
For instance, Rotation stops when users move focus...
Lower case "r" on "rotation".

@annabbott I added a review comment for you

@charmarkk
Copy link
Contributor

charmarkk commented Jan 12, 2019 via email

mcking65 and others added 2 commits January 12, 2019 13:15
Fix capitalization.

Co-Authored-By: mcking65 <a11yThinker@Gmail.com>
@jongund
Copy link
Contributor Author

jongund commented Jan 14, 2019

@mcking65 Removed the use of aria-describedby from each slide and updated the regression test to use new ids

@accdc
Copy link

accdc commented Jan 15, 2019

Hi,
It looks good to me. As we spoke of during the call, minus the aria-describedby attribute.

As discovered during testing, screen reader support for aria-roledescription is still incomplete. As such, the screen reader support of this design pattern is not sufficient to justify mainstream developers to use this as part of public web technologies, but for now, this widget is something that screen reader providers can use to test against to make sure this functionality is built in as time goes on.

@mcking65
Copy link
Contributor

All review comments have been addressed. Completion of regression test development is tracked by issue #969. Merging as discussed in today's meeting. Thank you all, especially @jongund, for helping bring this to life!

@mcking65 mcking65 merged commit 4c846e4 into master Jan 15, 2019
@mcking65 mcking65 deleted the issue458-add-carousel-example branch January 15, 2019 05:47
michael-n-cooper pushed a commit that referenced this pull request Jan 15, 2019
Add carousel example (pull #958)

Resolves issue #458 by adding a basic example of the carousel pattern that demonstrates how to make automatic rotation accessible.
@shirsha
Copy link

shirsha commented Jan 28, 2019

The content over the image, and previous/next button are difficult to see due to the busyness of the image behind it. Add a background color to the buttons and insert CSS color block between the text and the image so that they can be visible even when the background image is busy.

@elamurugan-r
Copy link

elamurugan-r commented Dec 16, 2021

a)

<section aria-roledescription="carousel" aria-labelledby="carousel-header" >
        <h1 id="carousel-header" >Popular Web series and Episodes</h1>
        <button>Trigger</button>
</section>

b)

<div role="region" aria-roledescription="carousel" aria-labelledby="carousel-header" >
        <h1 id="carousel-header" >Popular Web series and Episodes</h1>
        <button>Trigger</button>
</div>

For the above codes, axe-core-4.3.5 gives the following error eventhough using implicit/explicit aria-role:
a & b) aria-roledescription must be on elements with a semantic role.
Issue Description: Ensure aria-roledescription is only used on elements with an implicit or explicit role

Screen reader (ChromeVox) is reading the above as
a) Section Trigger button
b) Popular Web Series and its Episodes Trigger

Its not reading the text inside aria-roledescription.

FYI: Even the carousel design pattern examples provided in w3c is also not reading the aria-roledescription

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Example Page Related to a page containing an example implementation of a pattern
Development

Successfully merging this pull request may close these issues.

9 participants