Use `button` instead of `a` for closing modals

[Atmos4]

Describe the issue

The usage of button for closing modals makes a lot more sense than a.

• from w3:

The button element should be used for any interaction that performs an action on the current page. The a element should be used for any interaction that navigates to another view.

• from UX: Links only respond to Enter key presses. Buttons respond to both Enter and Space.
• Links need to have an href attribute in order to be accessible. This forces the usage of href=#, which can confuse screen readers and will in fact perform the scroll animation unless the onclick handler returns false, which further increases verbosity.

Solutions

One solution would be to recommend button as a way to close modals, and properly style any button with class=close (or rel=prev even though it is not valid on buttons, and also a non-standard way to describe dialog closing actions).

Extending the previous solution for classless pico, we can use <form method=dialog> HTML element, which is a non-Javascript solution to closing dialogs. Alternatively if the button is already in a form, we can use <button formmethod=dialog>.

Here is a demo of the extended solution on CodePen.

If you agree on the issue, let me know which solution you like best and I can start working on a PR!

[lucaslarroche]

@Atmos4,
Thank you. I appreciate your research. I agree. You made a good point.

But we must avoid breaking changes because we are no longer in beta or release-candidate.
So the current <a rel=prev></a> should still be functional.

I would prefer formmethod="dialog" (vs rel="prev").
I didn't know about HTMLDialogElement, so that's even better.

Sure, it would be great if you could open a PR.
Also, as following PRs, we would need to update the website and the examples.
It can come later.

Note, you also need to reset box-shadow

(EDITED)

[Atmos4]

@lucaslarroche
I will work on a PR as soon I have some free time!

Out of curiosity, how much does bundle size matter to this project? If it is important, then I might just go with the first solution I described. It should be doable with :is(button, a):has([rel=prev]).

I edited the codepen to reflect what I described above, and simplify the CSS to work with .close class instead.

[lucaslarroche]

Semantically valid is more important than the bundle size.

:is(button, a):has([rel=prev])

Agreed

(EDITED)

[Atmos4]

PR is ready, see #448

[lucaslarroche]

@Atmos4, I edited my comments about targeting <button formmethod="dialog" />
I was confused with your example using it, but dialog is not a valid formmethod.

So you were right, .close, :is(a, button)[rel=prev] and :is(a, button)[rel=prev] are good!

[Atmos4]

dialog is not a valid formmethod.

@lucaslarroche it is! Check out the MDN article and this codepen. The original codepen I posted above was also containing this solution.

[lucaslarroche]

@lucaslarroche it is! Check out the MDN article and this codepen.

Great. I missed it when researching yesterday.

The original codepen I posted above was also containing this solution.

Yes. That's why I suggested at some point to target button[formmethod] to avoid using rel.

[Atmos4]

@lucaslarroche The problem with using button[formmethod] as the way to style the icon is that there are other valid use cases for button[formmethod] without the icon, for example:

<dialog>
  <article>
    <!-- Modal content -->
    <footer>
      <form>
        <button formmethod=dialog>Cancel</button> <!-- this button should look normal! -->
      </form>
    </footer>
  </article>
</dialog>

By the way, is there a reason to have some part of the icon specific code coupled to the header? It could be interesting to allow usage of that icon anywhere inside the article, for example like this:

<dialog id=modal1>
  <article>
     <button rel=prev onclick="modal1.close()"></button> <!-- currently doesn't float:right -->
    <!-- Modal content -->
  </article>
</dialog>

I can create an issue/PR to decouple the code if you are interested. Maybe there are valid reasons to have it otherwise though!

[micchickenburger]

I think there are many problems with using rel=prev. Besides being an invalid attribute, the property can't be set when interacting with the DOM. For instance:

const closeButton = document.createElement('button');
closeButton.ariaLabel = 'Close';
closeButton.rel = 'prev'; // pico uses this attribute, even though it's invalid

Typescript will throw an error: Property 'rel' does not exist on type 'HTMLButtonElement'. ts(2339).

Furthermore, executing the code in the browser (at least in Safari) will not set the attribute in the DOM, making for a funny dialog:

I think whatever solution is chosen should be compatible with the specification, else issues will arise for those of us who generate code from scripts.

Thanks for this awesome project!!

[Atmos4]

@micchickenburger thank you for your interest in the project.

I think you are confused between DOM properties and HTML attributes. Technically you can set any attribute on any HTML element using the setAttribute method. DOM properties work very differently, so I suggest you read up on the subject.

As of the Safari issue, it works on my iOS Safari. Can you send a link to a CopePen / Stackblitz?