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

[Firefox] -moz-read-only is not duplicated with all associated css selectors #1301

Open
albanlorillard opened this issue Apr 14, 2020 · 20 comments

Comments

@albanlorillard
Copy link

albanlorillard commented Apr 14, 2020

Hello,

Description

This issue is refered to angular/angular-cli#17325
Since I upgrade Angular 8 to 9, autoprefixer has updated from 9.6.1 to 9.7.1. Due to this update, behaviour of autoprefixer has changed about a piece of my code concerned Read-Only css rule.

This issue appear when read-only is in a same block of other css selector associate to the same css code. For example :

.element1:focus,
.element1:read-only {
  background-color: red;
}

Now, in 9.7.1, render :

/* prefixed by https://autoprefixer.github.io (PostCSS: v7.0.26, autoprefixer: v9.7.3) */
.element1:-moz-read-only {
  background-color: red;
}
.element1:focus,
.element1:read-only {
  background-color: red;
}

But should render, like previously :

.element1:-moz-read-only,
.element1:focus {
 background-color:red
}
.element1:focus,
.element1:read-only {
 background-color:red
}

As you can seen, .element1:focus, is never associate to a code block with .element1:-moz-read-only.

Because :read-only is unknown for firefox, all the code block that include .element1:read-only is never interpreted included other rules of this same code block. ( On the example, :focus is never interpreted, because on firefox, the code blocks failed due to unkown attribute :read-only )
On previous version that work because other rules (here :focus) is duplicated in the generated code block that include :-moz-read-only.

Minimal reproduction :

I previously create 2 scratchs projects for angular, where we can see the bug
Here is a repository with angular 9, (so, with autoprefixer 9.7.x) where the bug appear :
https://github.com/albanlorillard/angular9-readonly-bug

Here is a repository with angular 8, (so, with autoprefixer 9.6.x) where the bug NOT appear :
https://github.com/albanlorillard/angular8-readonly-bug

Browsers tested :

Firefox developer 75.0b9 (64 bits)
Firefox 73.0 (64-bit)

@ai
Copy link
Member

ai commented Apr 14, 2020

As I understand, having .element1:-moz-read-only, .element1:focus together is a wrong output, because Firefox will ignore the rule because of unknown :focus selector.

Can you explain better why the wrong code from previous version workes for you and new versions doesn’t work?

@albanlorillard
Copy link
Author

albanlorillard commented Apr 14, 2020

Thanks for your quick answer and sorry, I will tried to clarify.

:focus was just an example it could be any random others rules.

When I pass autoprefixer on this block code :

randomSelector1,
.element1:read-only,
randomSelector2,
randomSelector3 {
 /* css rules */
}

Now, autoprefixer not repeat my selectors on the rules which :-moz-read-only is generated :

/* Code interpreted on firefox : */ 
.element1:-moz-read-only {
/* css rules */
}

/* Code not interpreted on firefox due to :read-only, because it's an unknown pseudo-class for firefox, that result to an error*/ 
randomSelector1,
.element1:read-only,
randomSelector2,
randomSelector3 {
 /* css rules */
}

In consequence, on last version of autoprefixer, randomSelector1, randomSelector2, randomSelector3, is not associate to any css rules for firefox browser. But it does for any other browser like google chrome. (Because this last browser can interpreted this last piece of code)

Previously, on 3.6.x this selectors was correctly copied around the selector that have :moz-read-only pseudo-class

/* Code interpreted on firefox : */ 
randomSelector1,
.element1:-moz-read-only,
randomSelector2,
randomSelector3
 {
 /* css rules */
}

/* Code not interpreted on firefox, but doesn't matter, equivalent work fine on the top */ 
randomSelector1,
.element1:read-only,
randomSelector2,
randomSelector3 {
 /* css rules */
}

