Move css/xpath attribute requirement generation into the builders

lib/capybara/queries/selector_query.rb

@@ -91,11 +91,11 @@ module Capybara
         exact = exact? if exact.nil?
         expr = apply_expression_filters(@expression)
         expr = exact ? expr.to_xpath(:exact) : expr.to_s if expr.respond_to?(:to_xpath)
-        filtered_xpath(expr)
+        filtered_expression(expr)
       end
 
       def css
-        filtered_css(apply_expression_filters(@expression))
+        filtered_expression(apply_expression_filters(@expression))
       end
 
       # @api private
@@ -233,23 +233,11 @@ module Capybara
         raise ArgumentError, "invalid keys #{invalid_names}, should be one of #{valid_names}"
       end
 
-      def filtered_xpath(expr)
+      def filtered_expression(expr)
-        expr = "(#{expr})[#{conditions_from_id}]" if use_default_id_filter?
+        conditions = {}
-        expr = "(#{expr})[#{conditions_from_classes}]" if use_default_class_filter?
+        conditions[:id] = options[:id] if use_default_id_filter?
-        expr
+        conditions[:class] = options[:class] if use_default_class_filter?
-      end
+        builder.add_attribute_conditions(expr, conditions)
-
-      def filtered_css(expr)
-        id_conditions = conditions_from_id if use_default_id_filter?
-        id_conditions = [''] unless id_conditions&.any?
-
-        class_conditions = conditions_from_classes if use_default_class_filter?
-        class_conditions = [''] unless class_conditions&.any?
-
-        conditions = id_conditions.product(class_conditions)
-        ::Capybara::Selector::CSS.split(expr).map do |sel|
-          conditions.map { |(id_cond, class_cond)| sel + id_cond + class_cond }.join(', ')
-        end.join(', ')
       end
 
       def use_default_id_filter?
@@ -260,14 +248,6 @@ module Capybara
         options.key?(:class) && !custom_keys.include?(:class)
       end
 
-      def conditions_from_classes
-        builder.class_conditions(options[:class])
-      end
-
-      def conditions_from_id
-        builder.id_conditions(options[:id])
-      end
-
       def apply_expression_filters(expression)
         unapplied_options = options.keys - valid_keys
         expression_filters.inject(expression) do |expr, (name, ef)|

lib/capybara/selector.rb

@@ -34,7 +34,7 @@ Capybara.add_selector(:css) do
 end
 
 Capybara.add_selector(:id) do
-  xpath { |id| XPath.descendant[builder.id_conditions(id)] }
+  xpath { |id| builder.add_attribute_conditions(XPath.descendant, id: id) }
   locator_filter {  |node, id| id.is_a?(Regexp) ? node[:id] =~ id : true }
 end
 
@@ -119,7 +119,7 @@ Capybara.add_selector(:link) do
   end
 
   expression_filter(:download, valid_values: [true, false, String]) do |expr, download|
-    expr[builder.attribute_conditions(download: download)]
+    builder.add_attribute_conditions(expr, download: download)
   end
 
   describe_expression_filters do |**options|
@@ -489,7 +489,7 @@ Capybara.add_selector(:element) do
   end
 
   expression_filter(:attributes, matcher: /.+/) do |xpath, name, val|
-    xpath[builder.attribute_conditions(name => val)]
+    builder.add_attribute_conditions(xpath, name => val)
   end
 
   node_filter(:attributes, matcher: /.+/) do |node, name, val|

lib/capybara/selector/builders/css_builder.rb

@@ -30,6 +30,30 @@ module Capybara
           end.join
         end
 
+        def add_attribute_conditions(selector, **attributes)
+          attributes.inject(selector) do |css, (name, value)|
+            conditions = if name == :class
+              class_conditions(value)
+            elsif value.is_a? Regexp
+              Selector::RegexpDisassembler.new(value).alternated_substrings.map do |strs|
+                strs.map do |str|
+                  "[#{name}*='#{str}'#{' i' if value.casefold?}]"
+                end.join
+              end
+            else
+              [attribute_conditions(name => value)]
+            end
+
+            ::Capybara::Selector::CSS.split(css).map do |sel|
+              next sel if conditions.empty?
+
+              conditions.map { |cond| sel + cond }.join(', ')
+            end.join(', ')
+          end
+        end
+
+      private
+
         def class_conditions(classes)
           case classes
           when XPath::Expression
@@ -46,19 +70,6 @@ module Capybara
             cls[true].to_a.map { |cl| ":not(.#{Capybara::Selector::CSS.escape(cl.slice(1..-1))})" }).join]
           end
         end
-
-        def id_conditions(id)
-          case id
-          when Regexp
-            Selector::RegexpDisassembler.new(id).alternated_substrings.map do |id_strs|
-              id_strs.map do |str|
-                "[id*='#{str}'#{' i' if id.casefold?}]"
-              end.join
-            end
-          else
-            [attribute_conditions(id: id)]
-          end
-        end
       end
     end
   end

