Move marionette workarounds into their own class and make browser check methods private

lib/capybara.rb

@@ -426,6 +426,7 @@ module Capybara
   require 'capybara/rack_test/css_handlers.rb'
 
   require 'capybara/selenium/node'
+  require 'capybara/selenium/nodes/marionette_node'
   require 'capybara/selenium/driver'
 end
 

lib/capybara/selector/filters/expression_filter.rb

@@ -12,9 +12,7 @@ module Capybara
       end
 
       class IdentityExpressionFilter < ExpressionFilter
-        def initialize(name)
-          super(name, nil, nil)
-        end
+        def initialize(name); super(name, nil, nil); end
         def default?; false; end
         def matcher?; false; end
         def apply_filter(expr, _name, _value); expr; end

lib/capybara/selenium/driver.rb

@@ -10,7 +10,6 @@ class Capybara::Selenium::Driver < Capybara::Driver::Base
     clear_session_storage: false
   }.freeze
   SPECIAL_OPTIONS = %i[browser clear_local_storage clear_session_storage].freeze
-
   attr_reader :app, :options
 
   def self.load_selenium
@@ -36,11 +35,12 @@ class Capybara::Selenium::Driver < Capybara::Driver::Base
 
       @processed_options = options.reject { |key, _val| SPECIAL_OPTIONS.include?(key) }
       @browser = Selenium::WebDriver.for(options[:browser], @processed_options)
-
       @w3c = ((defined?(Selenium::WebDriver::Remote::W3CCapabilities) && @browser.capabilities.is_a?(Selenium::WebDriver::Remote::W3CCapabilities)) ||
               (defined?(Selenium::WebDriver::Remote::W3C::Capabilities) && @browser.capabilities.is_a?(Selenium::WebDriver::Remote::W3C::Capabilities)))
-      main = Process.pid
 
+      @node_class = ::Capybara::Selenium::MarionetteNode if marionette?
+
+      main = Process.pid
       at_exit do
         # Store the exit status of the test run since it goes away after calling the at_exit proc...
         @exit_status = $ERROR_INFO.status if $ERROR_INFO.is_a?(SystemExit)
@@ -59,6 +59,7 @@ class Capybara::Selenium::Driver < Capybara::Driver::Base
     @exit_status = nil
     @frame_handles = {}
     @options = DEFAULT_OPTIONS.merge(options)
+    @node_class = ::Capybara::Selenium::Node
   end
 
   def visit(path)
@@ -90,11 +91,11 @@ class Capybara::Selenium::Driver < Capybara::Driver::Base
   end
 
   def find_xpath(selector)
-    browser.find_elements(:xpath, selector).map { |node| Capybara::Selenium::Node.new(self, node) }
+    browser.find_elements(:xpath, selector).map(&method(:build_node))
   end
 
   def find_css(selector)
-    browser.find_elements(:css, selector).map { |node| Capybara::Selenium::Node.new(self, node) }
+    browser.find_elements(:css, selector).map(&method(:build_node))
   end
 
   def wait?; true; end
@@ -299,38 +300,32 @@ class Capybara::Selenium::Driver < Capybara::Driver::Base
     Selenium::WebDriver::Error::NoSuchWindowError
   end
 
-  # @api private
+private
+
   def marionette?
     firefox? && browser && @w3c
   end
 
-  # @api private
   def firefox?
     browser_name == :firefox
   end
 
-  # @api private
   def chrome?
     browser_name == :chrome
   end
 
-  # @api private
   def edge?
     browser_name == :edge
   end
 
-  # @api private
   def ie?
     browser_name == :ie
   end
 
-  # @api private
   def browser_name
     browser.browser
   end
 
-private
-
   def native_args(args)
     args.map { |arg| arg.is_a?(Capybara::Selenium::Node) ? arg.native : arg }
   end
@@ -406,9 +401,13 @@ private
     when Hash
       arg.each { |k, v| arg[k] = unwrap_script_result(v) }
     when Selenium::WebDriver::Element
-      Capybara::Selenium::Node.new(self, arg)
+      build_node(arg)
     else
       arg
     end
   end
+
+  def build_node(native_node)
+    @node_class.new(self, native_node)
+  end
 end

lib/capybara/selenium/node.rb

@@ -92,12 +92,6 @@ class Capybara::Selenium::Node < Capybara::Driver::Node
       scroll_to_center
     end
 
-    if driver.marionette? && e.is_a?(::Selenium::WebDriver::Error::ElementNotInteractableError) && (tag_name == "tr")
-      warn "You are attempting to click a table row which has issues in geckodriver/marionette - see https://github.com/mozilla/geckodriver/issues/1228. " \
-           "Your test should probably be clicking on a table cell like a user would. Clicking the first cell in the row instead."
-      return find_css('th:first-child,td:first-child')[0].click
-    end
-
     raise e
   end
 
@@ -140,18 +134,7 @@ class Capybara::Selenium::Node < Capybara::Driver::Node
   alias :checked? :selected?
 
   def disabled?
-    return true unless native.enabled?
-
-    # workaround for selenium-webdriver/geckodriver reporting elements as enabled when they are nested in disabling elements
-    if driver.marionette?
-      if %w[option optgroup].include? tag_name
-        find_xpath("parent::*[self::optgroup or self::select]")[0].disabled?
-      else
-        !find_xpath("parent::fieldset[@disabled] | ancestor::*[not(self::legend) or preceding-sibling::legend][parent::fieldset[@disabled]]").empty?
-      end
-    else
-      false
-    end
+    !native.enabled?
   end
 
   def content_editable?
@@ -272,17 +255,12 @@ private
         arguments[0].dispatchEvent(new InputEvent('input'));
         arguments[0].dispatchEvent(new Event('change', { bubbles: true }));
       }
-      JS
+    JS
   end
 
   def set_file(value) # rubocop:disable Naming/AccessorMethodName
     path_names = value.to_s.empty? ? [] : value
-    if driver.marionette?
-      native.clear
-      Array(path_names).each { |p| native.send_keys(p) }
-    else
-      native.send_keys(Array(path_names).join("\n"))
-    end
+    native.send_keys(Array(path_names).join("\n"))
   end
 
   def set_content_editable(value) # rubocop:disable Naming/AccessorMethodName

lib/capybara/selenium/nodes/marionette_node.rb

@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+class Capybara::Selenium::MarionetteNode < Capybara::Selenium::Node
+  def click(keys = [], **options)
+    super
+  rescue ::Selenium::WebDriver::Error::ElementNotInteractableError
+    if tag_name == "tr"
+      warn "You are attempting to click a table row which has issues in geckodriver/marionette - see https://github.com/mozilla/geckodriver/issues/1228. " \
+           "Your test should probably be clicking on a table cell like a user would. Clicking the first cell in the row instead."
+      return find_css('th:first-child,td:first-child')[0].click
+    end
+    raise
+  end
+
+  def disabled?
+    return true if super
+
+    # workaround for selenium-webdriver/geckodriver reporting elements as enabled when they are nested in disabling elements
+    if %w[option optgroup].include? tag_name
+      find_xpath("parent::*[self::optgroup or self::select]")[0].disabled?
+    else
+      !find_xpath("parent::fieldset[@disabled] | ancestor::*[not(self::legend) or preceding-sibling::legend][parent::fieldset[@disabled]]").empty?
+    end
+  end
+
+  def set_file(value) # rubocop:disable Naming/AccessorMethodName
+    path_names = value.to_s.empty? ? [] : value
+    native.clear
+    Array(path_names).each { |p| native.send_keys(p) }
+  end
+end

spec/selenium_spec_marionette.rb

@@ -154,7 +154,7 @@ RSpec.describe Capybara::Selenium::Node do
       tr = session.find(:css, '#agent_table tr:first-child')
       allow(tr.base).to receive(:warn)
       tr.click
-      expect(tr.base).to have_received(:warn).with /Clicking the first cell in the row instead/
+      expect(tr.base).to have_received(:warn).with(/Clicking the first cell in the row instead/)
     end
   end
 end