Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to list multiple variants in arbitrary variant #10576

Closed
brandonmcconnell opened this issue Feb 14, 2023 · 3 comments · Fixed by #10655
Closed

Unable to list multiple variants in arbitrary variant #10576

brandonmcconnell opened this issue Feb 14, 2023 · 3 comments · Fixed by #10655
Assignees

Comments

@brandonmcconnell
Copy link
Contributor

What version of Tailwind CSS are you using?

v3.2.6

What build tool (or framework if it abstracts the build tool) are you using?

postcss-cli v8.4.19

What version of Node.js are you using?

v18.12.1

What browser are you using?

Chrome v109

What operating system are you using?

macOS Ventura

Reproduction URL

https://play.tailwindcss.com/7TUEdEbhJH

Describe your issue

I'd expect to be able to do something like [span,h1,&_h1]:font-bold using an arbitrary variant, but that doesn't appear to work.

To better demonstrate the issue, I'll compare the CSS generated by the above utility vs. what I think would be expected:

Expected:

span,
.\[span\,\&_h1\]\:font-bold h1 {
  font-weight: 700;
}

Reality:

span {
  font-weight: 700;
}

This is inconvenient for a couple of reasons:

  • This adds a global style that matches all spans on the page, not just those matching the class
  • The h1 in the arbitrary variant gets completely ignored
@adamwathan
Copy link
Member

Hey! So in your expected CSS the span would match all spans on the page too:

span,
.\[span\,\&_h1\]\:font-bold h1 {
  font-weight: 700;
}

That's the same as this:

span {
 font-weight: 700;
}
.\[span\,\&_h1\]\:font-bold h1 {
  font-weight: 700;
}

I'm not really sure what the expected output should be here personally, I kinda just don't think anyone should do this 😅

What do you actually want to achieve here?

I think there's a bug here in that the span style is being generated but will have to think through exactly what we even want to happen here instead. Generally with Tailwind stuff it's a bit unconventional to ever write CSS rules that have multiple selectors so I'm inclined to just discard these classes to avoid the surprising output in the short term.

@brandonmcconnell
Copy link
Contributor Author

brandonmcconnell commented Feb 14, 2023

@adamwathan My use of the span was really only to demonstrate that everything after the span seems to get ignored. I'm not seeing .\[span\,\&_h1\]\:font-bold h1 get generated, and that's all I was really looking for. it seems to skip that part completely, so if I do this…

<h1 class="[span,h1]:font-bold">Test</h1>
<div class="[span,&_h1]:font-bold"><h1>Test</h1></div>

Neither h1 gets matched when both should be AFAIK, though I could be mistaken about how arbitrary variants work.

My specific use case here is essentially— match the element that this utility is injected onto if it is an h1, and/or match any h1 elements under it. Sometimes, this utility is used on an h1 directly; other times, it is used on a header element that contains an h1.


If multiple selectors aren't natively supported yet but you think they'd be useful, I'd be happy to whip up/collab on a PR to get that added. My guess is it'd likely require a relatively lightweight adjustment to the applyVariant function in src/lib/generateRules.js to recognize when there are multiple "unwrapped" commas to split/delimit by.

In other words…

  • test would NOT match for multiple 👎🏼
  • &[attr="1,2,3"] would NOT match for multiple 👎🏼
  • &:is(test,other) would NOT match for multiple 👎🏼
  • test,other WOULD match for multiple 👍🏼 ✅

I already have a little helper function that can effectively do the splitting for only commas that appear in a string and are not wrapped in parentheses, brackets, braces, etc.:

function splitUnwrappedCommas(str) {
  const wrapperPairs = { '(' : ')', '[' : ']', '{' : '}' };
  const stack = [];
  const result = [];
  let current = '';
  for (const c of str) {
    if (c in wrapperPairs) {
      current += c;
      stack.push(c);
    } else if (Object.values(wrapperPairs).includes(c)) {
      if (stack.length === 0) {
        throw new Error(`Unexpected closing bracket: ${c}`);
      }
      const last = stack.pop();
      if (last in wrapperPairs && c !== wrapperPairs[last]) {
        throw new Error(`Mismatched brackets: ${last} and ${c}`);
      }
      current += c;
    } else if (c === ',' && stack.length === 0) {
      result.push(current);
      current = '';
    } else {
      current += c;
    }
  }
  if (stack.length > 0) {
    throw new Error(`Unclosed brackets: ${stack.join('')}`);
  }
  if (current !== '') {
    result.push(current.trim());
  }
  return result;
};

Using this function:

splitUnwrappedCommas('test')             -> (length: 1) [ 'test' ]
splitUnwrappedCommas('&[attr="1,2,3"]')  -> (length: 1) [ '&[attr="1,2,3"]' ]
splitUnwrappedCommas('&:is(test,other)') -> (length: 1) [ '&:is(test,other)' ]
splitUnwrappedCommas('test,other')       -> (length: 2) [ 'test', 'other' ]

After that, each item in the arbitrary variant array(s) can be iterated over, with an arbitrary variant generated for each.

In the case I mentioned earlier, that could look like this:

// variant = '[span,h1,&_h1]'

// Register arbitrary variants
if (isArbitraryValue(variant) && !context.variantMap.has(variant)) {
  let selectors = splitUnwrappedCommas(normalize(variant.slice(1, -1))) // [ 'span', 'h1', '&_h1' ]

  if (!selectors.every(isValidVariantFormatString)) {
    return []
  }

  let fns = selectors.map(parseVariant)

  let sort = context.offsets.recordVariant(variant)

  context.variantMap.set(variant, [[sort, fns]])
}

I'm sure I'm probably oversimplifying the example here, maybe even grossly, but that's the general idea. ☝🏼

@thecrypticace
Copy link
Contributor

Hey, we talked this through and the gist is:

  1. Arbitrary variants were not intended to support multiple selectors and this generating anything at all is a bug. In some cases trying to use multiple selectors would already fail. We suggest that instead of writing [&_h1,&_span]:underline that you write it as two separate classes: [&_h1]:underline [&_span]:underline which will achieve what you want.
  2. The selector you give to an arbitrary variant must include the & so we know where to put the candidate selector. Otherwise we could end up targeting all of a specific element, class, etc… on a page regardless of whether or not a utility is used. As such, even if we did support multiple selectors, every individual selector would require the presence of an & to be valid.

I've merged in #10655 that ensures we don't generate any rules in this case. Thanks for reporting this! ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants