javierriveracastro / betteroll-swade

A Better Rolls port for SWADE
GNU General Public License v3.0
15 stars 31 forks source link

Most/all of the entries in brsw-tw-styles.css are unscoped which can stomp over CSS from other modules (and is) #677

Closed ddbrown30 closed 3 months ago

ddbrown30 commented 4 months ago

While looking into the Item Piles module, I noticed that the UI was super broken. I used find the culprit to narrow it down to BR2 being the problem and then went digging. The specific issue is that .flex is unscoped meaning it will stomp over the CSS of other modules. Looking through the css file, it looks like everything in there is unscoped. You'll need to do a pass on all the CSS in that file, add scopes, and update all the places that use it.

I'm probably going to take a quick stab at fixing the .flex one so that it will fix the issue with Item Piles but I'll leave it to you to do the rest.

For the record, this is stuff that I know nothing about. I posted this on the module-development channel to get help and that was the response.

javierriveracastro commented 4 months ago

As talked on the PR it is an autogenerate file. Scoping it could be possible, but also, given how much verbose is (by design) TailWind it will be a nightmare.

Not easy to fix. I'll take a look but I don't expect to be able to fix this.

BTW, if the other modules are scoped this could be avoided ;P.

javierriveracastro commented 4 months ago

It looks like it could be mostly done using the tailwind prefix option

javierriveracastro commented 4 months ago

And I think we need a CONTRIBUTING.md file, but this is probably another issue.

ddbrown30 commented 4 months ago

BTW, if the other modules are scoped this could be avoided ;P.

Just FYI, that's not true. Selectors are what need to be scoped, not the html. Take this example:

<div class="someModule">
    <div class="flex">
    </div>
</div>

.someModule .flex {
}

This module is properly scoped and their CSS will only affect stuff that is in someModule flex.

Now BR2 comes along with:

.flex {
}

It sees that div block above with flex and goes, great, I see flex, that means I need to apply to this block. This is why unscoped CSS selectors are a problem.

javierriveracastro commented 4 months ago

Generally speaking (not Foundry speaking), scoping css usually means adding a preffix (i.e. brws-flex.

And that is what TailWind can do.

ddbrown30 commented 4 months ago

Yeah, but that prefix is added to your selector which is what I'm getting at. I'm just trying to make sure that you understand that this isn't a problem with other modules having built their stuff incorrectly but with BR having an unscoped selector that uses a very common class name. In other words, the problem isn't that other modules are using "flex" as a class but rather that BR has a selector that is just searching for "flex" without scoping it within another class.

No shade here, by the way. I definitely could have made the same mistake like a week ago. This is just stuff I'm learning that I want to pass onto you so you don't make the same mistake in the future.

javierriveracastro commented 4 months ago

Yes I understand you. But I have never, ever (and I'm been in programing for quite a lot of years) have understood CSS as that.

The common way of scoping is using a class named brws-flex instead of flex. not surrounding it with another class. So I misunderstood you.

Anyway, AFAIK, TailWind doesn't support your idea, not do any other css framework that I know off.

ddbrown30 commented 4 months ago

Ah, okay. Makes sense then. :)

javierriveracastro commented 4 months ago

BTW, there could be a way to wrap everything in one class:

https://tailwindcss.com/docs/configuration#selector-strategy

It is a worse option than using a preffix, but way easier to implement as your strategy of just wrapping everything in a class will work (mostly). It will avoid changing other module CSSs, but will keep us vulnerable to other modules using more specific selectors.

ddbrown30 commented 4 months ago

Yeah, no idea if that's better or worse. As you said, seems like it would be easier to make the fixes but otherwise I have no input. I guess the question is do you want to do a bit more work now to be more future proof or do the easier thing that will probably also be fine but might have an issue later?

javierriveracastro commented 3 months ago

This should be done on the css-scoping brach. Take a look if you want. I'll do some more testing and merge it on Monday.