Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Github Issue:1178 -- throw warning with trying to set readonly attribute #1181

Closed
wants to merge 3 commits into from

4 participants

@comjf

Inspired from this issue: #1178

This is my first PR to an open source project, so please correct me if I didn't follow some protocol, I'd like to contribute more in the future.

I decided to throw a warning instead of throwing an error (As the issue requested) for backwards compatibility

I did not create any new tests, but modified the 2 tests I could find that cared about read only fields.

@jnicklas
Owner

Thanks for contributing, and welcome ;)

I'm not sure if I like using a warning like this. At least the warning could be more descriptive.

I'd actually forgotten that we'd written tests codifying the behaviour of ignoring writes to readonly elements. Semver then tells us we shouldn't raise an exception.

I'm torn on this one. @twalpole, what do you think?

@comjf

Thanks! I can obviously change the warning to be whatever message you guys like, I just wanted to get start contributing to see if I could do it.

Is there a priority list or something for my to look into doing next?

@jnicklas
Owner

@comjf not really. Just check around on the issues list if you see anything open that you think you can fix. Thanks for helping out!

@twalpole
Collaborator

I would lean more towards raising an exception (and bumping the version if we feel necessary) since users obviously can't edit readonly fields and therefore any test that attempts to do so is broken. This change could be reserved until we have a few other breaking changes that would justify a version bump. One solution may be to go with a deprecation warning for now and change to raising an exception at the next breaking version.

@jnicklas
Owner

So what do we do about this? I'm not super enthusiastic about forcing drivers to print a warning to pass the suite, I don't think that's the right call.

@comjf

Sorry, I completely forgot about this. I'll go ahead and update to have it raise an exception later this week.

What would you like the exception to be?

Thanks for the reminder.

@twalpole
Collaborator

@jnicklas - how about we have the built-in drivers output a deprecation warning for now, but not test for it - and add a test for throwing an exception that is pending until version >= 3.0.0 which would be when semver allows for the change

@jnicklas
Owner

@twalpole that sounds like an excellent suggestion. :+1:

@abotalov
Collaborator

@comjf Do you plan to work on this? Otherwise I can do it myself.

@twalpole
Collaborator

@comjf / @abotalov - any progress on this?

@comjf

@twalpole Please review, I'm rusty and completely forgot about this. I didn't quite understand what you meant by 'have the built-in drivers output a deprecation warning for now', not sure how to do that. So I simply made a new exception and modified tests for it. When 3.0 is ready we can look at this again.

Perhaps I should close this and make a new PR to a different branch? (I also need to squash my history)

@twalpole
Collaborator

@comjf The thought was that instead of raising an exception currently, just do

warn "Attempt to set readonly element with value: #{value}"  

but add a test that checks for an exception being thrown if the version of capybara is >= 3 which will remind us that we need to implement that in the future. Don't test for the warn though since it's not actually an update to the required API for other drivers

@comjf

Ah ok, that should be easy enough, sorry that I misunderstood you. I'll close this and reopen a PR with just what you describe, but is there anything else that should be done at the moment?

Thanks,
James Flowers

@twalpole
Collaborator

nope -- I think those changes would complete this - thanks

@abotalov abotalov added the Bug label
@comjf comjf closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 26, 2013
  1. @comjf
Commits on Apr 2, 2014
  1. @comjf

    Fix upstream merge

    comjf authored
Commits on Apr 3, 2014
  1. @comjf
This page is out of date. Refresh to see the latest.
View
1  lib/capybara.rb
@@ -13,6 +13,7 @@ class FileNotFound < CapybaraError; end
class UnselectNotAllowed < CapybaraError; end
class NotSupportedByDriverError < CapybaraError; end
class InfiniteRedirectError < CapybaraError; end
+ class ReadOnlyElementError < CapybaraError; end
class << self
attr_accessor :asset_host, :app_host, :run_server, :default_host, :always_include_port
View
10 lib/capybara/rack_test/node.rb
@@ -27,7 +27,11 @@ def set(value)
elsif input_field?
set_input(value)
elsif textarea?
- native.content = value.to_s unless self[:readonly]
+ unless self[:readonly]
+ native.content = value.to_s
+ else
+ raise Capybara::ReadOnlyElementError.new "Attempt to set readonly element with value: #{value}"
+ end
end
end
@@ -164,8 +168,10 @@ def set_input(value)
new_native['value'] = v.to_s
end
native.remove
+ elsif self[:readonly]
+ raise Capybara::ReadOnlyElementError.new "Attempt to set readonly element with value: #{value}"
else
- native['value'] = value.to_s unless self[:readonly]
+ native['value'] = value.to_s
end
end
View
8 lib/capybara/selenium/node.rb
@@ -41,8 +41,12 @@ def set(value)
native.clear
else
#script can change a readonly element which user input cannot, so dont execute if readonly
- driver.browser.execute_script "arguments[0].value = ''", native unless self[:readonly]
- native.send_keys(value.to_s)
+ unless self[:readonly]
+ driver.browser.execute_script "arguments[0].value = ''", native
+ native.send_keys(value.to_s)
+ else
+ raise Capybara::ReadOnlyElementError.new "Attempt to set readonly #{tag_name} with value: #{value}"
+ end
end
elsif native.attribute('isContentEditable')
#ensure we are focused on the element
View
8 lib/capybara/spec/session/node_spec.rb
@@ -94,13 +94,17 @@
it "should not set if the text field is readonly" do
expect(@session.first('//input[@readonly]').value).to eq('should not change')
- @session.first('//input[@readonly]').set('changed')
+ expect do
+ @session.first('//input[@readonly]').set('changed')
+ end.to raise_error(Capybara::ReadOnlyElementError)
expect(@session.first('//input[@readonly]').value).to eq('should not change')
end
it "should not set if the textarea is readonly" do
expect(@session.first('//textarea[@readonly]').value).to eq('textarea should not change')
- @session.first('//textarea[@readonly]').set('changed')
+ expect do
+ @session.first('//textarea[@readonly]').set('changed')
+ end.to raise_error(Capybara::ReadOnlyElementError)
expect(@session.first('//textarea[@readonly]').value).to eq('textarea should not change')
end
Something went wrong with that request. Please try again.