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

Dealing with styling of "a" tags (with or without hrefs?) #142

Closed
pkozlowski-opensource opened this issue Dec 17, 2015 · 41 comments
Closed

Comments

@pkozlowski-opensource
Copy link
Member

A very common markup in Angular version of bootstrap directives goes like this: <a (click)="prev()">Previous</a>

Pure bootstrap version would have an "empty" href (<a (click)="prev()" href="#">Previous</a>) to provide appropriate styling (cursor) but this is not needed in ng-version and can interfere with routing.

The question: how do we deal with this situation? Few ideas:

Now, I must admit that I don't know what is the impact of removing href on accessibility and keyboard navigation, so the encapsulated CSS path might be a non-option in the end...

Thoughts?

@Foxandxss
Copy link
Member

IIRC @wesleycho changed it on uibs to not use an anchor at all.

I am fine with option 1 and 2.

@wesleycho
Copy link
Member

Only cavaet of not using an anchor in UI BS is that I also had to add documentation where the styles from Bootstrap are mirrored so people can easily copy/paste it for their apps.

I'm wary about the anchor tag due to the prior routing issues, and problems with nesting buttons under an anchor tag, which some users wanted to do. I toss my vote to using a div instead, partly because we should also let users nest an anchor tag in their tab heading template if they so wish to to get the routing benefits (assuming they are using tabs to route to different views), but also not interfere with users who want to do more complex things in their tab header.

@icfantv
Copy link
Member

icfantv commented Dec 17, 2015

I vaguely recall there were accessibility issues that arose when switching to <div>.

@icfantv
Copy link
Member

icfantv commented Dec 17, 2015

Also, is it worth looking at what Material does?

@mendeza
Copy link

mendeza commented Apr 6, 2016

This is what we do at Thomson Reuters:
<a (click)="prev()" href>Previous</a>
The href, even when truly empty like this, keeps the anchor focusable and in tabindex. This is essential for A11y.

@RobJacobs
Copy link
Contributor

@mendeza That's the pattern used in the angular-ui-bootstrap library as well.

<a href></a>

It's the # in the href tag that causes problems.

@mendeza
Copy link

mendeza commented Apr 6, 2016

@RobJacobs Precisely - and there's simply no need for it.

@wesleycho
Copy link
Member

It sounds like we're going with leaving an empty href - shall we close this issue?

@pkozlowski-opensource
Copy link
Member Author

We should check what happens wit new router and empty hrefs before closing.

@pkozlowski-opensource
Copy link
Member Author

Empty hrefs are failing tests with:

Some of your tests did a full page reload!

@pkozlowski-opensource
Copy link
Member Author

So, my best idea for today is to use href="javascript:void(0)". While it is far from being beautiful (quite ugly, to be frank...) it actually gets the job done.

Thoughts?

@wesleycho
Copy link
Member

We will need to remove that in the future if we do that - it is a CSP violation.

@pkozlowski-opensource
Copy link
Member Author

it is a CSP violation

Oh man.... Indeed...

@mendeza
Copy link

mendeza commented Jul 9, 2016

If this a limited and specific use case such as the example shown at the outset, use a button instead of an anchor. Buttons are more mobile-friendly anyway.

@icfantv
Copy link
Member

icfantv commented Jul 9, 2016

@mendeza there are going to be a few places in the library where we use the anchor tag and cannot use a button. E.g., tab headings. Additionally, styling a button as a link has some a11y ramifications. We'll need to figure this out somehow. (I haven't looked, but BS4 CSS may already handle this for tab headings...idk off the top of my head).

@pkozlowski-opensource
Copy link
Member Author

OK, another idea. How about this:

  • we add empty href (this gives us proper cursor and accessibility)
  • we call prevent default / stop propagation in a click handler.

To avoid pain of writing too much for prevent default we could use this trick: <a href (click)="!!doSth()">. Taken from angular/angular#2042 (comment)

@wesleycho
Copy link
Member

Can't say I love API that hides behavior like that, but it'll do for solving this problem.

