Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

### Unreleased

* Fix selector splitting inside functional pseudo-classes (`:is()`, `:where()`, `:not()`, etc.) — commas inside parenthesised selector lists are no longer treated as selector separators

### Version 2.0.0
* Drop ruby <3.2, fix a memory leak

Expand Down
36 changes: 32 additions & 4 deletions lib/css_parser/rule_set.rb
Original file line number Diff line number Diff line change
Expand Up @@ -684,17 +684,45 @@ def unmatched_open_parenthesis?(declarations)
(lparen_index = declarations.index(LPAREN)) && !declarations.index(RPAREN, lparen_index)
end

#--
# TODO: way too simplistic
#++
# Split selector string on commas, but not commas inside parentheses
# (e.g. :is(rect, circle) or :where(.a, .b) should not be split).
Comment on lines +708 to +709
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

comment belongs to split_selectors

def parse_selectors!(selectors) # :nodoc:
@selectors = selectors.split(',').map do |s|
@selectors = split_selectors(selectors).map do |s|
s.gsub!(/\s+/, ' ')
s.strip!
s
end
end

def split_selectors(selectors)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should add this to stay fast:

Suggested change
def split_selectors(selectors)
def split_selectors(selectors)
return selectors.split(',') unless selectors.include?("(") # fast for simple cases
Comparison:
      plain (simple):  4437547.5 i/s
     hybrid (simple):  3727921.3 i/s - 1.19x  slower
      plain (nested):  3502059.9 i/s - 1.27x  slower
       char (simple):   895368.7 i/s - 4.96x  slower
       char (nested):   512457.1 i/s - 8.66x  slower
     hybrid (nested):   498396.6 i/s - 8.90x  slower

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

... actually how about

def split_selectors(selector)
  return selector.split(',') unless selector.include?("(") # fast for simple cases
  selectors = []
  start = 0
  depth = 0

  selector.each_char.with_index do |char, i|
    case char
    when '(' then depth += 1
    when ')' then depth -= 1
    when ','
      if depth == 0
        selectors << selector[start...i]
        start = i + 1
      end
    end
  end

  selectors << selector[start..]
  selectors
end

that performed the same (within 1%) and looks simpler to me

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good call - I've spent some more time looking at performance and persuaded Claude to try a CSS selector aware scan, which seems to be only marginally less performant than .split(',') on newer rubies, especially with YJIT enabled.

result = []
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe call this selectors and the arg selector

current = +''
depth = 0

selectors.each_char do |char|
case char
when '('
depth += 1
current << char
when ')'
depth -= 1
current << char
when ','
if depth > 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

would flip conditional to if depth == 0 since that is the whole idea of this code

current << char
else
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
else
else # this selector is done

result << current
current = +''
end
else
current << char
end
end

result << current unless current.empty?
result
end

def split_value_preserving_function_whitespace(value)
split_value = value.gsub(RE_FUNCTIONS) do |c|
c.gsub!(/\s+/, WHITESPACE_REPLACEMENT)
Expand Down
21 changes: 21 additions & 0 deletions test/test_rule_set.rb
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,27 @@ def test_selector_sanitization
assert rs.selectors.member?("h3")
end

def test_selector_splitting_preserves_functional_pseudo_classes
# :is() and :where() can contain comma-separated selector lists
rs = RuleSet.new(selectors: ':is(rect, circle)', block: 'fill: red;')
assert_equal [':is(rect, circle)'], rs.selectors
end

def test_selector_splitting_preserves_where_pseudo_class
rs = RuleSet.new(selectors: ':where(.a, .b)', block: 'color: blue;')
assert_equal [':where(.a, .b)'], rs.selectors
end

def test_selector_splitting_with_functional_pseudo_class_and_other_selectors
rs = RuleSet.new(selectors: ':is(h1, h2, h3), .other', block: 'color: red;')
assert_equal [':is(h1, h2, h3)', '.other'], rs.selectors
end

def test_selector_splitting_with_nested_functional_pseudo_classes
rs = RuleSet.new(selectors: ':is(:not(.a), .b)', block: 'color: red;')
assert_equal [':is(:not(.a), .b)'], rs.selectors
end

def test_multiple_selectors_to_s
selectors = "#content p, a"
rs = RuleSet.new(selectors: selectors, block: "color: #fff;")
Expand Down