dillonkearns / elm-pages-starter

Starter blog for elm-pages
https://elm-pages-starter.netlify.com
95 stars 41 forks source link

Accessibility: Add focus styles #3

Closed fpapado closed 3 years ago

fpapado commented 5 years ago

Hello @dillonkearns!

Here with the PR for focus styles, as promised :) (Closes #2 )

It is a small PR code-wise, but I think the conceptual parts are larger.

What I'm aiming for is to enable people to adjust the styles to their application, without nuking them altogether. To that end, I left a few comments in the code and tried to centralise them in Palette. I went with the blue colour already present. Blue works quite well for contrast (part of why it's the browser default). I also went with box-shadows, because they follow borders, such as in rounded buttons.

The elm-ui APIs for focus styles are a bit rough in my experience. The layoutWith options only apply to buttons, but not to links. I've had to do this same exercise at a work project, and there were always a couple of places to add focus styles. That's the main reason for containing them in Palette.

About Markdown Links

There is one part that I'm a bit stuck (not a blocker necessarily).

In the starter, links in Markdown have the default browser link styles, and I do not understand how that's possible! (Running locally at http://localhost:3000/blog/hello).

Meanwhile, in the elm-pages demo site (at https://elm-pages.com/blog/introducing-elm-pages), the links inside Markdown do not have link styles at all.

This to me hints that the former are not under elm-ui, but the latter somehow are? Could you point me to the part of the code that handles that? I think that's the final place where we can have the "good by default" behaviour, or to add comments etc.

Aside about elm-ui

I'm adding a couple of thoughts about elm-ui for accessibility here. I'm doing it mostly because this is a good example of a large project using it, and it can showcase the API decisions and implications for accessibility.

If you have a contact with Matthew, I'd love to bring them up with him. I have extremely limited time and energy, and the impression I have so far is that much of elm-ui is focused on perf/V2. I'll probably post these to the elm-ui tracker if they look good to you.

1) Focus options

In Windows High-Contrast Mode (WHCM), used approximately by 4% of folks on Windows, box-shadows are hidden. This is by design. In those cases, preserving an outline (even transparent) will mean that it is visible in WHCM. This is something that should change at the elm-ui level, to either allow outlines, or include a transparent one by default.

2) Opt-in vs opt-out

Something we mentioned on Twitter, would be to have the default browser focus styles if not otherwise specified. I am skeptical of people opting-in to them, and removing them without prompting the developers to replace them is a bit scary.

Similarly, something that came up would be a focus style constructor for browser defaults, such as Element.defaultFocusStyle .

3) layoutWith

Something I'm wondering is why layoutWith does not apply focus to links. I would guess that they can always be overridden with Element.focusStyle. Is it an omission? Something that was considered? I'm not sure! I think it would make things easier for rendering links in Markdown, where specifying focus styles ad-hoc is hard/impossible.

Wrapping up

Thank you for your time and willingness to integrate these!

dillonkearns commented 5 years ago

Hello @fpapado,

Thank you for taking the time to make this PR, that's awesome!

One thing I notice is that if you tab through the links, we can get multiple links highlighted (you can see it live at the deploy preview):

Image 2019-10-02 at 9 55 30 AM

I think this is related to this issue:

https://github.com/mdgriffith/elm-ui/issues/47#issuecomment-443216823

So it looks like we need to do a layoutWith and add a focus style?

dillonkearns commented 5 years ago

This to me hints that the former are not under elm-ui, but the latter somehow are? Could you point me to the part of the code that handles that? I think that's the final place where we can have the "good by default" behaviour, or to add comments etc.

Yeah, so the markdown in this starter is currently being rendered with elm-explorations/elm-markdown, which does its own thing and renders into plain Html (I'm not sure if that answers your question?).

fpapado commented 5 years ago

Ah, I thought the multi-focus issue was fixed. We already use layoutWith in this PR, but the workaround seems to only work for buttons, not links. The focus happens because of a general sibling combinator (~) in the elm-ui styles. I'm not sure what it's used for, I most often see it for custom checkboxes.

Anyway, we can wrap the links in single elements, to prevent that. I'll try it out in this PR.

fpapado commented 5 years ago

That's exactly what I was wondering for Markdown. I then found this line in the examples repo, with the custom rendering https://github.com/dillonkearns/elm-pages/blob/master/examples/docs/src/MarkdownRenderer.elm#L90.

Two more questions with that in mind:

fpapado commented 4 years ago

Hi @dillonkearns, is there anything else I could do with this PR? :)

dillonkearns commented 4 years ago

Hello @fpapado! The links are looking good in the deploy preview now.

Oh, and by the way, to your question, styles can be added by doing an import statement from index.js. There's not a CSS file in this starter repo because ideally it can be done in pure Elm. But it might be worth considering adding something in case users want to add global styles 🤔 I'm on the fence about that.

Regarding this change, it would be ideal if it wasn't necessary to use a helper function when adding links (as you said in your comment here). I'm okay with merging this for now, but it would be nice to revisit this when that bug gets fixed in elm-ui.

Would you mind adjusting the Palette.link function to take a Pages.PagePath Pages.PathKey instead of a a String for the url? That would make it a little easier to use I think. What do you think?

dillonkearns commented 3 years ago

Hey @fpapado, since https://github.com/mdgriffith/elm-ui/issues/47 was fixed upstream it looks like this should be resolved now. Thank you!