@pkozlowski-opensource
Copy link
Member Author

Can't say I love API that hides behavior like that, but it'll do for solving this problem.

It makes me feel dirty as well.... but the big advantage is that it works :-)

@mendeza
Copy link

mendeza commented Jul 10, 2016

Just to keep playing devil's advocate; <button/> might still be an option. In the BS4 docs I replaced <a> elements in a tabset, and it didn't break either styling or keyboard control. Of course more testing would be required.

tab-buttons

@wesleycho
Copy link
Member

Bootstrap moved to classes purely in Bootstrap 4 I believe, and less selector opinionation.

I believe anchor tags are better for accessibility in many situations, but I'd need to look into it.

@mendeza
Copy link

mendeza commented Jul 10, 2016

@wesleycho, some food for thought...

As an aside, I hope you guys are getting some rest. You've been going what seems like 24-hours a day.

@pkozlowski-opensource
Copy link
Member Author

You've been going what seems like 24-hours a day.

There are some advantages of having people in different time-zones :-)

@AlmeroSteyn
Copy link

Hi everyone.

<a> with no href or an empty href is not a true link. The href should be populated with the actual destination url, otherwise it is interpreted as linking to a position in the current document.

Keyboard users expect links to be activated with Enter and buttons with Enter or Space. Therefore having a link with only (click) will make it inaccessible to keyboard users. And all users will lose the ability to open this link in a new window or tab, as can be done with an <a> with a proper href.

One would need to implement all the required keyboard events and provide the proper role (role="link") if not using an <a> tag. But the <a> tag remains the superior choice.

@mendeza provides some great resources. Here is another very clear one from the last week: https://marcysutton.com/links-vs-buttons-in-modern-web-applications/

So if it does navigation and change the URL (with the exception of a form post and subsequent navigation) it should really be a properly formatted <a> tag.

If it initiates a user action in the page it should be a button.