(Related issue on Bugzilla about :read-only pseudo-class : https://bugzilla.mozilla.org/show_bug.cgi?id=312971 / https://bugzilla.mozilla.org/show_bug.cgi?id=313111 )

@ai
Copy link
Member

ai commented Apr 14, 2020

Why do you need all these selectors together?

randomSelector1,
.element1:-moz-read-only,
randomSelector2,
randomSelector3

@albanlorillard
Copy link
Author

albanlorillard commented Apr 14, 2020

Because on my original CSS code, I gathered many selectors in a same piece of css code

If you prefer,this piece of code :

randomSelector1,
.element1:read-only,
randomSelector2,
randomSelector3 {
 /* css rules */
}

Not work at all on Firefox. Firefox not just ignore .element1:read-only and execute code for other selectors. It ignore css rules for all selectors, because for him, their is a syntax error (I imagine) due to unknown :read-only pseudoclass

@albanlorillard albanlorillard changed the title [Firefox] -moz-read-only is not duplicated with all associate rules [Firefox] -moz-read-only is not duplicated with all associated css selectors Apr 14, 2020
@ai
Copy link
Member

ai commented Apr 14, 2020

Got it.

I do not have time for this issue in the next few weeks.

You can help me by sending PR to this file: https://github.com/postcss/autoprefixer/blob/master/lib/selector.js

@rpearce
Copy link

rpearce commented Apr 29, 2020

@albanlorillard Were you able to find a way to work around this for now? I have the same issue.

@rpearce
Copy link

rpearce commented Apr 29, 2020

Can confirm that it works in 9.6.4 but breaks in 9.6.5

@albanlorillard
Copy link
Author

albanlorillard commented Apr 29, 2020

Hi !

I've not starting at all because I've not found the time for the moment. If you want to try to solve no problem for me ;)

Else, I will try to found some time next week

@rpearce
Copy link

rpearce commented Apr 29, 2020

I will look at it, but if you don't see anything from me in a week, assume that it might not happen

@shrpne
Copy link

shrpne commented May 29, 2020

Am I right, that solution to the issue is to revert this commit: f626441 ?

@ai
Copy link
Member

ai commented May 29, 2020

@shrpne that commit fixed abouther bug. BY reverting it we will close one issue by openning the another.

@ai
Copy link
Member

ai commented May 29, 2020

/cc @fanich37

@shrpne
Copy link

shrpne commented May 29, 2020

Is not this #1196 issue was invalid? Because what happens now with -moz-read-only is exactly what that issue was proposing.

@ai
Copy link
Member

ai commented May 29, 2020

@shrpne nope. The origin issue was about not putting all prefixes to the selector (if Safari will find -moz- in the selector it will ignore the whole rule).

Seems like we had an issue selector cleaning implementation and we need to fix it

@fanich37
Copy link
Contributor

fanich37 commented May 30, 2020

The origin issue was about not putting all prefixes to the selector

@ai May be it wasn't a good idea to separate them. Futhermore it out of autoprefixer scope to optimize the code somehow.

I haven't look through the code carefully but at first glance it's this line that causes such an issue"

let toProcess = ruleParts.filter(el => el.includes(this.name))

@ai
Copy link
Member

ai commented May 30, 2020

Oops, I missed the Expected and Actual in the original issue.

Yeap, let’s revert it.

@ai
Copy link
Member

ai commented May 30, 2020

@fanich37 can I ask you to send PR and add tests to prevent this regression in the future?

@fanich37
Copy link
Contributor

fanich37 commented May 30, 2020

Oops, I missed the Expected and Actual in the original issue.

Yeap, let’s revert it.

Reverting will open another bug as you mentioned above.

@fanich37
Copy link
Contributor

fanich37 commented Jun 1, 2020

@fanich37 can I ask you to send PR and add tests to prevent this regression in the future?

I'll try to check it ASAP. Just removing this line:

let toProcess = ruleParts.filter(el => el.includes(this.name))

won't help since we get this result:

.el1:focus, .el2::-moz-selection, .el3:read-only {
  background-color: red;
}

.el1:focus, .el2::selection, .el3:-moz-read-only {
  background-color: red;
}

.el1:focus,
.el2::selection,
.el3:read-only {
  background-color: red;
}

And the first rule generated with autoprefixer is still broken for Firefox. And it's also broken in 9.6.4:

.el1:focus,
.el2::-moz-selection,
.el3:read-only {
  background-color: red;
}

.el1:focus,
.el2::selection,
.el3:read-only {
  background-color: red;
}

@JustBeYou
Copy link

JustBeYou commented May 21, 2021

Using multiple pseudoclasses on the same element is also broken.

.example:any-link:read-only {}

Output:

/*
* Prefixed by https://autoprefixer.github.io
* PostCSS: v7.0.29,
* Autoprefixer: v9.7.6
* Browsers: >0%
*/

.example:any-link:-moz-read-only {}
.example:-webkit-any-link:read-only {}
.example:-moz-any-link:read-only {}
.example:any-link:read-only {}

Expected:

.example:any-link:read-only {}
.example:-moz-any-link:-moz-read-only {}
.example:any-link:-webkit-read-only {}

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

6 participants