CheckBox::getLabel finds wrong label

[aik099]

Method getLabel of CheckBox class uses following xpath following-sibling::label to find label, associated with a checkbox. However in most cases this would find wrong label, that even don't belong to that checkbox.

Sample problematic markup:

<input type="checkbox" id="checkbox-without-label">
<input type="checkbox" id="checkbox-with-label"><label for="checkbox-with-label">Checkbox Label</label>

I think, that we either should find the "label" that placed right after a checkbox or use "for" attribute of label to match with "id" attribute of checkbox.

Of course following case still won't be covered:

<label><input type="checkbox"> label text</label>

[proxeter]

@aik099, i'll do this

[aik099]

You can see how I solved this in my HtmlElements PHP fork: https://github.com/aik099/qa-tools/blob/75f6712360ac97467d2f2379a2fc2f2e64aab6c7/library/aik099/QATools/HtmlElements/Element/LabeledElement.php#L27-L49

[proxeter]

@aik099, what will return this line?
$escaped_id = $this->getSelectorsHandler()->xpathLiteral($id);

[aik099]

It escapes a string so it can be safely injected in XPATH expression. Here it's contents: https://github.com/Behat/Mink/blob/055da154b9fcb0bb238332adda3c4b38210c85d3/src/Behat/Mink/Selector/SelectorsHandler.php#L116-L140

For example test becomes 'test'. And test | fail will return 'test \| fail'.

[proxeter]

We have this 3 situations:

1. Label with matching "for" attribute (http://prntscr.com/3cr9u2)
2. Label wrapped around checkbox (http://prntscr.com/3crf2q)
3. Label right next to checkbox (http://prntscr.com/3crg8k)

@aik099, your code a little issue in second variant have because result of this code will be:

<label>
  <input type="checkbox" />
  Some text
</label>

but i don't see a problem here. What do you think about?

[aik099]

What these screenshots show? I see XPATH and XML, but no result that was matched.

And I've tested that (have unit tests) to verify that correct elements are indeed found in each of 3 cases.

[proxeter]

@aik099, i mean that you receive element with child nodes. Can it be problem at this place in your realization?
And i have one improvement to your code:

$label = $this->getContainer()->find('xpath', 'descendant-or-self::label[@for = ' . $escaped_id . ']');

to

$label = $this->getWrapperElement()->find('xpath', '//descendant-or-self::label[@for = ' . $escaped_id . ']');

What do you think about?

[aik099]

If I understand xpath correctly then your change allows to locate the labels across all page and only ones that are located directly after the checkbox. If so, then you're welcome to send a PR. Don't forget to update the tests.

Thanks for your idea.

[proxeter]

@aik099, yes we will try to find element at all of page. But we have a little problem here (http://prntscr.com/3d7v9d)
If user add two label elements with equal "for" attribute we will take array of Label elements. Can we assume collision to find only the first label element? (http://prntscr.com/3d7vto)

[aik099]

If user add two label elements with equal "for" attribute we will take array of Label elements.

That is allowed in HTML and is valid?

Can we assume collision to find only the first label element? (http://prntscr.com/3d7vto)

Absolutely.

[proxeter]

@aik099, this image will answer your question (http://pikatomsk.ru/Content/dynamic/pics/14338/other/1.jpg)
And i know very much programmers which need to use this beautiful item.
I'm trying to secure code from them =)

OK. In result code will be:

$label = $this->getWrapperElement()->find('xpath', '(//label[@for = ' . $escaped_id . '])[1]');

I'll send PR to your repository and this issue today's evening or night

[aik099]

One more [1] to the xpaths used in the getLabel method. :)

[proxeter]

@aik099, i'm trying to rewrite your php code of method xpathLiteral to java language but have a question. I can't get result as you asked me: "For example test becomes 'test'. And test | fail will return 'test \| fail'."

Look to the screenshot: http://yadi.sk/d/TDjVJbwkNEtnz
What i'm doing wrong?

[aik099]

Doesn't Selenium Java already have method for escaping Xpath?

Have you tried to run that xpathLiteral PHP version first to see what it does in reality? I suspect that there should be \| but maybe xpathLiteral doesn't encode it like that. Maybe it places everything in single quotes and that's all.

[proxeter]

@aik099, i've run your php code which you give to me and not take expected result. Are you sure that function xpathLiteral working correctly?

[aik099]

Are you sure that function xpathLiteral working correctly?

Define correctly. I don't know what it should do, but I do know that if I feed a valid xpath into it and place result into another xpath, then it won't break it.