Another thing to keep in mind is NOT to use buttons and style them as links or use links and style them as buttons as it can also lead to confusion (see Marcy Sutton's post mentioned above).

@pkozlowski-opensource
Copy link
Member Author

@AlmeroSteyn If I'm reading you correctly the lesson from all this would be to use <button>s instead of <a> in tabs / accordions and the like?

@mendeza
Copy link

mendeza commented Jul 10, 2016

Hi @AlmeroSteyn, thanks for this. I do believe everyone here is aware that <a> needs href, that is the basis of the thread.

I totally agree with:

So if it does navigation and change the URL (with the exception of a form post and subsequent navigation) it should really be a properly formatted tag.

If it initiates a user action in the page it should be a button.

NOT [...] use links and style them as buttons

But I disagree that <button> should never be styled as a link. At least one of the links I provided above puts that forth as an acceptable option. And in the BS4 world it has no ill effect on A11y.

@AlmeroSteyn
Copy link

AlmeroSteyn commented Jul 10, 2016

@pkozlowski-opensource If it is not linking to another resource by URL that is the way to go, IMO.

@mendeza yes this is a bit of a gray area when looking online but the reason for that is that it can lead to confusion further down the line. From Marcy's post:

If a screen reader user calls tech support and gets instructions to “click the button” in your UI that’s really coded as a link, they may have trouble finding it. Also, consider voice interfaces: if you say a command to click a button but it’s really coded as a link, you might have problems, no?

The style itself does not affect the core of how the browser or screen reader handles the element, but these kinds of soft issues can happen.

@pkozlowski-opensource
Copy link
Member Author

Just had a look at the various components and it seems like we can replace <a> with <button> for most of our components (tabs, pagination, ...). Some others would give us headache, though (ex. pager)

@AlmeroSteyn
Copy link

AlmeroSteyn commented Jul 10, 2016

@mendeza Yes, inside the app, by using proper descriptive text and things like aria-describedby one can avoid a lot of confusion.

Outside that we have little control over what users will do. So I think the takeaway is simply that using the tags as intended removes the issues without further effort (First rule of ARIA).

But this remains an area that can be easier to discuss than to implement.

NOTE: The example you show above is a big issue. An <a> with role="button". That should definitely not be happening (Second rule of ARIA).

@mendeza
Copy link

mendeza commented Jul 10, 2016

@AlmeroSteyn - agreed, but I was conceding the point that the BS4 team have punted on proper A11y for their carousel, thus it's arguable that it isn't the ng-bootstrap team's responsibility to fix. I haven't yet looked at the (to use @wesleycho's phrase) selector opinionation of the BS4 carousel, which one could imagine might defeat or make too onerous a fix by ng-bootstrap. My team wrote a new carousel that is accessible, at the expense of not leveraging BS4 CSS and adding custom code.

@icfantv
Copy link
Member

icfantv commented Jul 10, 2016

@pkozlowski-opensource, might it be worth seeing what AM is doing here or reaching out to Kara or Jeremy to see what are doing or intend to do?

W/r/t tab changing and using buttons, wiring in tab navigation to browser history is common and I think switching to buttons would break this, no? Or make it quite a bit more difficult to manage. Same with accordions.

@mendeza
Copy link

mendeza commented Jul 10, 2016

Due to the interesting discussion about semantic markup and A11y, the original question perhaps got a bit obscured; even if <button> took the place of <a href> in many components, at some point there would still be a need for something like:

@Directive({
  selector: `click-stop-propagation`
  events: 'stopClick($event)'
})
class ClickStopPropagation {
  stopClick(event:Event) {
    event.preventDefault();
    event. stopPropagation();
  }
}
<a (click)="doSomething()" click-stop-propagation></a>
// or
<a (click)="!!doSomething()"></a>
// or even?
<a (click)="!!()"></a>

So having a nice-enough pattern that people don't mind re-using when needed (at least for internal project consistency) is still a definite nice-to-have, no?

@mendeza
Copy link

mendeza commented Jul 11, 2016

So apparently what's needed is the NG2 version of: https://github.com/angular/angular.js/blob/master/src/ng/directive/a.js

@pkozlowski-opensource
Copy link
Member Author

So the Bootstrap 4-alpha.3 release made this topic rather urgent (styling is broken without hrefs). I've spent some time today trying out various things and here is my current thinking:

Alternative, practical proposals (that is: ones we can turn into code and merge) are more than welcomed!!!

@RobJacobs
Copy link
Contributor

That approach (#493) has worked out the best so far in the angular-ui-bootstrap library.

icfantv pushed a commit that referenced this issue Jul 29, 2016
This fixes accessibility and cursor display issues.
Part of #142

closes #490
icfantv pushed a commit that referenced this issue Jul 29, 2016
@pkozlowski-opensource pkozlowski-opensource removed this from the alpha.1 milestone Jul 29, 2016
@pkozlowski-opensource pkozlowski-opensource modified the milestones: alpha.6, beta.0 (Feature-complete) Sep 14, 2016
@pkozlowski-opensource
Copy link
Member Author

Very important voice in our discussion: twbs/bootstrap#20737

It looks like using buttons for all those links would be a sensible way to go. We need to follow up on this issue.

@icfantv
Copy link
Member

icfantv commented Sep 17, 2016

I find it interesting (and this in no way invalidates the issue filed) that the account used to file that issue was literally created today.

@dmytroyarmak
Copy link
Contributor

@pkozlowski-opensource Is there some list of todos for this issue? What is the current status?

@pkozlowski-opensource
Copy link
Member Author

@dmytroyarmak there is no particular TODO. The status is that we want to do full accessibility review with people using screen readers and then incorporate changes based on their feedback. But it will take time. If you / anyone else know people using screen readers on a daily basis we would love to hear about any issues encountered in practice.

@pkozlowski-opensource pkozlowski-opensource removed this from the beta.0 (Feature-complete) milestone Mar 27, 2017
@pkozlowski-opensource
Copy link
Member Author

After over a year we are well set on the "pattern" from #493. Now new information surfaced as of lately and I don't see us revising the approach without new info. Based on this I'm going to close this issue for now but will keep an eye on any new developments in this area.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants