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

[Bug]: Popover.Button emits type="button" even when the component isn't a button #619

Closed
MrLeebo opened this issue Jun 14, 2021 · 9 comments

Comments

@MrLeebo
Copy link

MrLeebo commented Jun 14, 2021

What package within Headless UI are you using?

@headlessui/react

What version of that package are you using?

v1.1.0

What browser are you using?

Chrome

Reproduction repository

https://codesandbox.io/s/magical-bash-ij6dg

Describe your issue

Hello headlessui! 👋

This is a really minor issue, but I thought I'd do my due diligence and write it up.

Given this component:

<Popover.Button as="a" href="#">Click Me</Popover.Button>

The emitted HTML is:

<a href="#" type="button">

However, type="button" is not an applicable property for an anchor tag. Ordinarily the extra prop is harmless, but under a perfect storm of unlikely circumstances, it can make a difference. For example, normalize.css uses it as a CSS selector:

[type="button"] {
  -webkit-appearance: button;
}

So this can cause the link to be styled like a button, for instance, in an older version of Chrome (v78). If you try to override the property by adding type={null} or something to the component, your prop is ignored.

This is a really minor issue so I won't be offended if you think the best course is to just close it and move on, but I think that it would be better if Popover.Button only emitted type="button" when the component tag is the default as="button".

@mikoz93
Copy link

mikoz93 commented Jun 18, 2021

What's the use case of setting the Popover.Button as an a element, given that it's supposed to open the popover and not navigate anywhere? I'm not sure if this is the correct use of the as prop here.

@MrLeebo
Copy link
Author

MrLeebo commented Jun 18, 2021

The button is inside the Popover.Panel, meant to close the popover and navigate to the next page. The popover is part of a app-level Layout component, so it won't be closed by the page navigation. I used a in the example to simplify the explanation, but in practice it was a Link component from react-router.

@mikoz93
Copy link

mikoz93 commented Jun 18, 2021

The Popover.Button is meant to be used as a toggle to open/close the panel and sit outside of it. It seems like you need to use the Link component directly instead.

<Popover>
  <Popover.Button>Toggle popover</Popover.Button>
  <Popover.Panel>
    <div>
      <Link to="/page1">Page 1</Link>
      <Link to="/page2">Page 2</Link>
    </div>
  </Popover.Panel>
</Popover>

@MrLeebo
Copy link
Author

MrLeebo commented Jun 19, 2021

That leaves the popover open after the user clicks on the link. The documentation points out that you can use Popover.Button inside the panel to close it.

@maximegheraille
Copy link

maximegheraille commented Jun 24, 2021

@MrLeebo this is the case but when having a Popover.Button with a <Link> inside your Popover.panel and using tab to go to the link and using enter, it closes the Panel but it does not go to the href.

<Popover.Panel>
    <div>
        <Popover.Button>
                <Link href="/page1">Page 1</Link>
        </Popover.Button>
        <Popover.Button>
              <Link href="/page2">Page 2</Link>
        </Popover.Button>
    </div>
  </Popover.Panel>

@MrLeebo
Copy link
Author

MrLeebo commented Jul 4, 2021

@maximegheraille that's why the repro is <Popover.Button as={Link} to="/page2">, not them separately

@ianwalter
Copy link

I have the same problem. Adding type="button" makes the element display as a button in Safari and makes it look bad (I'm trying to use an image as the trigger).

@arpitjacob
Copy link

Have the same issue with

<Disclosure.Button as="div">

it still renders

type="button"

which throws off the css styling

@RobinMalfait
Copy link
Member

Hey! Thank you for your bug report!
Much appreciated! 🙏

This should be fixed, and will be available in the next release.

You can already try it using:

  • npm install @headlessui/react@dev or yarn add @headlessui/react@dev.
  • npm install @headlessui/vue@dev or yarn add @headlessui/vue@dev.

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

No branches or pull requests

6 participants