-
Notifications
You must be signed in to change notification settings - Fork 117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for issue #65 #66
Conversation
// element must be <textarea> or <input> with type text, password, email etc | ||
// See https://github.com/yandex-qatools/htmlelements/issues/65 | ||
private static String getClearTextInputElementCharSequence(WebElement element) { | ||
return StringUtils.repeat(Keys.DELETE.toString() + Keys.BACK_SPACE, element.getText().length()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you call .toString()
method?
I believe if we call it, then it needs to be done for BACK_SPACE key as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't compile without .toString()
beause Keys is an enum.
Keys.BACK_SPACE
is implicitly converted to string.
http://stackoverflow.com/questions/328661/explicit-vs-implicit-call-of-tostring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for some fields element.getText()
will return empty string when element.getAttribute("value")
will return not empty result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case we need to replace element.getText().length()
with Math.max(element.getText().length(), element.getAttribute("value"))
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I finally see, why there is a problem. In the original code, that was used to create that PR: https://github.com/minkphp/MinkSelenium2Driver/blob/master/src/Selenium2Driver.php#L659-L662 we're:
- operation only on
input
andtextarea
elements - always taking the
value
attribute value (theattribute
method in selenium in fact looks in JS.value
property on DOM node and gets it's value, rather then HTML attributevalue
, that doesn't exist on textarea)
And using getText
here is a huge mistake, because https://code.google.com/p/selenium/wiki/JsonWireProtocol#/session/:sessionId/element/:id/text it returns visible text on element (e.g. div and such), but not inputs :)
Issue numbers mentioned in PR title doesn't auto-link PR with corresponding issue. Usually I put this into description: Related to #65 |
Will this pull request be merged? |
Looks good, can you add some tests to document new behavior? |
well done! |
Aren't there any universal method for getting element value? |
I thought |
} | ||
} | ||
|
||
// Returns sequence of backspaces and deletes that will clear element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for future prs :) - use javadoc style comments on methods - \** */
Related to #65