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.

Sign up for GitHub

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

Jump to bottom

Bugfix/separate selectors #1253

Merged
merged 4 commits into from Oct 14, 2019
Merged

Bugfix/separate selectors #1253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
622e8a3
Fix #1196
fanich37 Oct 7, 2019
b73faa5
Add tests for grouping rules
fanich37 Oct 8, 2019
d3099c6
Use instance data instead of global const
fanich37 Oct 11, 2019
68d8718
Make solution more proper and easier to reason about
fanich37 Oct 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 24 additions & 7 deletions lib/selector.js
View file
Open in desktop
@@ -1,3 +1,5 @@
let { list } = require('postcss')

let OldSelector = require('./old-selector')
let Prefixer = require('./prefixer')
let Browsers = require('./browsers')
Expand Down Expand Up @@ -53,15 +55,30 @@ class Selector extends Prefixer {
*/
prefixeds (rule) {
if (rule._autoprefixerPrefixeds) {
return rule._autoprefixerPrefixeds
if (rule._autoprefixerPrefixeds[this.name]) {
return rule._autoprefixerPrefixeds
}
} else {
rule._autoprefixerPrefixeds = {}
}

let prefixeds = {}
for (let prefix of this.possible()) {
prefixeds[prefix] = this.replace(rule.selector, prefix)
if (rule.selector.includes(',')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely lost the idea behind this method (but I wrote it 😭).

Can you describe (you can do it in Russian) what do we do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to do it in english 🤞

This method collects all possible prefixes for the selector, creates prefixed version of the rule.selector, caches the result and adds it to the rule. So the rule looks like this at the end (copied from master branch):

Rule {
    type: 'rule',
    selector: '.a, .a .b::placeholder, a:read-only',
    // lots of properties
    _autoprefixerPrefixeds: {
        '-webkit-': '.a, .a .b::-webkit-input-placeholder, a:read-only',
        '-moz-': '.a, .a .b::-moz-placeholder, a:read-only',
        '-ms-': '.a, .a .b::-ms-input-placeholder, a:read-only',
        '-o-': '.a, .a .b::-o-placeholder, a:read-only',
        '-moz- old': '.a, .a .b:-moz-placeholder, a:read-only',
        '-ms- old': '.a, .a .b:-ms-input-placeholder, a:read-only' }
    }
}

As you can see :read-only is left without prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored code, and I think now it's much more easier to reason about. Plus, I decided that each selector has to process only that part of rule that contains the selector and not the whole rule as I did before.

let ruleParts = list.comma(rule.selector)
let toProcess = ruleParts.filter(el => el.includes(this.name))

for (let prefix of this.possible()) {
prefixeds[prefix] = toProcess
.map(el => this.replace(el, prefix))
.join(', ')
}
} else {
for (let prefix of this.possible()) {
prefixeds[prefix] = this.replace(rule.selector, prefix)
}
}

rule._autoprefixerPrefixeds = prefixeds
rule._autoprefixerPrefixeds[this.name] = prefixeds
return rule._autoprefixerPrefixeds
}

Expand All @@ -79,8 +96,8 @@ class Selector extends Prefixer {
}

let some = false
for (let key in prefixeds) {
let prefixed = prefixeds[key]
for (let key in prefixeds[this.name]) {
let prefixed = prefixeds[this.name][key]
if (before.selector === prefixed) {
if (prefix === key) {
return true
Expand Down Expand Up @@ -117,7 +134,7 @@ class Selector extends Prefixer {
return
}

let cloned = this.clone(rule, { selector: prefixeds[prefix] })
let cloned = this.clone(rule, { selector: prefixeds[this.name][prefix] })
rule.parent.insertBefore(rule, cloned)
}

Expand Down
7 changes: 7 additions & 0 deletions test/autoprefixer.test.js
View file
Open in desktop
Expand Up @@ -44,6 +44,10 @@ let uncascader = autoprefixer({
let gradienter = autoprefixer({
overrideBrowserslist: ['Chrome 25', 'Opera 12', 'Android 2.3']
})
let grouping = autoprefixer({
overrideBrowserslist: ['Chrome 25', 'Firefox > 17', 'IE 10', 'Edge 12'],
grid: 'autoplace'
})
let ffgradienter = autoprefixer({
overrideBrowserslist: ['Chrome 25', 'Opera 12', 'Firefox 6']
})
Expand Down Expand Up @@ -90,6 +94,8 @@ function prefixer (name) {
return gradienter
} else if (name === 'gradient-fix') {
return ffgradienter
} else if (name === 'grouping-rule') {
return grouping
} else if (name === 'flexbox' || name === 'flex-rewrite') {
return flexboxer
} else if (name === 'double') {
Expand Down Expand Up @@ -244,6 +250,7 @@ it('saves declaration style', () => check('style'))
it('uses ignore next control comments', () => check('ignore-next'))
it('uses block control comments', () => check('disabled'))
it('has actual example in docs', () => check('example'))
it('process grouping rules correctly', () => check('grouping-rule'))

it('uses control comments to whole scope', () => {
let input = read('scope')
Expand Down
23 changes: 23 additions & 0 deletions test/cases/grouping-rule.css
View file
Open in desktop
@@ -0,0 +1,23 @@
.grid {
display: grid;
}

.a,
.b,
.c::selection,
.d:read-only,
.e::placeholder {
color: yellow;
}

::selection {
color: red;
}

:read-only {
color: black;
}

.f:read-write, .g:read-write {
background: #fff;
}
64 changes: 64 additions & 0 deletions test/cases/grouping-rule.out.css
View file
Open in desktop
@@ -0,0 +1,64 @@
.grid {
display: -ms-grid;
display: grid;
}

.c::-moz-selection {
color: yellow;
}

.e::-webkit-input-placeholder {
color: yellow;
}

.e:-moz-placeholder {
color: yellow;
}

.e::-moz-placeholder {
color: yellow;
}

.e:-ms-input-placeholder {
color: yellow;
}

.e::-ms-input-placeholder {
color: yellow;
}

.d:-moz-read-only {
color: yellow;
}

.a,
.b,
.c::selection,
.d:read-only,
.e::placeholder {
color: yellow;
}

::-moz-selection {
color: red;
}

::selection {
color: red;
}

:-moz-read-only {
color: black;
}

:read-only {
color: black;
}

.f:-moz-read-write, .g:-moz-read-write {
background: #fff;
}

.f:read-write, .g:read-write {
background: #fff;
}
35 changes: 29 additions & 6 deletions test/selector.test.js
View file
Open in desktop
Expand Up @@ -28,7 +28,7 @@ describe('regexp()', () => {
})

describe('check()', () => {
it('shecks rule selectors', () => {
it('checks rule selectors', () => {
let css = parse('body .selection {}, ' +
':::selection {}, body ::selection {}')
expect(selector.check(css.nodes[0])).toBeFalsy()
Expand All @@ -38,13 +38,36 @@ describe('check()', () => {
})

describe('prefixeds()', () => {
it('returns all avaiable prefixed selectors', () => {
it('grouping rule gets correct _autoprefixerPrefixeds property', () => {
let css = parse('.c::selection, .d:read-only {}')
let rSel = new Selector(':read-only', ['-moz-'])
selector.prefixeds(css.first)
rSel.prefixeds(css.first)
expect(css.first._autoprefixerPrefixeds).toEqual({
'::selection': {
'-webkit-': '.c::-webkit-selection',
'-moz-': '.c::-moz-selection',
'-ms-': '.c::-ms-selection',
'-o-': '.c::-o-selection'
},
':read-only': {
'-webkit-': '.d:-webkit-read-only',
'-moz-': '.d:-moz-read-only',
'-ms-': '.d:-ms-read-only',
'-o-': '.d:-o-read-only'
}
})
})

it('returns all available prefixed selectors', () => {
let css = parse('::selection {}')
expect(selector.prefixeds(css.first)).toEqual({
'-webkit-': '::-webkit-selection',
'-moz-': '::-moz-selection',
'-ms-': '::-ms-selection',
'-o-': '::-o-selection'
'::selection': {
'-webkit-': '::-webkit-selection',
'-moz-': '::-moz-selection',
'-ms-': '::-ms-selection',
'-o-': '::-o-selection'
}
})
})
})
Expand Down