feat(tabs): Introduce Tabs component

[andybraren]

This is an initial implementation of Tabs that resolves #30. The design spec for tabs is available here.

Implementation notes

1. The scrollbar isn't hidden. We'll need to discuss this. (EDIT: We did in Firefox doesn't hide scrollbar styles #859. We're keeping the scrollbar.)

2. The first example includes sample HTML for the sections that the tabs would control with the appropriate role, id, and aria-labelledby attributes. Is this fine? (EDIT: Yes.)

CSS notes

1. The organization of the SCSS file could probably be improved. Tabs, secondary tabs, and the hover/focus/active states for each are all kinda interconnected.

2. Some modifiers like pf-m-tabs-scrollLeftVisible are highly specific to Tabs so I included "tabs" in their name. I’m guessing this should be changed to some existing convention? (EDIT: Per @mcoker's recommendation, they're now .pf-m-start and '.pf-m-end`)

3. Primary tabs need a 3px blue line above them when focused. Removing the existing border-top would make the tab’s text jump, so border-top-color is changed along with adding a 2px box-shadow to achieve the desired effect. It works, but maybe there's a cleaner way? (EDIT: Yes, using ::before and ::after, which is now implemented)

Questions

1. A parent container is necessary whenever secondary tabs are used in order to position the fixed scroll buttons correctly (see the example). Should this parent container become a requirement, even when secondary tabs aren’t used? (EDIT: This component no longer needs a container. Primary and secondary tabs are now independent, and secondary receives extra styling only when directly adjacent to primary.)

2. The hover and focus states weren’t fully specified in the design spec. The contrast of secondary tabs when hovered may not meet WCAG AA requirements. (EDIT: Fixed, now they should.)

3. I’m unsure whether tabs should use the same styling as .pf-m-current when focused/clicked, or if they should continue to look like their hover state until JS finishes loading the tab content and adds pf-m-current. If JS ever fails to load the content the tab shouldn't look active, right? (EDIT: The custom focus state has been removed in favor of the eventual universal focus state.)

Future notes

1. Scroll buttons should probably fade in and out when activated, but because display: none; is needed to guarantee that they don’t appear to screen readers, adding that animation is a bit tricky.

2. Page 2 of the design spec shows the tab separator stretching past the width of the content area while the tabs themselves stay within it. I’m not quite sure how this would be implemented. (EDIT: Probably some negative margin trickery. It'll be weird.)

[patternfly-build]

Deploy preview for pf-next ready!

Built with commit 39e1586

https://deploy-preview-868--pf-next.netlify.com

[mcarrano]

This is looking good @andybraren But there are a few comments I have. First, to address your questions, yes, I agree that the hover state associated with secondary tabs is too light. @mceledonia can you take a look? Good question about whether we should have a "down" state between hover and selection. Do we have this as a distinct visual state for similar components (buttons for example)? I'll defer to Visual Design here.

In comparing the implementation to the visual design, I feel like the border around the tabs is a little heavy. @mceledonia what do you think? Maybe it's just the way the PDF renders but want to make sure the border weight and color is correct.

Finally, I'm thinking that the arrow keys should have a hover as well as a selected state. In general, anything that is clickable should have a hover state.

[mceledonia]

A few hopefully quick visual changes, good eye @mcarrano .

Border color
#EDEDED

Spacers
Above and below the text should be 8px spacers, just double checking that here.

Primary tab hover state
3px tall top-aligned rectangle, #8B8D8F, other borders still stay #EDEDED. I think this only needs the color change.

Secondary tab hover state
I agree this is too light. Let's use our secondary text color, I believe it might be $pf-global--Color--dark-200. It will be a lot more subtle but in use should be a clear change.

Arrow button hover state
Let's reuse the same hover state as primary tab hover state, having that same 3px rectangle appear at the top.

Pressed/Down State
I don't think we have this for other components. It may be something we want to explore, typically you see this as a shade darker than the hover state, or an inner-shadow appearing, etc... Material design has a very cool animated state for this. I don't think this state is crucial but could be nice to have if we want to explore it, though since we don't have it in other components I'm not sure of the best approach for adding it.

[andybraren]

Thank you @mcarrano @mceledonia!

• Border color is now #EDEDED (is there a global variable for that color?)
• Spacers above and below tab text are 8px ($pf-global--spacer--sm)
• Primary tab hover state rectangle is now #8B8D8F
• Secondary tabs are now $pf-global--Color--dark-200 on hover/focus
• Arrows now use the same hover state

The current pressed/down state is the same as the hover state. I can remove this if we want.

I have no idea if this is a possible scenario, but if there's ever a situation where the content of a tab is dependent on JavaScript/React pulling in stuff from elsewhere, some kind of state that indicates that the tab has been clicked but hasn't been populated yet might make sense. My thinking is that if the tab were to immediately switch to the regular unhovered/unclicked state, the user may interpret that as the tab "not working" the way that it should when it's actually just JS being a bit slow. Maybe it's a non-issue.

[mcarrano]

Good point @andybraren . We will probably need some type of general "loading" state that can be applied in places like this. @LHinson do we have this in the backlog? If not I can open a new issue for this.

[LHinson]

@mcarrano we have an issue for skeleton loading #745 and loading guidance #758
I'm not sure if we should address this specifically for tab, rather just as a general rule for loading?

[mcarrano]

I think this is pretty related to #758 @LHinson . Whatever we do there should carry into tabs. A new issue is not needed.

[mcarrano]

Looks good @andybraren . Thanks for making all those changes.

[jgiardino]

The tabs look great!

In your original comment, you included this note:

Scroll buttons should probably fade in and out when activated, but because display: none; is needed to guarantee that they don’t appear to screen readers, adding that animation is a bit tricky.

When testing with JAWS, I have noticed that display:none alone is not an effective way to hide elements from the screen reader. The most reliable way to hide something from screen readers is with the attributes hidden or aria-hidden=“true”. Using just display: none; could result in some odd behavior.

I’m copying the following from the Accessibility chapter in Smashing Book 6, from the section titled “Disabling Background Content”:

Is the inactive content hidden from everyone?

“Use CSS display: none. This turns the display off and disables all focusable items inside. If you have to animate it, you can transform opacity and set the display on the last keyframe of a CSS animation or after a setTimeout in JavaScript.”

Excerpt From: Smashing Magazine. “Smashing Book 6: New Frontiers In Web Design.” iBooks.

I found an example similar to what the author describes in this medium article.

P.S. After skimming through the article I mentioned, they suggest that you could effectively hide elements from screen readers using display:none and visibility: hidden. I can test this combination in JAWS later this week when I’m at home.

Here are some other comments based on review:

1. It isn’t obvious when a selected tab has keyboard focus.
2. The css removes default browser focus styles for tabs, but not for the scroll buttons.
3. The scroll buttons and tabs should be placed in the DOM in the same order that they appear on screen—just in case the screen reader user happens to come across them. Here were comments that I had posted in the original sequence PR:
   • I tested this codepen in JAWS, which is similar to the design we're using. I noticed that the tabs will scroll to the left as JAWS focus moves through the tabs. Unfortunately, the tabs will not scroll to the right as JAWS focus moves through the tabs to the left. And the arrow buttons are visible to the screen reader, but not labeled and not in a DOM order that matches layout. They’re positioned at the end of the tabs, so leaving the last tab, I hear “unlabeled one button” and “unlabeled two button”. They’re not labeled for the screen reader or hidden from the screen reader.
   • My suggestion would be to place the buttons in the DOM to match the order they appear in the layout (e.g. start button before the list items and end button after the list items). This way, if the screen reader user does need them, they're present in a logical order.
4. This is probably a topic for our next a11y discussion, but... My suggestion would be to remove the role attributes from this component. Using the roles associated with tabs would require us to manage focus as described by the authoring practices (managing focus, tabs examples).
   • I'm fine with holding off on changes related to this until we've had a chance to discuss as a team just to confirm what approach we want to take. In a recent meeting, we had discussed our approach to keyboard interaction. My recommendation is that for this initial release, we only focus on enabling the Esc key, and otherwise we rely on keyboard interaction that’s enabled by default (i.e. Tab key to shift focus) instead of managing focus as described in the resources I referenced above. My thinking here is that managing focus in a consistent way across all of our components might be more than we can handle for our initial release, and that this is an enhancement that we should be able to add later to our JS components without resulting in a breaking change.

[LHinson]

@mattnolting will you help review this one as well?

[andybraren]

Thank you for your comments @jgiardino !

0. Let me know if using display:none; and visibility:hidden; works just as well as adding a hidden attribute to the buttons in HTML. I imagine controlling that “automatically” through the simple addition of .pf-m-start or .pf-m-end would be preferred by devs, rather than remembering to add the hidden attribute.
   • I tried to do that keyframe trick but unfortunately couldn’t get the animation to be correct on both fade in and fade out. I’ll try again at a later date.
1. I agree. @mceledonia is our intent still to override the default browser keyboard focus styling with our own? If so, active tabs (like “Tab item 2” at the very top of the preview) need a focus state that doesn’t rely on the top rectangle’s color (since active tabs already have one).
   • If this is a problem we’d prefer to revisit later on, I can re-enable the browser’s default focus style for now.
2. I’ve removed the focus style of scroll buttons for consistency’s sake, but active scroll buttons have the same problem as 1.
3. The DOM now matches the visual order. Sub-bullet 1 is referring to that Pen and not this implementation, right? Our buttons have aria-label, and I don’t have JAWS so I can’t test the scrolling behavior you describe.
4. I’ve removed role=“tab”, role=“tablist”, role=“tabpanel”, and role=“presentation”. I based those attributes on Inclusive Components’ example.

[mcarrano]

I took another look at this. It looks good to me except that didn't we originally have a variant where the width of the tabs scaled to fit their container? I may be mistaken, but just wanted to check @andybraren and @mceledonia .

[andybraren]

@mcarrano "Filled tabs" has been fixed, thank you for spotting that.

[jgiardino]

I was just taking a quick look again at your latest updates, and noticed that you have a <button> element as a child of <ul>, which I think is invalid?

Also, a question that would be good to explore in the future is the relationship of the secondary tabs to the primary tabs. Would/should the secondary tabs be included within the tab panel of the primary tab? I think it's totally fine to leave the secondary tabs example as-is for now, and then later revisit it if we find that the react example is using a different html structure.

Great job on this one!!

[andybraren]

@jgiardino Agreed, this is one of the last things to fix/figure out. I think @matthewcarleton wants to discuss component structuring with the larger group (should the first element always be the top-level component class?) and we could probably discuss secondary tab location as well.

Thanks, and thank you and everyone else who reviewed for helping me fix things. 👍

[mattnolting]

Just a couple more changes and it looks good to me