Require id and controls when setting up tabs and tabpanels

[jamesrweb]

Fixes #28.

[tesk9]

Thanks for the PRs! I'll take a look this week 🙏

[jamesrweb]

@tesk9 Just added changes to attempt to autocorrect bad selected state input from users, in case you wondered what the pushes I just made were for. No more to come for this PR, just thought it could be a nice addition for review.

[tesk9]

Thanks for the PR!

If possible, I think it would be nice to avoid a major version bump -- so maybe adding a new helper, rather than changing the existing ones.

Part of why is that I'll be faster about releasing minor and patch changes than I will major versions, so the changes will be available sooner for you.

The other part is that I think it's possible/likely that people are using the various tabs helpers in ways that might be unexpected, like for a Carousel, for instance. If they are, then changing the scope of the helpers available to do more might lead to a less fun upgrade path.

[tesk9]

Hmm, so the tabpanel shouldn't have the tabs as its children.

We want a structure like:

• tablist
  • tab
  • tab
• tabpanel
• tabpanel

There's an example of this structure in the authoring practices guide

[jamesrweb]

Corrected the logic to apply in the tab list rather than the tab panel. That was my bad, I knew about this actually and just brain melted I guess. Thanks for pointing that out!! 😅

[jamesrweb]

I updated the implementation but it is still a breaking change so before you would do something like:

tablist [] [
  tab [] [ Html.text "Tab 1" ],
  tab [] [ Html.text "Tab 2" ],
  tab [] [ Html.text "Tab 3" ]
]

And now you would do:

tablist [] [
  tab { id = "tab-1", controls = "tab-panel-1", selected = True } [] [ Html.text "Tab 1" ],
  tab { id = "tab-2", controls = "tab-panel-2", selected = False } [] [ Html.text "Tab 2" ],
  tab { id = "tab-3", controls = "tab-panel-3", selected = False } [] [ Html.text "Tab 3" ],
]

It is simpler but still allows for implementers to make mistakes like having multiple tabs selected at once for example. WDYT?

[tesk9]

I am liking this much better!

I am a little concerned that the tabs might end up referring to tabpanel ids that aren't included on the page. It would be easy to accidentally do something like:

    div []
        [ tablist []
            [ tab { id = "tab-1", controls = "tab-panel-1", selected = True } [] [ Html.text "Tab 1" ]
            ]
        , tabpanel [] [ text "Some content..." ]
        ]

Especially if you didn't fully understand what the controls field was for. 🤔

[jamesrweb]

That was also something of concern for me but I amn't entirely sure how to relate the two in a simple way because you could have a case like:

div []
        [ tablist []
            [ tab { id = "tab-1", controls = "tab-panel-2", selected = True } [] [ Html.text "Tab 1" ]
            [ tab { id = "tab-2", controls = "tab-panel-1", selected = False } [] [ Html.text "Tab 2" ]
            ]
        , tabpanel [ id "tab-panel-1" ] [ text "Some content..." ]
        , tabpanel [ id "tab-panel-2" ] [ text "Some content..." ]
        ]

In this case the tabs and tab panels don't share the same ordering and as you pointed out, some might be missing correct attributes, etc. Any ideas?

[tesk9]

Interesting! When would you want the tabpanel to be in a different order than the tabs?

[jamesrweb]

Personally I wouldn't but I could imagine it being a case that could happen. Imagine a user looping some settings from an API and rendering the tabs and tab panels but there's some logic where the selected tab should be the first one, that would change the DOM order and it is a case I've seen in the wild previously.

It's probably a very small use case but it is worth thinking about such things.

Still, even for the majority use case I'm not entirely sure how to cleanly marry the tabs and panels with one another without exposing helper functions and relying on users putting things in the right order. For example:

tabLayout: List TabSettings -> List (Html msg) -> List (Html msg) -> Html msg
tabLayout settings tabContents tabPanelContents =
  ...

Note: input types are just labels for the "real" implementation.

This would then loop the settings and recursively render the panels and tabs with the relevant contents, ids and so on but it's still open to potential issues such as ordering issues or typos, etc.

Any ideas on a cleaner implementation or does something like this seem reasonable to you? I'm on the fence.

[tesk9]

I think tabs auto-re-ordering would probably be confusing experience for a screenreader user, so I think it's reasonable to enforce that the tabpanel & tabs be in the same order in the DOM.

At NoRedInk, the API for setting up a tabbed layout is pretty involved (more involved than maybe makes sense for accessible-html, which isn't structured to be a full component library)

A simplified version looks something like this:

type alias Tab id msg =
    { id : id
    , idString : String
    , tabView : List (Html msg)
    , panelView : List (Html msg)
    }

viewTabs : List (Tab id msg) -> id -> Html msg
viewTabs tabs currentlySelectedId = 
    ....

[jamesrweb]

Added and exposed a new viewTabs helper, what do you think?

[jamesrweb]

Can we resolve this thread or is there something to add to this one? 😄

[tesk9]

I think List.map toTab tabs needs to be wrapped with a tablist

[tesk9]

Ideally speaking, I think the tablist should be able to take arbitrary attributes, since I think it's common to do something like this:

<h3 id="tabs-name">Books</h3>
<div role="tablist" aria-labelledby="tabs-name">
    ....
</div>

[jamesrweb]

I updated the function signature for viewTabs to accommodate this idea, WDYT?

[tesk9]

This is looking good!

Just so you're aware, I'm considering removing the Accessibility file from this package. I think there are many valid patterns for a given accessible component, and better APIs are possible when each component has its own file, if not its own package. Also I've been really focused on NoRedInk-specific accessible components, rather than generic accessible components... I feel like I've been neglecting the parts of this package that require more thinking to get right.

Regardless of whether this file continues to exist in the package long-term or not, I'll release a version with the new tab helpers in it. And then if I do remove the file, please feel free to make a package like it.

[jamesrweb]

Oh, I see, so you would keep the Utilities like those for Aria but not the base elements?

Perhaps the package should be renamed if you do that though because accessible-html implies an accessible version of the html package in my view.

Either way, thanks for the heads up. Will you merge this and the other PRs before the next major release then before acting on such a decision?

[tesk9]

@jamesrweb Yes I'll release a new major version with this code in it first!

[jamesrweb]

Once #34 is merged, the conflicts with the latest master should be resolved.

[tesk9]

I think I want to move this library toward just being attribute helpers after all, sorry! I highly encourage you to take the code you wrote and make a tab component package -- I think it'll be easier to manage as an isolated package than it would be as part of accessible-html and accessible-html-with-css.

Sorry again for the incredibly slow response here.