Add utility classes to interact with checkboxes and radio buttons #545

dunglas · 2018-03-13T16:34:03Z

This PR adds an utility class to easily manipulate checkboxes and radio buttons.

Imagine the following form:

<form id="my-form" method="POST">
    <input type="checkbox" name="c" value="1">

    <label>
        Checkbox Foo

        <input type="checkbox" name="c" value="2">
    </label>

    <input type="radio" name="r" value="x">

    <label>
        Radio Foo

        <input type="radio" name="r" value="y">
    </label>

    <input type="submit" value="OK">
</form>

<label for="c1">Checkbox Bar</label>
<input type="checkbox" form="my-form" name="c" value="3" id="c1">

<label for="r1">Radio Bar</label>
<input type="radio" form="my-form" name="r" value="z" id="r1">

The proposed class allows to interact with it in a smooth way:

$checkbox = new WebDriverCheckbox($this->driver->findElement(WebDriverBy::xpath('//input[@type="checkbox"]'))); // select any checkbox
$radio = new WebDriverRadio($this->driver->findElement(WebDriverBy::xpath('//input[@type="radio"]'))); // Select any radio button

var_dump($checkbox->getOptions()); // 1, 2, 3
var_dump($radio->getOptions()); // x, y, z

$checkbox->selectByValue('1');
$checkbox->selectByVisibleText('Checkbox Foo');
$checkbox->selectByVisiblePartialText('Bar');
$checkbox->deselectByVisiblePartialText('Bar');

$radio->selectByValue('a');
$radio->selectByIndex(1);

var_dump($checkbox->getAllSelectedOptions()); // 1, 2
var_dump($radio->getAllSelectedOptions()); // b

var_dump($radio->getFirstSelectedOption()); // b

$checkbox->deselectAll());

Features:

Checkbox and radio buttons support
Ability to select an element by its label (full or partial matching)
Ability to select an element by its (pseudo) 0-based index

For consistency and convenience - especially when using libraries such as Symfony Form that allow to switch from checkboxes to selects in 1 line - the new class mimics the public API of WebDriverSelect and implements the WebDriverSelectInterface.

Closes #373.

dunglas · 2018-03-13T16:48:39Z

lib/WebDriverCheckbox.php

+                }
+
+                try {
+                    // Since the mechanism of getting the text in xpath is not the same as


I'm not sure that this case really occurs. But as the WebDriverSelect already does that, I've kept this behavior.

OndraM

Hi @dunglas , this is such a great contribution! I've done some more in-depth code-review, hope you don't mind :-). Would be fantastic if you could have look at it.

Thanks again!

OndraM · 2018-03-14T13:51:06Z

lib/WebDriverCheckbox.php

+/**
+ * Provides helper methods for checkboxes and radio buttons.
+ *
+ * @author Kévin Dunglas <dunglas@gmail.com>


We don't use these @author annotation in other parts of php-webdriver :).

OndraM · 2018-03-14T13:51:48Z

lib/WebDriverCheckbox.php

+
+    public function __construct(WebDriverElement $element)
+    {
+        if ('input' !== $tagName = $element->getTagName()) {


Could you please swap all of yoda conditions used (here and on all other places)? They are not used in the codebase and are IMO making the code harder to read.

OndraM · 2018-03-14T13:56:05Z

tests/functional/WebDriverCheckboxTest.php

+    }
+
+    /**
+     * @expectedException \Facebook\WebDriver\Exception\NoSuchElementException


Could you please change this annotation (here and below) to $this->expectException(NoSuchElementException::class) and also add expectedExceptionMessage (both placed on the lines right before the exception should occur)?

The expectedException annotations are now not considered as being the best practice in PHPUnit.

OndraM · 2018-03-14T13:58:32Z

lib/WebDriverCheckbox.php

+
+            $xpath = 'ancestor::label';
+            $xpathNormalize = sprintf('%s[%s]', $xpath, $normalizeFilter);
+            if (null !== $id = $element->getAttribute('id')) {


This kind of conditions combined with variable assigning is really hard to read - could you please expand them (also on other places)? I know it is longer, but also much more readable IMO.

OndraM · 2018-03-14T16:41:25Z

lib/WebDriverCheckbox.php

+        return $this->getRelatedElements();
+    }
+
+    public function getAllSelectedOptions()


Hmm, look like the interface is a bit too specific. I may change it to something like WebDriverSelectableIterface, and rename this to getAllSelected or something like that in the future - because the keyword "option" is related to <option> tags in selects :].

For now I suggest at least renaming the variable(s) to $selected or $selectedElements so that thet don't suggest you are dealing with <options>. What do you think?

I definitely agree, the interface is too specific, but it's convenient to have a common one. Introducing the new one can even be done (in another PR?) without breaking changes.

OndraM · 2018-03-14T16:42:07Z

lib/WebDriverCheckbox.php

+            }
+        }
+
+        throw new NoSuchElementException('No options are selected');


Maybe rather No checkboxes are selected? (supposing the class is split for radios and checkboxes)

OndraM · 2018-03-14T16:51:38Z

lib/WebDriverCheckbox.php

+            }
+
+            try {
+                $element->findElement(WebDriverBy::xpath($xpathNormalize));


Could you please add some short comment about the logic there? (Why first trying to find element this way and why in different way inside the catch block?)

See #545 (comment)
It mimics the behavior of WebDriverSelect but I don't even know if it's really necessary. I propose to remove this for now, and we'll add it back if someone reports an issue.

OndraM · 2018-03-14T17:02:13Z

lib/WebDriverCheckbox.php

+ *
+ * @author Kévin Dunglas <dunglas@gmail.com>
+ */
+class WebDriverCheckbox implements WebDriverSelectInterface


I suggest splitting the class in two (Checkbox and Radio), because:

the semantic is confusing ($radiobutton = new WebDriverCheckbox($radibuttonElement))

there are lot of $this->isMultiple() conditionals, which separates logic for checkboxes and radiobuttons

Maybe some common abstract class could be introduced to DRY the logics. What do you think?

OndraM · 2018-03-14T17:09:02Z

tests/functional/web/form_checkbox_radio.html

+    <input type="checkbox" name="j2" value="j2a">
+
+    <label>
+        J2B


To make sure the normalize-space() selectors work as expected, there could also be a case where the visible text has multiple spaces inside, ie. not being just one word, like:

<label> J 2 B ...

What are you thoughts?

OndraM · 2018-03-14T17:46:36Z

lib/WebDriverCheckbox.php

+        }
+
+        if (null === $name = $element->getAttribute('name')) {
+            throw new WebDriverException('The input have a "name" attribute.');


does not have?

dunglas · 2018-03-14T22:59:40Z

@OndraM all comments fixed.
CS differences was because I followed Symfony ones.

OndraM · 2018-03-28T07:47:03Z

@dunglas Sorry for delays, I will review this soon.

OndraM

I tried it on some webpages, it looks really great! Just a few remaining suggestions from me.

Thank you very much :)

OndraM · 2018-04-13T13:02:46Z