Support filling in `contenteditable` elements #911

Closed
wants to merge 10 commits into
from

Projects

None yet
@JonRowe

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?

@JonRowe JonRowe and 1 other commented on an outdated diff Dec 15, 2012
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
@JonRowe
JonRowe Dec 15, 2012

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?

@jnicklas
jnicklas Jan 23, 2013

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

@tgaff

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

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

@JonRowe JonRowe referenced this pull request in jnicklas/xpath Jan 22, 2013
Closed

Allow contenteditable elements as fillable fields #48

@JonRowe

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.

@JonRowe

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...

@JonRowe

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.

@jnicklas
Owner

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

@JonRowe

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.

@jnicklas
Owner

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

@JonRowe

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...

@jnicklas jnicklas and 1 other commented on an outdated diff Feb 15, 2013
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
@jnicklas
jnicklas Feb 15, 2013

why the :contenteditable metadata?

@JonRowe
JonRowe Feb 15, 2013
@jnicklas
jnicklas Feb 15, 2013

That would be excellent :)

@JonRowe

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

@jnicklas
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?

@JonRowe

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...

@twalpole
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

@JonRowe

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

@jnicklas
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.

@twalpole
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)

@jnicklas
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.

@JonRowe

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 :/)

@jnicklas
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.

@JonRowe

👍 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 :)

@jnicklas
Owner

Yes, 1.x still support 1.8.7.

@JonRowe

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.

@jnicklas
Owner

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

@JonRowe

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.

@jarib

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.

@JonRowe

@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).

@jnicklas
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.

@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.

@JonRowe What browser are you testing this in?

@JonRowe

@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.

@JonRowe

@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...

@jnicklas
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.

@abotalov
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

@jnicklas
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.

@JonRowe

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.

@twalpole
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

@JonRowe
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?

@twalpole
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).

@JonRowe

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.

@jarib

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.

@twalpole
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

@JonRowe

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?

@jeromegn

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

@JonRowe

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.

@h3h
h3h commented Aug 3, 2013

+1 for this feature.

@jondkinney

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

@IamNaN

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.

@samccone

+1 for this

@jnicklas jnicklas closed this in 3f5180f Oct 20, 2013
@jnicklas
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.

@JonRowe

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

@samccone

huzzah!

@route route referenced this pull request in teampoltergeist/poltergeist Dec 1, 2013
Merged

Update capybara dependency #416

@JonRowe JonRowe deleted the JonRowe:content_editable branch Apr 30, 2014
@JonRowe JonRowe referenced this pull request in teampoltergeist/poltergeist Apr 30, 2014
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