JSS plugin nested => incorrect nesting complex selector

[heyymarco]

Hi, i found a bug: if nesting with any pseudo class like :is() & :where() that contains two/more sub selector inside the (...), the strange result occured:

export default {
  'btn': {
    '&:is(.hello, .world)': {
      color: 'red'
    },
    '&:where(.hello, .world)': {
      color: 'red'
    },
    '&:not(:where(.hello, .world))': {
      color: 'red'
    },
  }
}

the expected result:

.btn-0-1-114:is(.hello, .world) {
  color: red;
}
.btn-0-1-114:where(.hello, .world) {
  color: red;
}
.btn-0-1-114:not(:where(.hello, .world)) {
  color: red;
}

but i got:

.btn-0-1-114:is(.hello, .btn-0-1-114 .world) {
  color: red;
}
.btn-0-1-114:where(.hello, .btn-0-1-114 .world) {
  color: red;
}
.btn-0-1-114:not(:where(.hello, .btn-0-1-114 .world)) {
  color: red;
}

I think the jss-plugin-extend just splitting the selector list with comma and then merging each of it with parent selector. it doesn't aware of parentheses balancing

[kof]

The bug is somewhere along these lines, feel free to send a PR with a test and/or fix

[heyymarco]

yes, i meant the jss-plugin-nested. the bug is in here. The selector cannot just simply splited by regex. Regex cannot handle nested parentheses balancing. You need a selector parser.

[kof]

I am not sure why we even try instead of simply replacing the ampersand. Need to read the change history and unit tests to figure this out.

[seiyab]

jss-plugin-nested handles selector list.

sheet = jss.createStyleSheet({
  'a': {
    float: 'left',
    '& div, & span': {color: 'red'},
  }
})

The code above yields sheet that passes following.

it('should generate correct CSS', () => {
  expect(sheet.toString()).to.be(stripIndent`
  .a-id {
    float: left;
  }
  .a-id div, .a-id span {
    color: red;
  }
`)

So simply replacing the ampersand will break selector list feature.

Edited. Old wrong content is here.

On the other hand, JSS itself might not support selector list. ```js sheet = jss.createStyleSheet({ 'a, b': { float: 'left', '&.c': {color: 'red'} } }) ``` The code above yields sheet that passes following, that doesn't seem intended. ```js it('should generate correct CSS', () => { expect(sheet.toString()).to.be(stripIndent` .a\\,\\ b-id { float: left; } .a\\.c, \\ b-id.c { color: red; } ` ) ``` Simply replacing the ampersand might be better, giving up to handle selector list.

[kof]

Oh yeah, we should not simply split by comma without fully understanding the syntax, this was oversimplification for the sake of not having to write & each time I believe.

We need to get rid of that simplification and release a major version because this is going to be a breaking change

[kof]

Actually is there a unit test that confirms that behavior with automatic parent reference? I don't think this was ever part of public API or communicated anywhere, which might be a good excuse to try and bring it in a minor version? I am not sure if that's too dangerous, especially if bugs can't be spotted that easily

[seiyab]

Following might be reasonable cost if we want to keep split selector work.

  function splitSelector(selector) {
    let i = 0
    let nest = 0
    const result = []
    while (i < selector.length) {
      let j = i
      while (j < selector.length && (selector.charAt(j) !== ',' || nest > 0)) {
        if (selector.charAt(j) === '(') {
          nest += 1
        }
        if (selector.charAt(j) === ')') {
          nest -= 1
        }
        j += 1
      }
      const end = j < selector.length ? j : j + 1
      result.push(selector.slice(i, end).trim())
      i = j + 1
    }
    return result
  }

It passes at least all the existing automated tests and the following new test case.

describe('function selector', () => {
  let sheet

  beforeEach(() => {
    sheet = jss.createStyleSheet({
      a: {
        float: 'left',
        '&:is(p, i)': {
          color: 'blue'
        }
      }
    })
  })

  it('should add rules', () => {
    expect(sheet.getRule('a')).to.not.be(undefined)
    expect(sheet.getRule('.a-id:is(p, i)')).to.not.be(undefined)
  })

  it('should generate correct CSS', () => {
    expect(sheet.toString()).to.be(stripIndent`
      .a-id {
        float: left;
      }
      .a-id:is(p, i) {
        color: blue;
      }
    `)
  })
})

[kof]

I think generally going too deep into selector syntax is going to cause bugs and shouldn't be done at runtime

[seiyab]

It totally makes sense. Also, function selector seems to be more important than selector list.

If jss-plugin-nested stop to handle selector list, it might be better to be a major update. Some users use selector list (, that doesn't fully work). https://github.com/cssinjs/jss/issues/1185

[kof]

yeah, I am up for it if anyone wants to create a pr

[seiyab]

Just an idea: Following might ease compatibility worry.

const functionSelectorRegexp = /\(.*\,.*\)/
const parentSelectors = functionSelectorRegexp.test(parentProps) ? [parentProps] : parentProp.split(separatorRegExp)
const nestedSelectors = functionSelectorRegexp.test(nestedProps) ? [nestedProps] : nestedProp.split(separatorRegExp)

I'm not sure whether it is minor update or not.