Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

Accessibility for tab component #3908

Closed
wants to merge 2 commits into from
Closed

Accessibility for tab component #3908

wants to merge 2 commits into from

Conversation

RicardoRFaria
Copy link
Contributor

We added some features to provide accessibility in tabs.

  • Aria attributes to describe the component and your states for screen readers;
  • Arrow navigation like:
    • Tab to change focus from tab to your container
    • Top and left to backward, right and down to forward
    • Enter to select one tab

The documentation has been updated and tests were created.
Referencing the discussion #3526

wesleycho
wesleycho reviewed Jul 17, 2015
View reviewed changes
src/tabs/docs/readme.md
@@ -25,11 +25,15 @@ AngularJS version of the tabs directive.
* `active` <i class="glyphicon glyphicon-eye-open"></i>
_(Defaults: false)_ :
Whether tab is currently selected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary?

@wesleycho
Copy link
Contributor

I am not sure that this is the best way to handle this.

Also the history needs to be fixed (there should not be a merge commit).

@wesleycho wesleycho added this to the Backlog milestone Jul 17, 2015
@RicardoRFaria RicardoRFaria force-pushed the tabs_accessibility branch 2 times, most recently from 2eb6af7 to 455d3fc Compare July 20, 2015 12:34
@RicardoRFaria
Copy link
Contributor Author

Hi @wesleycho, I fixed the history and migrate key navigation to pure javascript.
What you think it would be the best way to add accessibility for this component?

Added accessibility for component "tabs"
ebdda0f 
- Created tests to check aria attributes in tabs
- Added arrow navigation for tabs
- Updated tabs documentation
@RicardoRFaria
Copy link
Contributor Author

I made a merge to update this branch and squashed the commits.

wesleycho
wesleycho reviewed Aug 10, 2015
View reviewed changes
src/tabs/tabs.js
@@ -192,6 +193,7 @@ angular.module('ui.bootstrap.tabs', [])
scope: {
active: '=?',
heading: '@',
id : '@',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be @?

@wesleycho
Copy link
Contributor

There are a lot of code style problems here - most problematic though is the DOM traversal mentioned #3908 (comment) . That would be a breaking change since if a user is overriding the template, it enforces DOM structure on that template.

@RicardoRFaria
Copy link
Contributor Author

Sorry wesley, automatic merge made some changes in code style that I didn't see. My fault.
About DOM traversal problem, I will need more time to think about it. I trought that if add event to each 'a' element after tab is added we would avoid .parent().parent(), but I need more time to test.

@icfantv
Copy link
Contributor

icfantv commented Oct 29, 2015

Closing in favor of #4772.

@icfantv icfantv closed this Oct 29, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
component: tab needs: work type: accessibility
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.
3 participants
@RicardoRFaria @wesleycho @icfantv