Ensure Buttons with a url prop output valid HTML

[BPScott]

WHY are these changes introduced?

Ensure disabled Buttons with a url prop output valid HTML

Fixes #757.

WHAT is this pull request doing?

When a button has a url it is rendered as an <a> tag, which does not
have a disabled attribute.

Stop adding the disabled attribute in that case.

As disabled is an invalid prop it doesn't do what we were hoping in this context

You can't click these <a> tags due to pointer-events:none being set on them, but you can still tab to them using the keyboard.

@dpersing / others: We should come up with what we want the expected behaviour to be when rendering a <Button url="foo" disabled>BAR</Button>

EDIT! Plan made: use an <a> without a href

How to 🎩

Copy-paste this code in playground/Playground.tsx:

import * as React from 'react';
import {Button, Page} from '@shopify/polaris';

interface State {}

export default class Playground extends React.Component<{}, State> {
  render() {
    return (
      <Page
        title="Playground"
        primaryAction={{content: 'View Examples', url: '/examples'}}
      >
        <h2>Normal</h2>
        <Button>ONE (rendered as &lt;button>)</Button>
        <Button url="/foo">TWO (rendered as &lt;a>)</Button>
        <h2>Disabled</h2>
        <Button disabled>THREE (rendered as &lt;button>)</Button>
        <Button disabled url="/foo">
          FOUR (rendered as &lt;a>)
        </Button>
        <h2>Loading</h2>
        <Button loading>FIVE (rendered as &lt;button>)</Button>
        <Button loading url="/foo">
          SIX (rendered as &lt;a>)
        </Button>

        <h2>Final</h2>
        <Button>FINAL BUTTON</Button>
      </Page>
    );
  }
}

Ensure that the <Button disabled url="/foo">TWO</Button> element does not render a disabled attribute in the generated HTML markup.

[dpersing]

I think there are two things to consider: The disabled-related functionality and the styling.

Ensure that the TWO element does not render a disabled attribute in the generated HTML markup.

Thinking about this more, I think it would make sense to convert the "disabled" link to a non-interactive link by removing the href. That is, if someone produces a "disabled" link, it shouldn't have a valid href so it can't be accessed. This renders the <a> but makes it non-interactive.

Another alternative would be to add tabindex="-1" to the "disabled" link. However, this doesn't convey anything semantic, so I think it's not as clean a solution.

Whichever way we go, I think it would be good to also include a note about how the props can cancel each other out in the props explorer help text.

[kaelig]

Another alternative would be to add tabindex="-1" to the "disabled" link. However, this doesn't convey anything semantic, so I think it's not as clean a solution.

Would that link still show up when someone using a screen reader pulls the list of all links in the page? If so, this may not be a good solution, as we want the link to be unreachable for everyone.

[kaelig]

This PR fixes the issue, and the HTML is now valid.

Requires additional accessibility work for certain edge cases, but I think we can go ahead and merge.

[dpersing]

Would that link still show up when someone using a screen reader pulls the list of all links in the page?

It would. The negative tabindex just makes it unreachable with the keyboard. To make it hidden to screen reader users, it would also need aria-hidden="true". @kaelig

[BPScott]

Thinking about this more, I think it would make sense to convert the "disabled" link to a non-interactive link by removing the href. That is, if someone produces a "disabled" link, it shouldn't have a valid href so it can't be accessed. This renders the <a> but makes it non-interactive.

I think this is the preferable solution too. This will require a wee refactor as (sensibly) you can't omit the url property when calling <UnstyledLink> so we'd have to output a <span> or an <a> without a href within Button rather than deferring to UnstyledLink but I think that's fine.

[kaelig]

Should we still merge this?

[BPScott]

Dragging it up to date now

[BPScott]

Daft realisation: why bother rendering span or a's without hrefs - just render <button disabled> instead!

EDIT: Per Devon on Slack: Because buttons get announced in a separate list and it might be odd for someone to hear a disabled button in that list then for it to disappear when it gets enabled (as it is no longer a button)

[dpersing]

🎉

[kaelig]

Would this still appear as a navigable anchor in the accessibility tree (just like a good old <a name="foo"></a>)?

[dleroux]

would a span be a better idea @dpersing?

[dleroux]

Same with this one. The code is clear enough imo.

[BPScott]

Whoops, I thought I merged this last week!