lib/capybara/selector/builders/xpath_builder.rb

@@ -24,6 +24,19 @@ module Capybara
           end.reduce(:&)
         end
 
+        def add_attribute_conditions(xpath, **conditions)
+          conditions.inject(xpath) do |xp, (name, value)|
+            conditions = name == :class ? class_conditions(value) : attribute_conditions(name => value)
+            if xp.is_a? XPath::Expression
+              xp[conditions]
+            else
+              "(#{xp})[#{conditions}]"
+            end
+          end
+        end
+
+      private
+
         def class_conditions(classes)
           case classes
           when XPath::Expression, Regexp
@@ -39,12 +52,6 @@ module Capybara
           end
         end
 
-        def id_conditions(id)
-          attribute_conditions(id: id)
-        end
-
-      private
-
         def regexp_to_xpath_conditions(regexp)
           condition = XPath.current
           condition = condition.uppercase if regexp.casefold?

spec/css_builder_spec.rb

@@ -0,0 +1,99 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Capybara::Selector::CSSBuilder do
+  let :builder do
+    ::Capybara::Selector::CSSBuilder
+  end
+
+  context 'add_attribute_conditions' do
+    it 'adds a single string condition to a single selector' do
+      css = 'div'
+      selector = builder.add_attribute_conditions(css, random: 'abc')
+      expect(selector).to eq %(div[random='abc'])
+    end
+
+    it 'adds multiple string conditions to a single selector' do
+      css = 'div'
+      selector = builder.add_attribute_conditions(css, random: 'abc', other: 'def')
+      expect(selector).to eq %(div[random='abc'][other='def'])
+    end
+
+    it 'adds a single string condition to a multiple selector' do
+      css = 'div, ul'
+      selector = builder.add_attribute_conditions(css, random: 'abc')
+      expect(selector).to eq %(div[random='abc'], ul[random='abc'])
+    end
+
+    it 'adds multiple string conditions to a multiple selector' do
+      css = 'div, ul'
+      selector = builder.add_attribute_conditions(css, random: 'abc', other: 'def')
+      expect(selector).to eq %(div[random='abc'][other='def'], ul[random='abc'][other='def'])
+    end
+
+    it 'adds simple regexp conditions to a single selector' do
+      css = 'div'
+      selector = builder.add_attribute_conditions(css, random: /abc/, other: /def/)
+      expect(selector).to eq %(div[random*='abc'][other*='def'])
+    end
+
+    it 'adds wildcard regexp conditions to a single selector' do
+      css = 'div'
+      selector = builder.add_attribute_conditions(css, random: /abc.*def/, other: /def.*ghi/)
+      expect(selector).to eq %(div[random*='abc'][random*='def'][other*='def'][other*='ghi'])
+    end
+
+    it 'adds alternated regexp conditions to a single selector' do
+      css = 'div'
+      selector = builder.add_attribute_conditions(css, random: /abc|def/, other: /def|ghi/)
+      expect(selector).to eq %(div[random*='abc'][other*='def'], div[random*='abc'][other*='ghi'], div[random*='def'][other*='def'], div[random*='def'][other*='ghi'])
+    end
+
+    it 'adds alternated regexp conditions to a multiple selector' do
+      css = 'div,ul'
+      selector = builder.add_attribute_conditions(css, other: /def.*ghi|jkl/)
+      expect(selector).to eq %(div[other*='def'][other*='ghi'], div[other*='jkl'], ul[other*='def'][other*='ghi'], ul[other*='jkl'])
+    end
+
+    it "returns original selector when regexp can't be substringed" do
+      css = 'div'
+      selector = builder.add_attribute_conditions(css, other: /.+/)
+      expect(selector).to eq 'div'
+    end
+
+    context ':class' do
+      it 'handles string with CSS .' do
+        css = 'a'
+        selector = builder.add_attribute_conditions(css, class: 'my_class')
+        expect(selector).to eq 'a.my_class'
+      end
+
+      it 'handles negated string with CSS .' do
+        css = 'a'
+        selector = builder.add_attribute_conditions(css, class: '!my_class')
+        expect(selector).to eq 'a:not(.my_class)'
+      end
+
+      it 'handles array of string with CSS .' do
+        css = 'a'
+        selector = builder.add_attribute_conditions(css, class: %w[my_class my_other_class])
+        expect(selector).to eq 'a.my_class.my_other_class'
+      end
+
+      it 'handles array of string with CSS . when negated included' do
+        css = 'a'
+        selector = builder.add_attribute_conditions(css, class: %w[my_class !my_other_class])
+        expect(selector).to eq 'a.my_class:not(.my_other_class)'
+      end
+    end
+
+    context ':id' do
+      it 'handles string with CSS #' do
+        css = 'ul'
+        selector = builder.add_attribute_conditions(css, id: 'my_id')
+        expect(selector).to eq 'ul#my_id'
+      end
+    end
+  end
+end

