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

Separate two different button resets to avoid too much specificity #8704

Closed

Conversation

SupergreatAndrew
Copy link

  • Taking a look at https://github.com/sindresorhus/modern-normalize/blob/main/modern-normalize.css#L188, I think we should separate the style reset from the iOS Safari reset. I don't think we need to target the style reset of background-image and background-color to [type=] as well as button.
  • This will make it so using tailwind with other libraries is a bit simpler than before, since the specificity of [type=X] is fairly high compared to other selectors.

I wanted to start a discussion around possibly separating these two resets / normalize actions.

Based on what I see in the original modern-normalize, I'm thinking we only need to target button for the removal of default button styles.

NOTE: The snapshot test is failing on this branch, so if we want this change I'd need some help on how to update it.

 - Taking a look at https://github.com/sindresorhus/modern-normalize/blob/main/modern-normalize.css#L188, I think we should separate the style reset from the iOS Safari reset.  I don't think we need to target the style reset of background-image and background-color to [type=] as well as button.
 - This will make it so using tailwind with other libraries is a bit simpler than before, since the specificity  of [type=X] is fairly high compared to other selectors.
@adamwathan
Copy link
Member

Hey! So the reason we made this change in v3 originally is that without it, <button> and <input type="submit"> have different default styles which seems undesirable to me.

I wrote up a big explanation of the recommended way to deal with any issues that stem from this here a while back:

#6602 (comment)

My fear is that making any changes here is just going to lead to more people configuring their CSS incorrectly and not putting things in the right order, which can lead to other types of style collision problems down the road. I'd rather people just learn the right way to configure this stuff personally.

Do you have any examples of a common situation this would resolve that isn't better handled with the style ordering solution I outlined in that comment?

 - Thinking about the reasoning behind the update for v3, I think this would work well for keeping styling the same between these two things (inputs of certain types and buttons), while at the same time not applying more specificity to buttons than intended (and accidentally targeting buttons since they overlap in usage of the type attribute).
@SupergreatAndrew
Copy link
Author

Thanks for the detailed reply. In my scenario (MUI + tailwind), I'm not able to control the ordering so finely (not due to tailwind, this is on how MUI works at the moment, only allowing injectFirst or injectLast). I could look at contributing more fine grained approaches to MUI's styling engine.

I thought about what you said regarding inputs, and I agree (I actually had no idea those types existed on inputs, 🤯 ). I'm wondering though if we can accomplish the styling we want and also keep buttons from being targeted too aggressively. So I pushed an update that shows targeting inputs in particular instead of buttons.

Would this be alright? I tested it for my MUI + tailwind scenario and it fixes things there, and I think it keeps the styling we want for the base in Tailwind.

@adamwathan
Copy link
Member

The problem with that change you've made there is that selectors like input[type='button'] have an 011 specificity, which means adding classes like bg-black would stop working on those elements :(

One option is to use :where since it has zero specificity, but the browser support might not be quite where it should be to be suitable for core.

I'm really hesitant to start making changes to Preflight just to accommodate other third-party CSS because there's just no way to make it impossible to ever have conflicts. Even if we "fix" this problem there's just going to be another one with a different library down the road, and like I mentioned the correct solution to this stuff is just to order your CSS more intentionally. When you're combining CSS for different places there's just opportunities for conflict and it's tough to really do much about.

I'll think more about if we want to solve this like this or not:

button,
input:where([type='button']),
input:where([type='reset']),
input:where([type='submit']) {
  -webkit-appearance: button; /* 1 */
  background-color: transparent; /* 2 */
  background-image: none; /* 2 */
}

Just would like to avoid MUI-specific CSS fixes in Tailwind when it's an unrelated project that I don't think I would actually recommend using in conjunction with Tailwind in the first place 😕

@SupergreatAndrew
Copy link
Author

I should've figured you've thought about this way more than I have 😂 I appreciate you explaining it to me.

