Refactor FilterSet

History.md

@@ -5,6 +5,7 @@ Release date: unreleased
 
 * Make selenium driver :backspace clear stategy work even if caret location is in middle of field content [Champier Cyril]
 * Selenium issue with fieldset nested in disabled fieldset not being considered disabled
+
 ### Added
 
 * Work around Selenium lack of support for `file_detector` with remote geckodriver

lib/capybara/queries/selector_query.rb

@@ -43,7 +43,7 @@ module Capybara
         end
         @description << " with id #{options[:id]}" if options[:id]
         @description << " with classes [#{Array(options[:class]).join(',')}]" if options[:class]
-        @description << selector.description(skip_node_filters: applied && (@applied_filters != :node), **options)
+        @description << selector.description(node_filters: !applied || (@applied_filters == :node), **options)
         @description << ' that also matches the custom filter block' if @filter_block && (!applied || (@applied_filters == :node))
         @description << " within #{@resolved_node.inspect}" if describe_within?
         @description

lib/capybara/selector.rb

@@ -10,7 +10,7 @@ Capybara::Selector::FilterSet.add(:_field) do
   expression_filter(:name) { |xpath, val| xpath[XPath.attr(:name) == val] }
   expression_filter(:placeholder) { |xpath, val| xpath[XPath.attr(:placeholder) == val] }
 
-  describe(node_filters: true) do |checked: nil, unchecked: nil, disabled: nil, multiple: nil, **|
+  describe(:node_filters) do |checked: nil, unchecked: nil, disabled: nil, multiple: nil, **|
     desc, states = +'', []
     states << 'checked' if checked || (unchecked == false)
     states << 'not checked' if unchecked || (checked == false)
@@ -59,7 +59,7 @@ Capybara.add_selector(:field) do
     with.is_a?(Regexp) ? node.value =~ with : node.value == with.to_s
   end
 
-  describe do |type: nil, **options|
+  describe_expression_filters do |type: nil, **options|
     desc = +''
     (expression_filters.keys - [:type]).each { |ef| desc << " with #{ef} #{options[ef]}" if options.key?(ef) }
     desc << " of type #{type.inspect}" if type
@@ -130,7 +130,7 @@ Capybara.add_selector(:link) do
     expr[mod]
   end
 
-  describe do |**options|
+  describe_expression_filters do |**options|
     desc = +''
     desc << " with href #{options[:href].inspect}" if options[:href] && !options[:href].is_a?(Regexp)
     desc << ' with no href attribute' if options.fetch(:href, true).nil?
@@ -289,7 +289,7 @@ Capybara.add_selector(:select) do
     (Array(selected) - actual).empty?
   end
 
-  describe do |with_options: nil, **opts|
+  describe_expression_filters do |with_options: nil, **opts|
     desc = +''
     desc << " with at least options #{with_options.inspect}" if with_options
     desc << describe_all_expression_filters(opts)
@@ -326,7 +326,7 @@ Capybara.add_selector(:datalist_input) do
     end
   end
 
-  describe do |with_options: nil, **opts|
+  describe_expression_filters do |with_options: nil, **opts|
     desc = +''
     desc << " with at least options #{with_options.inspect}" if with_options
     desc << describe_all_expression_filters(opts)
@@ -416,8 +416,11 @@ Capybara.add_selector(:label) do
     end
   end
 
+  describe_expression_filters do |**options|
+    " for element with id of \"#{options[:for]}\"" if options.key?(:for) && !options[:for].is_a?(Capybara::Node::Element)
+  end
   describe_node_filters do |**options|
-    " for #{options[:for]}" if options[:for]
+    " for element #{options[:for]}" if options[:for]&.is_a?(Capybara::Node::Element)
   end
 end
 
@@ -433,7 +436,7 @@ Capybara.add_selector(:table) do
     xpath
   end
 
-  describe do |caption: nil, **|
+  describe_expression_filters do |caption: nil, **|
     " with caption \"#{caption}\"" if caption
   end
 end
@@ -450,7 +453,7 @@ Capybara.add_selector(:frame) do
     xpath
   end
 
-  describe do |name: nil, **|
+  describe_expression_filters do |name: nil, **|
     " with name #{name}" if name
   end
 end

lib/capybara/selector/filter_set.rb

@@ -5,14 +5,13 @@ require 'capybara/selector/filter'
 module Capybara
   class Selector
     class FilterSet
