Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(tabs) dummy tabs should not have acccessibilty roles #9452

Merged
merged 1 commit into from
Oct 14, 2016

Conversation

clshortfuse
Copy link
Contributor

  • Remove role="tab" from dummy tabs

Fixes #9450

@clshortfuse clshortfuse added a11y This issue is related to accessibility needs: review This PR is waiting on review from the team labels Aug 29, 2016
@clshortfuse
Copy link
Contributor Author

clshortfuse commented Aug 29, 2016

I'm not understanding why the spec tests are actively checking for md-dummy-tab to have a role='tab'.

@clshortfuse clshortfuse added needs: work and removed needs: review This PR is waiting on review from the team labels Aug 29, 2016
@crisbeto
Copy link
Member

crisbeto commented Aug 29, 2016

I'm not sure either. It could have something to do with them forwarding focus to the real tabs, but I'm just guessing.

@ThomasBurleson ThomasBurleson added this to the 1.1.2 milestone Aug 30, 2016
@ThomasBurleson ThomasBurleson modified the milestones: 1.1.3, 1.1.2 Sep 7, 2016
@ThomasBurleson ThomasBurleson added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Sep 7, 2016
@topherfangio
Copy link
Contributor

@clshortfuse Can you do some testing by removing the role from the non-dummy tabs and see if keyboard accessibility still works?

@clshortfuse
Copy link
Contributor Author

@topherfangio I wouldn't say it works properly because I found a keyboard bug with dynamic heights in master where you can't navigating while the animation is running, but yes, keyboard works like master.

Travis Fails because of a spec test is expecting a dummy tab to have a role. I'm not sure why this would be expected behavior. I'll need some clarification about this.

expect(tabItem.attr('role')).toBe('tab');

@topherfangio
Copy link
Contributor

@clshortfuse Is the dynamic height bug you mentioned fixed by the dummy tabs having the role?

@clshortfuse
Copy link
Contributor Author

@topherfangio I discovered that keyboard events aren't processed while a tab is in a transition phase and dynamic height is on. In other words, go to demo and spam right, enter and right, enter. It's unrelated to this PR, just something I found.

I'm going to just remove the role check in dummy tabs and add a spec test that ensures dummy tabs do not have role attribute.

I will say, there is another way to solve the issue, which is to set a dummy tab with a hidden attribute. Technically, dummy tabs are hidden and it may be a better solution, but it creates a lot more problems than just removing the role attribute.

@clshortfuse clshortfuse added needs: review This PR is waiting on review from the team and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: work labels Sep 13, 2016
@clshortfuse
Copy link
Contributor Author

clshortfuse commented Sep 13, 2016

Spec tests added. OK with ChromeVox.
Needs to be tested:

JAWS (Windows)

  • Chrome
  • IE
  • Edge
  • FireFox

NVDA (Windows)

  • Chrome
  • IE
  • Edge
  • FireFox

VoiceOver (Mac)

  • Safari
  • Chrome
  • Safari Mobile

TalkBack (Android)

  • Chrome

@clshortfuse clshortfuse added the needs: manual testing This issue or PR needs to have some manual testing and verification done label Sep 13, 2016
@ThomasBurleson ThomasBurleson added needs: presubmit and removed needs: manual testing This issue or PR needs to have some manual testing and verification done needs: review This PR is waiting on review from the team labels Oct 5, 2016
@ThomasBurleson ThomasBurleson modified the milestones: 1.1.2, 1.1.3 Oct 5, 2016
@kara kara added the pr: merge ready This PR is ready for a caretaker to review label Oct 14, 2016
@kara kara merged commit 9e0c30e into angular:master Oct 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a11y This issue is related to accessibility pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants