Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Support filling in `contenteditable` elements #911

Closed
wants to merge 10 commits into from

11 participants

Jon Rowe tgaff Jonas Nicklas Thomas Walpole Jari Bakken Andrei Botalov Jerome Gravel-Niquet Brad Fults Jon Kinney Dave Gerton Sam Saccone
Jon Rowe

Allow elements that are content-editable to be set, and allow them to be filled in with fill_in 'element', with: 'content'.

There are commits to xpath to make this possible too, I don't know how Github handles those.

Would you like me to update the Readme to indicate this is possible too?

lib/capybara/spec/session/fill_in_spec.rb
@@ -103,6 +103,20 @@
extract_results(@session)['first_name'].should == 'Harry'
end
+ it 'should allow me to change the contents of a contenteditable element', :requires => [:js], :contenteditable => true do
+ @session.visit('/with_js')
+ @session.fill_in('existing_content_editable', :with => 'WYSIWYG')
+ sleep 0.1
Jon Rowe
JonRowe added a note

I'd obviously rather not do this, but it seems capybara won't always wait long enough for the browser to reflect the content change in the assertion. (Heisenbug) any suggestions?

Jonas Nicklas Owner

Why not just use have_css('#existing_content_editable', :text => 'WYSIWYG')?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tgaff

Hi @JonRowe What (or where) are the xpath changes you mentioned?

I didn't see an xpath repo under your name.

Jon Rowe JonRowe referenced this pull request in jnicklas/xpath
Closed

Allow contenteditable elements as fillable fields #48

Jon Rowe

Sorry, wasn't used to working with Git submodules, assumed it would be bundled in with this. See jnicklas/xpath#48

tgaff

I see, it doesn't show up unless I pull down PRs for xpath as well.

I was able to reproduce the issue. It looks more to me like it's intermittently not setting the contenteditable field at all. I ran it with 10 second sleeps and still got failures.

Jon Rowe

Oh I never got any failures with the 1 second sleep, without this PR you can't fill in contenteditable fields at all of course, so I just assumed that the heisenbug was down to how Capybara asserts on content, (for me it observably just wasn't waiting for the browser to finish)

tgaff

Alright, I ran it a few hundred times more. It looks like it's only failing for me if firefox loses focus during the tests. Strange. So based on that I suspect it's send_keys.

I couldn't replicate this issue with the other tests.

I'd also like to get a solution for contenteditable, so I was hoping to help rather than throw a spanner into this...

Jon Rowe

Ah yeah, there's a bunch of issues around Firefox and focus, especially on OSX, but in general this works :) Personally I'm hoping @jnicklas will merge this, and then phantomjs and the like will eventually integrate it, rather than relying on selenium.

Jonas Nicklas
Owner

I commented on the pull request to XPath here. I'm not convinced this is a good idea, to be honest.

Jon Rowe

As commented on jnicklas/xpath#48, this is to allow contenteditable elements to be interacted with via set, the xpath change add's the syntactic sugar to allow us to use fill_in as well. Please don't reject this PR because of the syntax, I will change and move the tests to work directly with find('#element').set('text') if that is your preference, but this is a necessary change for those of us working with the HTML5 spec.

Jonas Nicklas
Owner

Can you repush this with the changes to the xpath submodule removed?

Jon Rowe

Done, was in the process of moving them to a more appropriate place ;) Note that firefox seems to now require focus to run these as a suite together, but not individually when run with bundle exec rspec spec/selenium_spec.rb -t contenteditable which I find curious...

lib/capybara/spec/session/node_spec.rb
@@ -71,6 +71,18 @@
@session.first('//input').set('')
@session.first('//input').value.should == ''
end
+
+ it 'should allow me to change the contents of a contenteditable element', :requires => [:js], :contenteditable => true do
Jonas Nicklas Owner

why the :contenteditable metadata?

Jon Rowe
JonRowe added a note
Jonas Nicklas Owner

That would be excellent :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jon Rowe

FYI it passed "on my machine" once again only with firefox having focus... I tried various things with synchronise / timeouts but no joy.

Jonas Nicklas
Owner

Okay, I just tried running this, but the failures are rather painful. I'm very hesitant to merge something which makes it necessary to have focus on the browser to get a pass. For me it consistently failed when the browser didn't have focus. For new contributors especially, that's a stumbling block we can't afford. Do you have any ideas how we can work around this?

Jon Rowe

Yeah I've been able to fix similar issues with Selenium + Capybara before (was some tag completion widget) by firing the relevant focus events, so I'll try that here...

Thomas Walpole
Collaborator

Setting focusmanager.testmode to true in the firefox profile may solve this on win/osx, unfortunately it breaks (or at least used to) text entry on linux

Jon Rowe

I'll try that on OSX, but I we'll need a linux solution, for TravisCI and Linux contributors ;)

Jonas Nicklas
Owner

Capybara is completely fubarred on TravisCI anyway. I'm pretty close to just disabling builds entirely. We haven't had a passing build in ages. That's beside the point though, we do need this to work on Linux.

Thomas Walpole
Collaborator

@jnicklas - I was working on enabling tests to run on chrome as well as firefox, which would allow the hover test to pass on chrome, while its ignored on firefox, would that be acceptable here too? As far as the failing builds, do you have any clue why the base version tests fail on travis, running them locally appear to be fine for me. Also is there any reason to continue testing the allowed failure matrix (ree, 1.8.7)

Jonas Nicklas
Owner

@twalpole no, we should get rid of all 1.8.7 builds. I think the suite is simply too slow, that's why it fails on Travis, but I haven't investigated it more than shrugging at the fact that travis is broken again and realizing that I don't really care.

Jon Rowe

1.8.7 considered harmful? :trollface:

Capybara should try to maintain 1.8.7 compatibility if it can, or at least have a functional release, even if 1.x. is 1.8.7 and 2.x is 1.9.3 and beyond, just because so many people use 1.8.7 and Capybara is such a wonderful tool!

I'm a TravisCI fan so I might look at the build issue, but I agree it could well simply be build time, Capybara is not a fast test suite... (lol firefox :/)

Jonas Nicklas
Owner

Well we have this: https://github.com/jnicklas/capybara/blob/master/capybara.gemspec#L11

Which means that 2.1 will not run on 1.8.7 at all. Maintaining 1.8.7 compatibility is too much of a hassle, and EOL is reached in like two months anyway.

Jon Rowe

:+1: It's a sensible decision, 1.x still support's it right? People using 1.8.7 will probably be stuck on older versions of Capybara too :P in any case it's not a relevant discussion for this PR :)

Jonas Nicklas
Owner

Yes, 1.x still support 1.8.7.

Jon Rowe

Hey, ok so I got this working when it's out of focus, basically forcing the browser to select the start of the editable area solves that issue.

Jonas Nicklas
Owner

Seems a bit crazy :) Maybe @jarib has some insight if this is a sane thing to do?

Jon Rowe

It's because firefox doesn't treat certain events the same way if the browser window doesn't have focus, I've run into perfectly sane code that otherwise breaks on firefox in CI because of that "feature". This is a relatively sane way of making sure we're typing where we think we are. It's just JS has a clunky api for it.

Jari Bakken

I think writing a good bug report against focusmanager.testmode would be the better action to take (cc @AutomatedTester).

That said. I'd be more concerned about the hack to work around the ElementNotVisible error than the focus bug. You're essentially giving Capybara the ability to type into an element a real user would not be able to type into.

Also I think it's a bad practice in general to just work around what you think is incorrect behaviour in WebDriver, which does occasionally happen even though we've got it right in this case. :) If you don't let us know we won't have a chance to fix it.

Jon Rowe

@jarib