-      attr_reader :descriptions, :node_filter_descriptions, :node_filters, :expression_filters
+      attr_reader :node_filters, :expression_filters
 
       def initialize(name, &block)
         @name = name
-        @descriptions = []
-        @node_filter_descriptions = []
-        @expression_filters = {}
         @node_filters = {}
+        @expression_filters = {}
+        @descriptions = Hash.new { |h, k| h[k] = [] }
         instance_eval(&block)
       end
 
@@ -25,21 +24,45 @@ module Capybara
         add_filter(name, Filters::ExpressionFilter, *types_and_options, &block)
       end
 
-      def describe(node_filters: false, &block)
-        if node_filters
+      def describe(what = nil, &block)
+        case what
+        when nil
+          undeclared_descriptions.push block
+        when :node_filters
           node_filter_descriptions.push block
+        when :expression_filters
+          expression_filter_descriptions.push block
         else
-          descriptions.push block
+          raise ArgumentError, 'Unknown description type'
         end
       end
 
-      def description(skip_node_filters: false, **options)
+      def description(node_filters: true, expression_filters: true, **options)
         opts = options_with_defaults(options)
-        d = @descriptions.map { |desc| desc.call(opts).to_s }.join
-        d += @node_filter_descriptions.map { |desc| desc.call(opts).to_s }.join unless skip_node_filters
+        d = +''
+        d += undeclared_descriptions.map { |desc| desc.call(opts).to_s }.join
+        d += expression_filter_descriptions.map { |desc| desc.call(opts).to_s }.join if expression_filters
+        d += node_filter_descriptions.map { |desc| desc.call(opts).to_s }.join if node_filters
         d
       end
 
+      def descriptions
+        warn 'DEPRECATED: FilterSet#descriptions is deprecated without replacement'
+        [undeclared_descriptions, node_filter_descriptions, expression_filter_descriptions].flatten
+      end
+
+      def import(name, filters = nil)
+        f_set = self.class.all[name]
+        filter_selector = filters.nil? ? ->(*) { true } : ->(n, _) { filters.include? n }
+
+        expression_filters.merge!(f_set.expression_filters.select(&filter_selector))
+        node_filters.merge!(f_set.node_filters.select(&filter_selector))
+
+        f_set.undeclared_descriptions.each { |desc| describe(&desc) }
+        f_set.expression_filter_descriptions.each { |desc| describe(:expression_filters, &desc) }
+        f_set.node_filter_descriptions.each { |desc| describe(:node_filters, &desc) }
+      end
+
       class << self
         def all
           @filter_sets ||= {} # rubocop:disable Naming/MemoizedInstanceVariableName
@@ -54,6 +77,20 @@ module Capybara
         end
       end
 
+    protected
+
+      def undeclared_descriptions
+        @descriptions[:undeclared]
+      end
+
+      def node_filter_descriptions
+        @descriptions[:node_filters]
+      end
+
+      def expression_filter_descriptions
+        @descriptions[:expression_filters]
+      end
+
     private
 
       def options_with_defaults(options)

lib/capybara/selector/selector.rb

@@ -349,28 +349,23 @@ module Capybara
     def_delegators :@filter_set, :node_filter, :expression_filter, :filter
 
     def filter_set(name, filters_to_use = nil)
-      f_set = FilterSet.all[name]
-      filter_selector = filters_to_use.nil? ? ->(*) { true } : ->(n, _) { filters_to_use.include? n }
-      @filter_set.expression_filters.merge!(f_set.expression_filters.select(&filter_selector))
-      @filter_set.node_filters.merge!(f_set.node_filters.select(&filter_selector))
-      f_set.descriptions.each { |desc| @filter_set.describe(&desc) }
-      f_set.node_filter_descriptions.each { |desc| @filter_set.describe(node_filters: true, &desc) }
+      @filter_set.import(name, filters_to_use)
     end
 
     def_delegator :@filter_set, :describe
 
     def describe_expression_filters(&block)
       if block_given?
-        describe(node_filters: false, &block)
+        describe(:expression_filters, &block)
       else
-        describe(node_filters: false) do |**options|
+        describe(:expression_filters) do |**options|
           describe_all_expression_filters(options)
         end
       end
     end
 
     def describe_node_filters(&block)
-      describe(node_filters: true, &block)
+      describe(:node_filters, &block)
     end
 
     ##