Yeah, I agree, it's not a big deal for me to fix it in our project (I have a custom preflight with that part commented out). It's way too hard to get it working for every scenario under the sun.

By the way, MUI + Tailwind works great, it's weird how many folks I run into who think that's a strange combo. I work fast and consistently under it, so thanks for the great work on Tailwind!

That's an interesting thought with the :where, I had no idea that was a thing. I'll close this for now, thanks again!

@SupergreatAndrew
Copy link
Author

SupergreatAndrew commented Jul 4, 2022

EDIT: DON'T DO THIS WITH NEXTJS, IN PRODUCTION IT CONCATENATES ALL THE CSS SO THIS METHOD IS NO GOOD 😭

So my curiosity got the better of me around if MUI was simply being stingy with what props they expose on their styled engine.

Ultimately I was able to use their underlying CacheProvider instead to access a different prop from injectFirst called insertionPoint.

For any curious folks who'd like to do the same so they don't have to customize preflight, here's what is working currently:

import React, { useEffect } from "react";

import "tailwindcss/lib/css/preflight.css";  // disable this in the tailwind config so you can control order of sheets
import "../styles/index.css";  // this is the rest of the tailwind styles, base etc.
import Theme from "../components/Theme";
import { ThemeProvider } from "@mui/material";

import { CacheProvider } from "@emotion/react";
import createCache from "@emotion/cache";

function CustomStyledEngineProvider(props) {
  const [cache, setCache] = React.useState(createCache({ key: "css" }));
  const { injectFirst, children } = props;
  React.useEffect(() => {
    if (document) {
      const newCache = createCache({
        key: "css",
        insertionPoint: document.querySelector("style"), // notice I import preflight first, then the index which is where I import tailwind base etc.  This puts MUI inbetween those two.
      });
      setCache(newCache);
    }
  }, []);
  return injectFirst ? <CacheProvider value={cache}>{children}</CacheProvider> : children;
}


const App = ({ Component, pageProps }: AppProps) => {
  return (
    <CustomStyledEngineProvider>
      <ThemeProvider theme={Theme}>
             <YourApp />
      </ThemeProvider>
    </CustomStyledEngineProvider>
  );
};

export default App;

Which results in the css order:

 // tailwind preflight
 // MUI styles
 // tailwind

This is a bit of a pick your poison scenario, I wouldn't say it's much better than having a custom preflight since it's a bit more complicated looking. But I thought I'd drop it here in case anyone wanted to use it.

@wanjas
Copy link

wanjas commented Jul 5, 2022

Same situation: MUI + Tailwind. (I just migrated from 2 to 3 and bumped into this issue)

Probably I'll disable preflight and copy-paste preflight.css without these two lines or with :where to fix this.

IMHO: Buttons with the reset even without forced transparent background aren't looking like buttons already and require proper custom styling anyway. So having transparent background doesn't make styling easier or default look better. I personally don't see a benefit in these two lines.

Update: Ant.Design + Tailwind is broken by default too.

P.s. Ant.Design has been requiring some minor "conflicts" resolution previously too.

@SupergreatAndrew
Copy link
Author

@wanjas the other thing you can try is to use the CSSBaseline in MUI instead of preflight from Tailwind. I might try switching to that when I have some time.

I realized the order CSS appropriately strategy is easier said than done when involving CSS in JS. The solution I posted above doesn't work in production for us because Nextjs concatenates all the css files into one, so it loses the ordering of having MUI in the middle.

Ultimately I don't think this is on Tailwind, I think we're both off the "expected" path of usage so I don't think it needs to be "fixed" (if you can even call it broken, which I don't think it is). It's hard to get this working automatically for everyone when the ecosystem of combinations is so diverse and complex.

And in spite of that, I think we have a lot of good options (not using preflight, customizing it, or figuring out how to order stylesheets correctly).

I'm convinced that correct ordering of stylesheets is the right answer here, so I might bug the Nextjs folks about how getting separate stylesheets in production could be done. My google attempts have been fruitless.

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 this pull request may close these issues.

3 participants