The hack to work around ElementNotVisible is a hack to work around a Selenium bug, you can type into an empty content-editable div just fine, Selenium just thinks it's not visible. In practise it is visible and usable.

The hack for focus is actually just to ensure consistency in tests, in my experience Chrome / Safari don't exhibit Firefox's behaviour when it comes to not responding to Javascript due to not having focus. My aim with this PR is actually to get the spec into Capybara, Selenium implementation is just a by product, if you have a suggested improvement please make it. (I want this feature in other [headless] drivers).

Jonas Nicklas
Owner

I agree with @jarib that we should report bugs upstream instead of hacking around them. If this is a legit bug, then the webdriver project should be made aware of it.

Jari Bakken

The hack to work around ElementNotVisible is a hack to work around a Selenium bug, you can type into an empty content-editable div just fine, Selenium just thinks it's not visible. In practise it is visible and usable.

@JonRowe What browser are you testing this in?

Jon Rowe

@jarib I take that statement back; It's actually a hack to work around Firefox not displaying them. Chrome and Safari display blank contenteditable elements as being one line high. I guess I made the mistake of not actually checking the test view in Firefox manually. This could be removed and the test data given styling to make it a line high instead.

Jon Rowe

@jnicklas @jarib What can I do to get this merged? Do you want me to remove/refactor the stuff around blank elements? I'm convinced the focus stuff should stay, as it's a workaround for allowing in the background tests...

Jonas Nicklas
Owner

@JonRowe I want to wait with this a little. I am focusing on getting 2.1 out the door, and this will have to wait until after that.

Andrei Botalov
Collaborator

@jnicklas Why don't you want to include it to 2.1 if it's already done? I'm talking about my PR too

Jonas Nicklas
Owner

@abotalov I think I'll include your PR in 2.1, I want to review it thoroughly again before I merge it though, since it's a rather large change. I don't really feel comfortable with this one though. It's implemented in a quite hacky way, and I'm worried that it might be difficult for other driver authors. I don't see it as a super important feature, so I want to wait.

Jon Rowe

I'm fine with waiting until after 2.1, however I feel the focus of this pull request has been missed.

My primary intention was to introduce the test for javascript drivers to support contenteditable, the better JS drivers follow Capybara for test support and in order to get this change into them (ok into poltergeist) I need to get the spec into Capybara.

I have to disagree with you over my implementation though, my method of implementing support in Selenium (to meet the base requirements of the project) is solely due to the "quite hacky way" Selenium works. If Selenium had real control of the browser/was a real browser, half my code wouldn't be required.

Thomas Walpole
Collaborator

@JonRowe I see 3 main issues with this PR
1. The ElementNotVisible hack definitely shouldn't be there, since if the element isn't visible how would the user ever click the element to begin editing it? A min-height in the div style so that it becomes visible and can then be clicked on fixes that.

2. The focus hack really shouldn't be necessary since clicking on the element should bring it into focus and allow editing. This may be something that needs a bug report over at selenium
3. This doesn't at all take into account the nested nature of contenteditable, which I believe there is an issue with in selenium

I took your PR as a start and played around with it in my contenteditable branch at https://github.com/twalpole/capybara/tree/contenteditable . One of the tests fails using both FF and Chrome, but for different reasons on each, and I think it may be an issue with selenium although I need to look into it further. The other 4 contenteditable tests pass

Jon Rowe
1. The ElementNotVisible hack definitely shouldn't be there, since if the element isn't visible
how would the user ever click the element to begin editing it? A min-height in the div style so
that it becomes visible and can then be clicked on fixes that.

Agreed, this was originally thought necessary because Firefox renders these differently to Chrome and I wasn't aware of the reason why I was getting the exception (I thought it was a Selenium bug, it's not).

So I've removed this.

2. The focus hack really shouldn't be necessary since clicking on the element should bring
it into focus and allow editing. This may be something that needs a bug report over at
selenium

I don't believe this to be a bug in Selenium, this is what I believe is happening, FireFox treats focus events differently when the browser is in the background as opposed to in the foreground. So when Selenium simulates a click/focus event, if the browser is in the background (whilst your tests are running, quite plausible) the event's don't work as they are supposed to and you get a heisenbug. This is a work around to allow this particular behaviour (simulating typing into a contenteditable) to work consistently.

3. This doesn't at all take into account the nested nature of contenteditable, which I believe
there is an issue with in selenium

Agreed, it deliberately doesn't, what's the issue? The use case here is you find the specfic element you want to type into, then set that, elements content, so you shouldn't have issues with nested content?

Thomas Walpole
Collaborator

@JonRowe - Note these are purely my opinions and not those of @jnicklas and his decisions are the ones that count for this project
2. Yes firefox has issues with focus, and on some platforms setting focusmanager.testmode in the profile will fix this. Unfortunately setting that on linux causes some other issues, and therefore isn't defaulted on by selenium yet. That being said I dont believe it is Capybaras job to work around the issue with a javascript hack, a click on the box should enough to select it and start editing, rather we should file bug reports upstream and hope/work to get them fixed.
3. Read the spec on contenteditable. If we're going to add support to the test cases other drivers need to follow then we should meet the spec for the feature. If a parent element is marked as contenteditable then its children become editable by default unless they specify contenteditable false. What if a parent is content editable and I want to edit one of the children -- checking self[:contenteditable] doesnt work for that. If you look at the branch in my repo you'll see that checking native.attribute('isContentEditable') does (docs for selenium seem to say it should be iscontenteditable but that doesnt work for FF or chrome).

Jon Rowe

Ah I see, you mean if we wanted to edit a child of a parent contenteditable this wouldn't work because it doesn't have the contenteditable flag itself.

Jari Bakken

Here's WebDriver's check for contenteditable. As you can see isContentEditable has problems in IE:

https://github.com/SeleniumHQ/selenium/blob/89f1e91ce/javascript/atoms/dom.js#L382

I'm not too familiar with Capybara's design, but why the need to duplicate this check instead of just falling through to native.send_keys when the user calls #set(value)? The default behaviour if none of the conditionals in that method kicks in is to do nothing, which I think is rather confusing - both send_keys or an exception would be a better user experience AFAICT.

Thomas Walpole
Collaborator

@jarib because there's a need to click the element to initiate editing, and a need to clear the element to meet capybaras definition of set

Jon Rowe

I changed this to select all text in an element, then type, removing the need for a manual clear. Still should work without focus. I also added a (pending) test for editing a nested element, but it doesn't seem to work properly. It selects the correct element but won't send the keys, I think that is a selenium bug. Any suggestions?

Jerome Gravel-Niquet

I'd like to use that. What's the status here?

Jon Rowe

This works as outlined so you're welcome to use my branch or take over this PR, I haven't had much luck convincing @jnicklas that this is necessary and I have no intention of going and hacking on selenium code to fix the issues this works around downstream. I mostly wanted the test to be included into capybara so I could go and implement this in poltergeist as I don't use selenium in my normal work.

Brad Fults
h3h commented

+1 for this feature.

Jon Kinney

:thumbsup: for this feature as well. I'd like to be able to 'type' (fill in) WYSIWYG fields.

Dave Gerton

Thanks for this PR @JonRowe. It's a shame it hasn't been pulled.

I can't speak for every +1, but I don't need the gold plating, just a narrow slice of all possible environments to prove my UI interaction work well enough for me to check in code. I'll take that over months of crossed fingers and missed bugs any day. With your change, I know the UI at least works in my dev environment. Without the change I don't know it works anywhere.

Sam Saccone

+1 for this

Jonas Nicklas jnicklas closed this in 3f5180f
Jonas Nicklas
Owner

Okay so you probably weren't expecting this, but better late than never. I just pulled this, it seems that focusmanager is now the default in Selenium, so this passes even without the browser having focus now, yay!

@JonRowe: I took the liberty of squashing your commits into one, with an appropriate commit message, hope that's okay. Since this is so old, I didn't want you to have to do the extra work.

Jon Rowe

:) Thanks! As you say, better late than never :heart:

Sam Saccone

huzzah!

Dmitry Vorotilin route referenced this pull request in teampoltergeist/poltergeist
Merged

Update capybara dependency #416

Jon Rowe JonRowe deleted the branch
Jon Rowe JonRowe referenced this pull request in teampoltergeist/poltergeist
Closed

Allow set or editing of content-editable #113

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 14, 2012
  1. Jon Rowe
Commits on Dec 15, 2012
  1. Jon Rowe
Commits on Jan 22, 2013
  1. Jon Rowe

    revert xpath to master

    JonRowe authored
Commits on Jan 23, 2013
  1. Jon Rowe
Commits on Feb 17, 2013
  1. Jon Rowe

    remove content editable tags

    JonRowe authored
Commits on Mar 5, 2013
  1. Jon Rowe
Commits on Mar 16, 2013
  1. Jon Rowe

    remove workaround for firefox behaviour, test page now has to have an…

    JonRowe authored
    … explicit height for firefox
  2. Jon Rowe

    compact focus workaround down

    JonRowe authored
Commits on Mar 17, 2013
  1. Jon Rowe
Commits on Mar 22, 2013
  1. Jon Rowe

    extra test for nested content

    JonRowe authored
This page is out of date. Refresh to see the latest.
9 lib/capybara/selenium/node.rb
View
@@ -34,6 +34,15 @@ def set(value)
elsif tag_name == 'textarea' or tag_name == 'input'
driver.browser.execute_script "arguments[0].value = ''", native
native.send_keys(value.to_s)
+ elsif native.attribute('isContentEditable')
+ #ensure we are focused on the element
+ script = <<-JS
+ var range = document.createRange();
+ range.selectNodeContents(arguments[0]);
+ window.getSelection().addRange(range);
+ JS
+ driver.browser.execute_script script, native
+ native.send_keys(value.to_s)
end
end
19 lib/capybara/spec/session/node_spec.rb
View
@@ -71,6 +71,25 @@
@session.first('//input').set('')
@session.first('//input').value.should == ''
end
+
+ it 'should allow me to change the contents of a contenteditable element', :requires => [:js] do
+ @session.visit('/with_js')
+ @session.find(:css,'#existing_content_editable').set('WYSIWYG')
+ @session.find(:css,'#existing_content_editable').text.should == 'WYSIWYG'
+ end
+
+ it 'should allow me to set the contents of a contenteditable element', :requires => [:js] do
+ @session.visit('/with_js')
+ @session.find(:css,'#blank_content_editable').set('WYSIWYG')
+ @session.find(:css,'#blank_content_editable').text.should == 'WYSIWYG'
+ end
+
+ it 'should allow me to change the contents of a contenteditable elements child', :requires => [:js] do
+ pending "Selenium doesn't like editing nested contents"
+ @session.visit('/with_js')
+ @session.find(:css,'#existing_content_editable_child').set('WYSIWYG')
+ @session.find(:css,'#existing_content_editable_child').text.should == 'WYSIWYG'
+ end
end
describe "#tag_name" do
8 lib/capybara/spec/views/with_js.erb
View
@@ -36,6 +36,14 @@
</p>
<p>
+ <div contenteditable='true' id='existing_content_editable'>Editable content</div>
+ <div contenteditable='true' id='blank_content_editable' style='height: 1em;'></div>
+ <div contenteditable='true' style='height: 1em;'>
+ <div id='existing_content_editable_child' style='height: 1em;'>Content</div>
+ </div>
+ </p>
+
+ <p>
<input type="checkbox" id="checkbox_with_event"/>
</p>
Something went wrong with that request. Please try again.