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

Made accordion-heading accessible by adding href="#" to its template #2807

Closed
wants to merge 1 commit into from
Closed

Conversation

henrihietala
Copy link

Improves accordion accessibility. This way accordion-groups can be accessed by keyboard.

@cvrebert
Copy link
Contributor

CSS

[...] But in AngularJS adding empty href attributes to link tags will cause unwanted route changes. This is why we need to remove empty href attributes from directive templates [...]

(emphasis added)

@henrihietala
Copy link
Author

@cvrebert Ah, so what we need is to pass the $event to toggleOpen() and call $event.preventDefault() in it. I will update my PR soon.

@jannic
Copy link

jannic commented Oct 21, 2014

Isn't this more or less the same issue as #1667, only on a different widget?
That issue was fixed by adding an empty href. If that was ok for #1667, shouldn't it work here, as well?

BTW, probably obvious to most of you, but it was new to me: href="" is only ok with angular.js, as angular actively prevents the default action in that case: https://docs.angularjs.org/api/ng/directive/a
Therefore, all the stackoverflow articles regarding href="#" vs. href="javascript:void(0)" don't apply here, href="" should really be ok, otherwise it's an angular.js bug and should be reported there.

@blah238
Copy link
Contributor

blah238 commented Oct 22, 2014

@henrihietala @cvrebert @jannic I have created a PR adding an empty href here: #2869

It works fine in the demo as well as my complex ui-router app.

@henrihietala
Copy link
Author

@blah238 Yes, that solves the accessibility issue and doesn't mess up the routing. Nice.

@chrisirhc chrisirhc added this to the 0.12 milestone Nov 7, 2014
@chrisirhc
Copy link
Contributor

Closing this in favor of #2869 which I intend to merge.

@chrisirhc chrisirhc closed this Nov 7, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants