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

Remove [dir] selectors #64

Open
LFFATE opened this issue Apr 1, 2020 · 30 comments
Open

Remove [dir] selectors #64

LFFATE opened this issue Apr 1, 2020 · 30 comments

Comments

@LFFATE
Copy link

LFFATE commented Apr 1, 2020

Hello everyone! Thank for your library, but i can't it use for now 😟
For what purpose some styles transformed to selectors with leading [dir] or [dir=ltr]?
Is it possible to leave original style with appended rtl rules?

Because there is some issues, for example, i'm using MUI with build-in jss styles and rtl.
So, also I use some styles from Bootstrap and at my css there is structure like this:
bootstrap.css

fieldset {
  border: 0
}

mui styles

.Mui-some-class-for-fieldset {
  border: 1
}

so by css rules - last with more weight. Class has more weight than tag selector. All is ok aaand
after postcss-rtl I have:

[dir] fieldset {...}
and .Mui-some-class-for-fieldset {}

So it breakes everything because now [dir] fieldset has more weight than mui selector. How a developer can predict all of this during develop?

I think it's not good to reassign selector`s weights for default direction.

@evilaliv3
Copy link

evilaliv3 commented Apr 1, 2020

In the GlobaLeaks project actually we have the same requirement. Is there an option for making this?

This is actually very useful to simplify customizations over the generated library.

In fact suppose that a project is using a certain library (e.g. boostrap.css) and have already a customization well writtend to support rtl (e.g. customization-over-bootstra-with-rtl-support.css).
After using the current version postcss-rtl probably the customization wont work anymore because you will have added [dir] in front of every css directive. With the suggestion by @LFFATE instead the customization would likely continue to work. What do you think?

@LFFATE
Copy link
Author

LFFATE commented Apr 2, 2020

@evilaliv3
There is the option:

addPrefixToSelector: Custom function for adding prefix to selector. Optional. Example:

function addPrefixToSelector ( selector, prefix ) {
return ${prefix} > ${selector} // Make selectors like [dir=rtl] > .selector
}

with the notice:

note: the returned string must include prefix to avoid an infinite recursion

So I ignored it and my quick workaround:

...
       use: [
          {
            loader: 'postcss-loader',
            options: {
              plugins: function () {
                return [
                  require('postcss-rtl')(
                    {
                      addPrefixToSelector: function addPrefixToSelector ( selector, prefix ) {
                        if (prefix === '[dir]' || prefix === '[dir=ltr]') {
                          return `${selector}`
                        }

                        return `${prefix} ${selector}`
                      }
                    }
                  )
                ]
              }
            }
          }

And as a side effect I got some styles duplicated. Something like

.selector {
  padding-left: 2rem;
  padding-left: 2rem;
}

But looks like it works in general

@evilaliv3
Copy link

evilaliv3 commented Apr 2, 2020

Thats nice @LFFATE thank you.

@vkalinichev do you have any suggestion on how we can improve this result removing this duplication effect? Do you consider this the only solution or do you have any other good solution that could work in our case? Thank you!

@evilaliv3
Copy link

evilaliv3 commented Apr 3, 2020

@LFFATE: By any chance, have you found any alternative?

@LFFATE
Copy link
Author

LFFATE commented Apr 7, 2020

@evilaliv3 no, I'll try to wait for answer from @vkalinichev

@evilaliv3
Copy link

I see, thank you anyway

@PaquitoSoft
Copy link

We're facing this same issue and the @LFFATE alternative is a way to go but duplicate rules is a little problem for us.
Hope to see any update regarding a more performant solution.

@evilaliv3
Copy link

@vkalinichev : what do you think?

@fabercancio
Copy link

fabercancio commented May 6, 2020

@LFFATE,

As a workaround, you can use the rtl:ignore directive to ignore a single rule or to ignore multiple of them.

Your workaround of creating an override of the main rule can create issues like this one (In rtl, there will be left and right properties at the same time). Or this one (in ltr the border-leftproperty is overridden).

@evilaliv3
Copy link

Thank you @fabercancio , but actually this wasnt the main goal of the ticket.

The main need is to make it possible to add only rules related to RTL without changing any existing CSS.
This in fact complicate the whole CSS in many situations like the one exposed in #64 (comment)

Do you have any advice on this?

@fabercancio
Copy link

fabercancio commented May 6, 2020

@evilaliv3 I know that this is not the purpose of the ticket, I brought just a workaround to avoid adding [rtl] for certain rules. I think that it is possible to do what you are proposing, the only thing is that it has some pitfalls like the ones that I described or some examples that can be found on the section Override CSS on the rtlcss library.

@evilaliv3
Copy link

I see, thank you for the clarification. I guess we are stuck waiting for the suggestions by Vladimir Kalinichev and the evaluation for the possibility to make a patch to the library.

In my opinion the library could assume that the default is RTL and dont ass [rtl] to any of the rtl roules. @vkalinichev do you thins this could be done? even just a config flag for setting the default and apply this change if the user has specified it would be great!

@dzearing
Copy link

dzearing commented May 13, 2020

FWIW just to add to this issue; we've been evaluating this plugin and we're blocked on using this as well. For us it creates large bundle regressions. Most css has very few asymmetrical scenarios, but has plenty of symmetrical cases:

.r { margin: 0; }
.r { border: 1px solid color; }
.r { left: 0; right: 0; }

None of these should be affected as they're all symmetrical, but the plugin adds a bunch of bloat to the output unnecessarily. I took a look at the tests and see that it's validating that the [dir] prefix is added to directionally sensitive things.

Is there any reasoning for adding the dir prefix and modifying symmetrical rules at all? I'm assuming it's a specificity related thing like make all directional rules at least 2 levels of specificity so that they can be predictably overridden, maybe?

The workaround mentioned above would work, but the duplication fights against bundle size as well.

Could the [dir] prefixes be removed? Accepting PRs?

@evilaliv3
Copy link

I thinks so @dzearing and i may support with retesting;

I'm experienced with testing and packaging, in the meantime we could eventually fork the project while the patch gets officially loaded in.

@LFFATE
Copy link
Author

LFFATE commented May 14, 2020

FWIW just to add to this issue; we've been evaluating this plugin and we're blocked on using this as well. For us it creates large bundle regressions. Most css has very few asymmetrical scenarios, but has plenty of symmetrical cases:

.r { margin: 0; }
.r { border: 1px solid color; }
.r { left: 0; right: 0; }

None of these should be affected as they're all symmetrical, but the plugin adds a bunch of bloat to the output unnecessarily. I took a look at the tests and see that it's validating that the [dir] prefix is added to directionally sensitive things.

Is there any reasoning for adding the dir prefix and modifying symmetrical rules at all? I'm assuming it's a specificity related thing like make all directional rules at least 2 levels of specificity so that they can be predictably overridden, maybe?

The workaround mentioned above would work, but the duplication fights against bundle size as well.

Could the [dir] prefixes be removed? Accepting PRs?

without [dir] (and such) prefixes you can use some postcss plugins like this one: https://www.npmjs.com/package/postcss-discard-duplicates

@evilaliv3
Copy link

evilaliv3 commented May 14, 2020

@LFFATE: Would please you explain it more in details? Thank you!

@dzearing
Copy link

dzearing commented May 15, 2020

I believe @LFFATE means if we use the previously suggested custom prefix to strip the dirs, then that will create the dupe rule situation, but that could be worked around with that extra plugin. That makes sense.

I certainly would just rather fix the plugin here though. And understand more deeply why symmetrical rules were prefixed with [dir] in the first place. It seems like there was some reason there because it's covered in the tests.

@evilaliv3
Copy link

Yes, i would prefer we could arrive to bugfix postcss-rtl that would offer more guarantees.

@fabercancio
Copy link

fabercancio commented May 16, 2020

Hey, I found this RTL plugin, I have been testing it and it seems to cover this specific issue (but it is very new so I don't know if it is reliable). Check the override mode (even if I'm normally against of overriding RTL it seems to work pretty well in this mode)

@evilaliv3
Copy link

evilaliv3 commented May 16, 2020 via email

@fabercancio
Copy link

fabercancio commented May 16, 2020

@evilaliv3 I can test any change, we are already using this library and we don't have any issues so far, so any change that you make I can check if it works with our code and doesn't break anything (but we are not overriding rules and we don't have any plans to do it).

@evilaliv3
Copy link

evilaliv3 commented May 17, 2020

I got it sorted out!

Here you can find the patch: evilaliv3@d0fe859

You could see the improvement as well in the commit evaluating which will be the change result in the modified CSS. In my experimentations on a library like bootstrap the save is around 3%.
Many are the advantages already documented in this ticket.

i've opened a pull request so that as soon we are confident we could eventually proceed with the integration.

In the meatime if any of you would like to support the retesting and provide feedback you find it on npm as @evilaliv3/postcss-rtl

@fabercancio
Copy link

The override works, some things that are still there or that are created by the overriding:

The second and third points didn't occur before:

@evilaliv3
Copy link

evilaliv3 commented May 17, 2020

Thank you @fabercancio for this fast reply.

will try to fix them all and gt back to you.
woull you please annotate me here for each of your reported bugs:

  1. the example of the initial rule

  2. the rule as currently badly rewritten

  3. the expected good rewrite

thank you!

evilaliv3 added a commit to evilaliv3/postcss-rtl that referenced this issue May 17, 2020
evilaliv3 added a commit to evilaliv3/postcss-rtl that referenced this issue May 17, 2020
evilaliv3 added a commit to evilaliv3/postcss-rtl that referenced this issue May 17, 2020
evilaliv3 added a commit to evilaliv3/postcss-rtl that referenced this issue May 17, 2020
@evilaliv3
Copy link

evilaliv3 commented May 17, 2020

@fabercancio
Copy link

@evilaliv3, the issues are fixed.
I'll try tomorrow with one of my projects and I let you know if everything is OK.
Regards ;)

@fabercancio
Copy link

fabercancio commented May 18, 2020

Hi @evilaliv3,

I have tested this with some big CSS files and it works OK in most of the cases. But I noticed that are multiple cases in which some declarations are overridden. This happens when multiple rules are applied in one element at the same time:

Imagine that both rules are added to the same element with the purpose of resetting the borders. This works with the current version of the library, but with the modified version the reset doesn't work because the prefixed rules are more specific (I think that this is the reason for the [dir] prefixes that @vkalinichev implemented):

@evilaliv3
Copy link

evilaliv3 commented May 18, 2020 via email

@hoorncj
Copy link

hoorncj commented Jun 12, 2020

Adding this here in case it helps someone:
My requirement was to create separate css files for rtl and ltr (using onlyDirection option), but like others, I ran into issues with the new [dir] tags overriding my other styles. To remove them I employed a trick similar to @LFFATE, with the prefix commented out. Seems to work fine from what I can tell.
addPrefixToSelector (selector, prefix) { return '/*${prefix}*/${selector}';}

@adrianmorales80
Copy link

Hi there! Encountered specificity issues between a 3rd party app and my application styles due to the [dir] attribute selector being added. I was able to overcome it using the !important attribute but looking forward to @evilaliv3's PR getting merged 🙏

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

No branches or pull requests

7 participants