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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Form] Add tabIndex={-1} to implicit submit form #2447

Merged
merged 1 commit into from Nov 21, 2019
Merged

Conversation

dleroux
Copy link
Contributor

@dleroux dleroux commented Nov 19, 2019

WHY are these changes introduced?

While doing an a11y audit on our Form component I found that our Form component is failing because ARIA hidden element must not contain focusable elements.

Our Form contains an implicitSubmit prop which generates a visually hidden submit button with aria-hidden set to true.

WHAT is this pull request doing?

adds a tabIndex={-1} to our Form implied submit button.

How to 馃帺

Using Fast pass on the Accessibility extension for web ensure there are no errors with the playground below.

Copy-paste this code in playground/Playground.tsx:
import React, {useState, useCallback} from 'react';
import {Page, Form, FormLayout, TextField, Button} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <FormExample />
    </Page>
  );
}

class FormExample extends React.Component {
  state = {
    url: '',
  };

  render() {
    const {url} = this.state;

    return (
      <Form noValidate onSubmit={this.handleSubmit}>
        <FormLayout>
          <TextField
            value={url}
            onChange={this.handleChange('url')}
            label="App URL"
            type="url"
          />

          <Button submit>Submit</Button>
        </FormLayout>
      </Form>
    );
  }

  handleSubmit = (event) => {
    this.setState({url: ''});
  };

  handleChange = (field) => {
    return (value) => this.setState({[field]: value});
  };
}

馃帺 checklist

@dleroux
Copy link
Contributor Author

dleroux commented Nov 19, 2019

@dpersing once you confirm that this in fact the behavior we want I'll update the changelog.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 19, 2019

馃挦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified2
Files potentially affected3

Details

All files potentially affected (total: 3)
馃搫 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

馃З src/components/Form/Form.tsx (total: 3)

Files potentially affected (total: 3)


This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

Copy link
Contributor

@dpersing dpersing left a comment

Choose a reason for hiding this comment

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

This is an okay temporary fix. I'm curious as to why we're using visually hidden styles on a component that users aren't meant to interact with, however.

Hiding with display: none or hidden would avoid having to apply aria-hidden="true" and tabindex="-1".

@dleroux
Copy link
Contributor Author

dleroux commented Nov 19, 2019

I believe is so that the form does submit by simply clicking enter as oppose to actually clicking on the button. Display none wouldn't have the same effect because the button would not be in the DOM.

If this is a temporary fix, what would the right fix be?

Copy link
Contributor

@dpersing dpersing left a comment

Choose a reason for hiding this comment

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

Display none wouldn't have the same effect because the button would not be in the DOM.

Oh boy, you're right, now that I've reread the whole thing. 馃う鈥嶁檧 Let'er rip. 馃帀

@kaelig
Copy link
Contributor

kaelig commented Nov 20, 2019

Would it help consumers to see an example in the style guide, or is this an edge case we want to discourage?

@dleroux
Copy link
Contributor Author

dleroux commented Nov 20, 2019

Would it help consumers to see an example in the style guide, or is this an edge case we want to discourage?

I'm not sure it's worth it because it is the default behavior. I'm guessing a safeguard for when people add more than 1 field and no submit button is present, clicking enter would not trigger the on submit.

@dleroux dleroux merged commit 6dbf07c into master Nov 21, 2019
@dleroux dleroux deleted the implicit-submit branch November 21, 2019 14:44
@dleroux dleroux temporarily deployed to production December 4, 2019 14:42 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants