segmentio / evergreen

🌲 Evergreen React UI Framework by Segment
https://evergreen.segment.com
MIT License
12.38k stars 830 forks source link

Style of selected tab differs between tab link and basic tab #1622

Closed limgit closed 1 year ago

limgit commented 1 year ago

When I use appearance="primary" for tab, style of the selected tab differs between tab link and basic tab.

Is it by design or a bug?

I examined the stylesheet and it seems to be a bug since the css selector looks not correct:

.ub-tfrm_qu4iyp_1d0tz5k[aria-current="page"], .ub-tfrm_qu4iyp_1d0tz5k[aria-selected="true"]:before {
    transform: scaleY(1);
}

I think it should be like this:

.ub-tfrm_qu4iyp_1d0tz5k[aria-current="page"]:before, .ub-tfrm_qu4iyp_1d0tz5k[aria-selected="true"]:before {
    transform: scaleY(1);
}

or at least, the selected tab link should have aria-selected="true" attribute.

Rendered Example

image

Code

import React from "react";
import {
  Pane,
  Tablist,
  TabNavigation,
  Paragraph,
  Tab,
  Heading,
} from "evergreen-ui";

/** Example copied from https://evergreen.segment.com/components/tabs#accessibility and modified to use appearance="primary" on Tab component */
function BasicTabsExample() {
  const [selectedIndex, setSelectedIndex] = React.useState(0);
  const [tabs] = React.useState(["Traits", "Event History", "Identities"]);
  return (
    <Pane height={120}>
      <Tablist marginBottom={16} flexBasis={240} marginRight={24}>
        {tabs.map((tab, index) => (
          <Tab
            aria-controls={`panel-${tab}`}
            isSelected={index === selectedIndex}
            key={tab}
            appearance="primary"
            onSelect={() => setSelectedIndex(index)}
          >
            {tab}
          </Tab>
        ))}
      </Tablist>
      <Pane padding={16} background="tint1" flex="1">
        {tabs.map((tab, index) => (
          <Pane
            aria-labelledby={tab}
            aria-hidden={index !== selectedIndex}
            display={index === selectedIndex ? "block" : "none"}
            key={tab}
            role="tabpanel"
          >
            <Paragraph>Panel {tab}</Paragraph>
          </Pane>
        ))}
      </Pane>
    </Pane>
  );
}

/** Example copied from https://evergreen.segment.com/components/tabs#tab_link_usage and modified to use appearance="primary" on Tab component */
function TabLinkExample() {
  const tabs = React.useMemo(
    () => ["Traits", "Event History", "Identities"],
    [],
  );
  // State is only used here for demo purposes, your routing abstraction might support hash links
  const [selectedIndex, setSelectedIndex] = React.useState(0);

  return (
    <TabNavigation>
      {tabs.map((tab, index) => {
        const id = tab.toLowerCase().replace(" ", "-");
        const hash = `#${id}`;
        return (
          <Tab
            href={hash}
            is="a"
            isSelected={selectedIndex === index}
            appearance="primary"
            key={tab}
            onClick={() => setSelectedIndex(index)}
          >
            {tab}
          </Tab>
        );
      })}
    </TabNavigation>
  );
}

function App() {
  return (
    <Pane margin={16}>
      <Pane marginBottom={8}>
        <Heading marginBottom={8}>Basic Tab</Heading>
        <BasicTabsExample />
      </Pane>
      <Pane marginBottom={8}>
        <Heading marginBottom={8}>Tab Link</Heading>
        <TabLinkExample />
      </Pane>
    </Pane>
  );
}

export default App;

Package version

    "evergreen-ui": "^7.1.4",
    "react": "^18.1.0",
    "react-dom": "^18.1.0"
brandongregoryscott commented 1 year ago

Hmm, this looks like a regression from the V7 changes. The same code looks fine in v6.13.3: https://codesandbox.io/s/issue-1622-selected-tab-doc3fp

It looks like using the Link component instead of an a tag directly should also style correctly, as a workaround on v7:

<Tab
  href={hash}
  is={Link}
  isSelected={selectedIndex === index}
  appearance="primary"
  key={tab}
  onClick={() => setSelectedIndex(index)}
>
  {tab}
</Tab>
limgit commented 1 year ago

Thanks for the reply! I'll use the workaround until a fix is applied.

Edit: FYI for future readers: I added aria-selected={selectedIndex === index} prop to the Tab component (along with isSelected={selectedIndex === index}) for workaround(Using Link component styles the text little bit differently, which is not what I wanted).

brandongregoryscott commented 1 year ago

I do think you're on to something with the incorrect selectors being generated - I think that's an edge case in the newer ui-box features that wasn't handled properly. The selector for _current maps to this: https://github.com/segmentio/evergreen/blob/e13b9b4f2609ed3ecb5f7dc218dfe4fe1a1da048/src/tabs/src/Tab.js#L27

The nested selectors defined in the theme (i.e. :before and :after) should be attached to both of these:

https://github.com/segmentio/evergreen/blob/e13b9b4f2609ed3ecb5f7dc218dfe4fe1a1da048/src/themes/default/components/tab.js#L45-L51

However, these are only being attached to the last of the parent selectors, as you noted:

-.ub-tfrm_qu4iyp_fiauus[aria-current="page"], .ub-tfrm_qu4iyp_fiauus[aria-selected="true"]:before {
+.ub-tfrm_qu4iyp_fiauus[aria-current="page"]:before, .ub-tfrm_qu4iyp_fiauus[aria-selected="true"]:before {
  transform: scaleY(1);
}

I should have a PR up there to fix it soon, and then we can bring it in here. It seems to solve the issue without having to change the existing tab.js theme file.

image

Once this lands, I don't think you should need the aria-selected={selectedIndex === index} prop anymore (since it sets aria-current="page" internally if isSelected is true).