spec/xpath_builder_spec.rb

@@ -0,0 +1,91 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Capybara::Selector::XPathBuilder do
+  let :builder do
+    ::Capybara::Selector::XPathBuilder
+  end
+
+  context 'add_attribute_conditions' do
+    it 'adds a single string condition to a single selector' do
+      xpath = './/div'
+      selector = builder.add_attribute_conditions(xpath, random: 'abc')
+      expect(selector).to eq %((.//div)[(./@random = 'abc')])
+    end
+
+    it 'adds multiple string conditions to a single selector' do
+      xpath = './/div'
+      selector = builder.add_attribute_conditions(xpath, random: 'abc', other: 'def')
+      expect(selector).to eq %(((.//div)[(./@random = 'abc')])[(./@other = 'def')])
+    end
+
+    it 'adds a single string condition to a multiple selector' do
+      xpath = XPath.descendant(:div, :ul)
+      selector = builder.add_attribute_conditions(xpath, random: 'abc')
+      expect(selector.to_s).to eq xpath[XPath.attr(:random) == 'abc'].to_s
+    end
+
+    it 'adds multiple string conditions to a multiple selector' do
+      xpath = XPath.descendant(:div, :ul)
+      selector = builder.add_attribute_conditions(xpath, random: 'abc', other: 'def')
+      expect(selector.to_s).to eq %(.//*[self::div | self::ul][(./@random = 'abc')][(./@other = 'def')])
+    end
+
+    it 'adds simple regexp conditions to a single selector' do
+      xpath = XPath.descendant(:div)
+      selector = builder.add_attribute_conditions(xpath, random: /abc/, other: /def/)
+      expect(selector.to_s).to eq %(.//div[./@random[contains(., 'abc')]][./@other[contains(., 'def')]])
+    end
+
+    it 'adds wildcard regexp conditions to a single selector' do
+      xpath = './/div'
+      selector = builder.add_attribute_conditions(xpath, random: /abc.*def/, other: /def.*ghi/)
+      expect(selector).to eq %(((.//div)[./@random[(contains(., 'abc') and contains(., 'def'))]])[./@other[(contains(., 'def') and contains(., 'ghi'))]])
+    end
+
+    it 'adds alternated regexp conditions to a single selector' do
+      xpath = XPath.descendant(:div)
+      selector = builder.add_attribute_conditions(xpath, random: /abc|def/, other: /def|ghi/)
+      expect(selector.to_s).to eq %(.//div[./@random[(contains(., 'abc') or contains(., 'def'))]][./@other[(contains(., 'def') or contains(., 'ghi'))]])
+    end
+
+    it 'adds alternated regexp conditions to a multiple selector' do
+      xpath = XPath.descendant(:div, :ul)
+      selector = builder.add_attribute_conditions(xpath, other: /def.*ghi|jkl/)
+      expect(selector.to_s).to eq %(.//*[self::div | self::ul][./@other[((contains(., 'def') and contains(., 'ghi')) or contains(., 'jkl'))]])
+    end
+
+    it "returns original selector when regexp can't be substringed" do
+      xpath = './/div'
+      selector = builder.add_attribute_conditions(xpath, other: /.+/)
+      expect(selector).to eq '(.//div)[./@other]'
+    end
+
+    context ':class' do
+      it 'handles string' do
+        xpath = './/a'
+        selector = builder.add_attribute_conditions(xpath, class: 'my_class')
+        expect(selector).to eq %((.//a)[contains(concat(' ', normalize-space(./@class), ' '), ' my_class ')])
+      end
+
+      it 'handles negated strings' do
+        xpath = XPath.descendant(:a)
+        selector = builder.add_attribute_conditions(xpath, class: '!my_class')
+        expect(selector.to_s).to eq xpath[!XPath.attr(:class).contains_word('my_class')].to_s
+      end
+
+      it 'handles array of strings' do
+        xpath = './/a'
+        selector = builder.add_attribute_conditions(xpath, class: %w[my_class my_other_class])
+        expect(selector).to eq %((.//a)[(contains(concat(' ', normalize-space(./@class), ' '), ' my_class ') and contains(concat(' ', normalize-space(./@class), ' '), ' my_other_class '))])
+      end
+
+      it 'handles array of string when negated included' do
+        xpath = XPath.descendant(:a)
+        selector = builder.add_attribute_conditions(xpath, class: %w[my_class !my_other_class])
+        expect(selector.to_s).to eq xpath[XPath.attr(:class).contains_word('my_class') & !XPath.attr(:class).contains_word('my_other_class')].to_s
+      end
+    end
+  end
+end