[backport] Add JavaScript tests for Form API #states: required, visible, invisible, expanded, checked, unchecked

Nice!

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
@@ -0,0 +1,48 @@
+    // This should make the element visible but it is already.
+    $driver->check($xpath);
+    $this->assertElementNotVisible('#edit-name', 'Name field is not visible.');

That comment is a bit weird.

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,110 @@
   +    $this->assertElementVisible('#edit-name', 'Name field is not visible.');
   

   not visible » visible

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,110 @@
   +//    $driver->setValue(CssSelector::toXPath('#edit-more-info'), '');
   +//    $this->assertFalse($driver->isChecked(CssSelector::toXPath('#edit-info-provide')), '#edit-info-provide is not checked');
   

   Did you try set value NULL or FALSE?

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
@@ -0,0 +1,110 @@
+  public function testJavascriptStates() {

I'm wondering how we could organize the test to have more of a structured approach to all the different kind of cases of tests

I'm wondering how we could organize the test to have more of a structured approach to all the different kind of cases of tests

Yeah, I wondered the same thing while reading the patch.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
@@ -0,0 +1,110 @@
+ * Tests if we can execute JavaScript in the browser.

Probably wants an actual docblock. :)

Related #994360: #states cannot check/uncheck checkboxes elements

• Fixed the failing tests. Pro tip: some time we have to wait for browser to do its work /HT: @larowlan
• Updated the docs.
• Updated the usage of a deprecated CssSelector.

Can we move #9 to follow up it is not really this issues problem we can discuss it in dedicated issue?

1. +++ b/core/includes/common.inc
   @@ -568,7 +568,7 @@ function drupal_js_defaults($data = NULL) {
   - * @see form_example_states_form()
   

   This doesn't exist in core not even in D7. I don't know why we added this ref in #767738: Rewrite drupal_process_states() documentation, deal with action of 'required' for #states.

2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -44,6 +45,7 @@
   +  use AssertContentTrait;
   

   I don't think this change is out of scope here.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -92,12 +86,13 @@ public function testJavascriptStates() {
   +    $this->assertJsCondition('!jQuery("#edit-more-info").val()');
   

   This is normal because browser are always slow to react. For behat we are used to of adding wait.

Is this really necessary? I don't think you have to wait for this... if waiting is necessary then we need to wait for #edit-info-provide to be unchecked.

Well, the interaction is not instant but async, but I cannot imagine that for states we ever do the communication with phantomjs fast enough, compared to phantomjs unchecking/checking a checkbox.

Thanks @alexpott for the review. From #14

• Fixed.
• Removed.
• Removed.
• This is a green patch so feel free to bikeshed on it. :D

In this patch

• Used page instead of driver. I think this makes more sense because of better error handling.
• Used fillField, uncheckField and checkField because these show what we are doing on the page.
• Used WebAssert instead of simple true false asserts because of better error handling.
• Empty and filled states are checked on keyup event and MinkDriver doesn't trigger keyup when the fields are cleared so I manually triggered that and fixed the fail.

1. +++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
   @@ -0,0 +1,273 @@
   +    $form['name'] = array(
   

   Do you mind using short array syntax?

2. +++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
   @@ -0,0 +1,273 @@
   +        'high_school'   => $this->t('High School'),
   +        'undergraduate' => $this->t('Undergraduate'),
   +        'graduate'      => $this->t('Graduate'),
   

   Can we avoid this indentation? Its just weird ...

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,105 @@
   +    // High school form testing.
   +    $this->click('#edit-student-type-high-school');
   +    $this->assertElementVisible('#edit-high-school');
   +    $this->assertElementNotVisible('#edit-undergraduate');
   +    $this->assertElementNotVisible('#edit-graduate');
   +    $this->assertElementVisible('#edit-average');
   

   So this is the thing, while this is a nice example from the example module, it makes it hard to follow the test. In case we would name things after what we test there, it would be possible to actually read the test without figuring the entire form out first. Just for example take 'Feedback testing', this comment doesn't help you at all of what we test, or right we test a textarea.

Thanks for awesome review @dawehner.

1. Fixed.
2. Fixed.
3. Well this was a huge task but I was able to convert it in a readable format. I have to add methods to \Drupal\simpletest\WebAssert but now this is perfectly readable imo. I don't know how much of this is out of scope here but I have updated \Drupal\Tests\toolbar\FunctionalJavascript\ToolbarIntegrationTest as well and cleaned up BTB.

In this patch

• Added WebAssert::assertFieldVisible(), WebAssert::assertFieldNotVisible(), WebAssert::assertElementVisible() and WebAssert::assertElementNotVisible() to \Drupal\simpletest\WebAssert
• Fixed doc issues in JavascriptStatesForm
• Modernized the ToolbarIntegrationTest
• Fixed doc issues in JavascriptStatesTest
• Removed the obsolete methods form JavascriptTestBase
• Updated JavascriptTestBase::assertJsCondition() to use session wait function instead of driver.
• Removed the obsolete methods form BrowserTestBase

FWIW I used \Behat\MinkExtension\Context\MinkContext as a reference for these changes.

@jibran
I'm sorry but I fear you didn't got my point. My point was that the test form is not really explicit about what its testing about. For example the feedback part of the form, is about testing 'textarea' states, so it should be named afterward

On top of that we should avoid putting too many custom stuff into your testing framework, but rather use what mink already provides for us, see https://github.com/Behat/Mink/blob/master/src/Behat/Mink/WebAssert.php
So yeah I think we should have an issue about using that. Also additional cleanup you were doing IMHO belongs into its own issue.

What I love about this test/these changes are the readability.

We are nearing the fluency of cucumber, I would argue a non-technical user could get a fair idea of what was being tested. And we're using domain language - there are very few css selectors left, which carry no meaning.

This is the first big javascript test in core, and also one of the biggest BTB new generation tests.

I think it is important to set the standard/bar high in terms of understanding.

I would bet that anyone new to testing would much prefer the format of the states test to your average simpletest.

Great work here @jibran

1. +++ b/core/modules/simpletest/src/WebAssert.php
   @@ -52,16 +56,149 @@ public function buttonExists($button, TraversableElement $container = NULL) {
   +  public function assertFieldVisible($locator, TraversableElement $container = NULL) {
   ...
   +  public function assertFieldNotVisible($locator, TraversableElement $container = NULL) {
   

   I think there is merit in assertField{Not}VisibleByName methods - would simplify the tests below (lots of 'named' calls). Ok for follow up

2. +++ b/core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php
   @@ -23,30 +23,29 @@ public function testToolbarToggling() {
   +    $content = $page->findLink('Content');
   +    $this->assertTrue($content->isVisible(), 'Toolbar tray is open by default.');
   +    $page->clickLink('Manage');
   +    $this->assertFalse($content->isVisible(), 'Toolbar tray is closed after clicking the "Manage" link.');
   +    $page->clickLink('Manage');
   +    $this->assertTrue($content->isVisible(), 'Toolbar tray is visible again after clicking the "Manage" button a second time.');
   

   This reads much better now

3. +++ b/core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php
   @@ -23,30 +23,29 @@ public function testToolbarToggling() {
   +    $page->pressButton('Vertical orientation');
   ...
   -    $this->click('#toolbar-item-administration-tray button.toolbar-icon-toggle-vertical');
   

   <3 so much better

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,109 @@
   +    $this->assertSession()->assertFieldNotVisible('Name');
   

   you could put $this->assertSession() in a local variable, and save the myriad of calls.

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,109 @@
   +    $this->assertSession()->assertElementNotVisible('named', ['fieldset', 'High School Information']);
   

   Merit in a assertFieldsetIsVisible method? follow up ok

6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,109 @@
   +    $page->selectFieldOption('How many years have you completed?', '5');
   ...
   +    $page->findById('edit-student-type-graduate')->selectOption('graduate');
   

   selectFieldOption reads much nicer than findById->selectOption

7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,109 @@
   +    $page->findField('Please describe your graduate studies')->keyUp(8);
   

   We should use a constant here

Modernized the ToolbarIntegrationTest

Created #2711963: Modernized ToolbarIntegrationTest for that.

@jibran I think @dawehner didn't mean that. I think he meant the labels on the form. Things like 'High School', etc... don't tell us what we're testing.

@jibran
I'm sorry but I fear you didn't got my point. My point was that the test form is not really explicit about what its testing about. For example the feedback part of the form, is about testing 'textarea' states, so it should be named afterward

Ok, I completely misunderstood that point :D

On top of that we should avoid putting too many custom stuff into your testing framework

I disagree. If we ever want to fix #1567500: [meta] Pave the way to replace the testing framework with PHPUnit and possibly rewrite Simpletest then we have to bring BTB/JTB more inline with our simpletest kind of asserts. I think we should add wrapper functions in \Drupal\simpletest\WebAssert so that we can have feature parity with WTB. And as I said before in #22 these changes exit in MinkContext and DrupalContext so this is not totally new stuff for regular behat users.

@larowlan thank for the review.

1. Let's create a follow up and keep the discussion alive in new issue.
2. Yeah, I know. Moved to #2711963: Modernized ToolbarIntegrationTest.
3. Oh yeah!
4. Done.
5. Yeah follow up is fine.
6. Fixed.
7. FIxed.

Lee also suggested we should move \Drupal\simpletest\WebAssert out of simpletest \Drupal\simpletest\BrowserTestBase.
In this patch

• Cleaned up \Drupal\simpletest\WebAssert.
• Added new methods to \Drupal\simpletest\WebAssert.
• Reverted changes of ToolbarIntegrationTest.
• Reverted JavascriptTestBase.
• Reverted BrowserTestBase.

On top of that we should avoid putting too many custom stuff into your testing framework, but rather use what mink already provides for us

+1

A significant reason why SimpleTest is painful to use, is because there's dozens and dozens and dozens of assert methods. Too many to chose from.

I think it'd be better to initially have all these new assert methods be in this particular test class. Once we see the need arise elsewhere, we can choose to put it in its own trait. And eventually we could chose to move it into Drupal's WebAssert itself.

i.e., at some time in the future:

1. +++ b/core/modules/simpletest/src/WebAssert.php
   @@ -52,16 +56,152 @@ public function buttonExists($button, TraversableElement $container = NULL) {
   +  public function assertFieldVisible($locator, TraversableElement $container = NULL) {
   ...
   +  public function assertFieldNotVisible($locator, TraversableElement $container = NULL) {
   ...
   +  public function assertFieldsetVisible($locator, TraversableElement $container = NULL) {
   ...
   +  public function assertFieldsetNotVisible($locator, TraversableElement $container = NULL) {
   

   I think these could fit in a FormWebAssert — many tests don't have forms, and these are just distracting.

2. +++ b/core/modules/simpletest/src/WebAssert.php
   @@ -52,16 +56,152 @@ public function buttonExists($button, TraversableElement $container = NULL) {
   +  public function assertElementVisible($selector_type, $selector, ElementInterface $container = NULL) {
   ...
   +  public function assertElementNotVisible($selector_type, $selector, ElementInterface $container = NULL) {
   ...
   +  protected function assertElement($condition, $message, Element $element) {
   

   These feel more universally useful and could potentially be moved into WebAssert at some point.

   But… I'm not even sure we really need these. It feels like overengineering at this time. Why can't these just use Mink's assertion methods directly?

Just reroll after #2702281: Move Drupal\simpletest\RandomGeneratorTrait, Drupal\simpletest\WebAssert and Drupal\simpletest\SessionTestTrait into Drupal\Tests namespace.

I had started to work on this on another issue (didn't find this one, thanks @dawehner for closing as duplicate) : #2708823: Test form #states

I was thinking about testing all states on all field types because I remember that some of them behave unexpectedly (see #994360: #states cannot check/uncheck checkboxes elements).
Even if I didn't progress a lot, I found out that checkboxes, managed_file, password_confirm and radios elements did not work as expected with some common states (optional/required, visible/invisible and enabled/disabled).

That JS code is highly dependant of the html structure of the fields, so I really think we should test all combinations on all elements.

We should try to keep this running again :)

Discussed moving this forward with @dawehner and @DuaelFr at DevDays.

We agreed that for this to move forward we need to address the feedback from @dawehner in #20/#23/#25 about the form elements needing to be more descriptive in what they are there to test for ('textarea_dependent_on_selector', along those lines), instead of having a nice real world example.

We also agreed that currently we don't want the extra assertion methods (as per @dawehner in #25 and @Wim Leers in #30), but the patch should look more along the lines of how it did in #19. We felt it's better to just get some javascript tests in and later decide if certain patterns are evident in the existing tests and only then add additional assertion methods, over adding methods that might not be used but we can't remove anymore because they might be used by contrib.
Easier to add things later then removing them and having to deal with BC stuff.

@DuaelFr said he was going to work on this, so feedback on these ramblings would be great and awesome work so far!

I'm working on this.

Here is my progress.
I'd need some help to figure out why the last assertions are failing. (See the @todo comment)
I'd also need to be comforted about the fact I didn't lost myself in translation too ;)

I discussed about this patch with @Lendude this morning (thanks!).
I'm going to rewrite it to make it more understandable.
New patch coming soon.

As we are seeing in #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) we need a lot of new/old assert methods so I still think we should keep the new assert methods.

+++ b/core/modules/simpletest/src/WebAssert.php
@@ -52,16 +56,152 @@ public function buttonExists($button, TraversableElement $container = NULL) {
+  public function assertFieldVisible($locator, TraversableElement $container = NULL) {
...
+  public function assertFieldNotVisible($locator, TraversableElement $container = NULL) {
...
+  public function assertFieldsetVisible($locator, TraversableElement $container = NULL) {
...
+  public function assertFieldsetNotVisible($locator, TraversableElement $container = NULL) {
...
+  public function assertElementVisible($selector_type, $selector, ElementInterface $container = NULL) {
...
+  public function assertElementNotVisible($selector_type, $selector, ElementInterface $container = NULL) {
...
+  protected function assertElement($condition, $message, Element $element) {
...
+  protected function getMatchingElementRepresentation($selector_type, $selector, $plural = FALSE) {

These all can now go into JsWebAssert or a AssertFieldTrait or AssertJSFieldTrait and then only this test will have to use the trait.

I'm going to rewrite it to make it more understandable.

I don't think this is necessary at all If you can't make sense of the form then just enable the module and visit the page. Test will be more understandable if we'll add new assert methods.

IMO this patch is now going in a wrong direction. I still think #31 is the way forward but if no one agrees with me then I'll keep mum about it.

I'm going to rewrite it to make it more understandable.

I don't think this is necessary at all If you can't make sense of the form then just enable the module and visit the page. Test will be more understandable if we'll add new assert methods.

Too late. Here is my patch.

@Lendude and @dawehner agreed it was too early to add a bunch of new assertions as, for tests, we are more in a mood where we do things then, when we'll have enough tests, we'll think about refactoring. Providing many assertions right now means they are going to be there forever because we won't want to break contrib tests that should use them.

1. +++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
   @@ -0,0 +1,183 @@
   +  public function buildForm(array $form, FormStateInterface $form_state) {
   

   This form is understandable now but not intuitive imho.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,260 @@
   +    $this->_testCheckboxTriggeredElements($page);
   +    $this->_testTextfieldTriggeredElements($page);
   +    $this->_testRadiosTriggeredElements($page);
   +    $this->_testCheckboxesTriggeredElements($page);
   +    $this->_testSelectTriggeredElements($page);
   +    $this->_testMultipleTriggeredElements($page);
   ...
   +    $this->assertTrue($page->findField('textfield_invisible_when_checkbox_trigger_checked')->isVisible());
   ...
   +    $this->assertFalse($page->findField('textfield_in_details')->isVisible());
   ...
   +    $this->assertFalse($page->findField('textfield_invisible_when_checkbox_trigger_checked')->isVisible());
   ...
   +    $this->assertTrue($page->findField('textfield_in_details')->isVisible());
   

   This is exactly the stuff we used to do in WTB. What is the use of introducing BTB if we are not going to use it properly? i.e The way user interact with browser. This imo is just a WTB adapting JTB. I'm sorry but we are not setting a good example for adaption of BTB or JTB for rest of the core or contrib.

when we'll have enough tests, we'll think about refactoring.

It means we are allowing ourselves to continue practicing old WTB testing habits.

Providing many assertions right now means they are going to be there forever

Yes, that is exactly the intention here so that we'll adapt the browser testing, the way it suppose to be done, with interacting actual browser.

@jibran
Thank you for looking at the quite big changes done by @DuaelFr. I totally agree with you that we should discuss about adding a lot of assertions vs. not adding a lot of assertions. Let's do that here: #2755617: Add assertions for javascript testing

This form is understandable now but not intuitive imho.

Well, if you just enable the test module and look at it, its equally clear, from just looking at it. On the other hand, when you actually would have to debug this test, you would see a difference:

+
+    $form['student_type'] = [
+      '#type' => 'radios',
+      '#options' => [
+        'high_school' => $this->t('High School'),
+        'undergraduate' => $this->t('Undergraduate'),
+        'graduate' => $this->t('Graduate'),
+      ],
+      '#title' => $this->t('What type of student are you?'),
+    ];
+    $form['high_school'] = [
+      '#type' => 'fieldset',
+      '#title' => $this->t('High School Information'),
+      // This #states rule says that the "high school" fieldset should only
+      // be shown if the "student_type" form element is set to "High School".
+      '#states' => [
+        'visible' => [
+          ':input[name="student_type"]' => ['value' => 'high_school'],
+        ],
+      ],
+    ];

vs.

+    $form['radios_trigger'] = [
+      '#type' => 'radios',
+      '#title' => 'Radios trigger',
+      '#options' => [
+        'value1' => 'Value 1',
+        'value2' => 'Value 2',
+        'value3' => 'Value 3',
+      ],
+    ];
....
   $form['fieldset_visible_when_radios_trigger_has_given_value'] = [
+      '#type' => 'fieldset',
+      '#title' => 'Fieldset visible when radio trigger has given value',
+      '#states' => [
+        'visible' => [
+          ':input[name="radios_trigger"]' => ['value' => 'value2'],
+        ],
+      ],
+    ];

@jibran Do you understand where we are coming from?

It is a very good idea to create #2755617: Add assertions for javascript testing so thank you for that.

On the other hand, when you actually would have to debug this test, you would see a difference

It is just a personal preference. I can live with this change.

+    $this->_testCheckboxTriggeredElements($page);
+    $this->_testTextfieldTriggeredElements($page);
+    $this->_testRadiosTriggeredElements($page);
+    $this->_testCheckboxesTriggeredElements($page);
+    $this->_testSelectTriggeredElements($page);
+    $this->_testMultipleTriggeredElements($page);

But can we please remove this? It can be a big test but if we want to split it in different methods then let's make multiple test methods.

But can we please remove this? It can be a big test but if we want to split it in different methods then let's make multiple test methods.

Well yeah its slower, and well, things share certainly a common setup. I don't care to be honest.

I made real test cases so core commiters can choose what they prefer between this patch and #40

This test is incredible well documented!

We want to check all the states.

The following states may be applied to an element:
- enabled
- disabled
- required
- optional
- visible
- invisible
- checked
- unchecked
- expanded
- collapsed
- relevant
- irrelevant
- valid
- invalid
- touched
- untouched
- readwrite
- readonly

Here are some doc improvements and added helper methods in JSWebAssert and JavascriptStatesTest

We want to check all the states.

While I agree with you on the point on we should do it, there is no requirement to do it as part of one issue. You know, we can always iterate and enlarge the test coverage over time.

Reroll of #48, just a simple conflict on JSWebAssert.

Oh

Didn't apply anymore. Added the interdiff of the updated part.

Ups. wrong patch extension.

Added new tests for Required and Optional elements.
Abstracted the logic to find a node in the page. It was repeated in four methods.

Wrong patch.

Is this patch waiting to have all the test cases? @dawehner suggested in #49 we could commit this and iterate the missing tests cases in a follow up.

I think @dawehner and @dagmar are right, and more seeing that there are issues like #2700667: Notice: Undefined index: #type in Drupal\Core\Form\FormHelper::processStates() waiting for this to add their own cases.

@jibran Are you okay with adding the base here and expanding it in related issue?

There are currently a number of issues regarding states that are blocked on the core gate for testing and could benefit greatly from the tests added in this issue:
#2700667: Notice: Undefined index: #type in Drupal\Core\Form\FormHelper::processStates()
#2226405: FAPI #states: dependent element added via AJAX initializes incorrectly if dependee has been initialized earlier
#1149078: States API doesn't work with multiple select fields
#2847425: #states not affecting visibility/requirement of managed_file
#736182: Match States API required fields theming with Forms API
#994360: #states cannot check/uncheck checkboxes elements
#2820586: #states not applied to link elements

Yeah, sure. Let's create a meta and track the #47 states and all the issues mentioned in #64.

Create meta issue #2892872: [META] Add Javascript tests for all Form API's #states

Updated the title reflect the states covered.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -46,6 +48,157 @@ function isAjaxing(instance) {
+  public function assertElementVisible($selector_type, $selector, ElementInterface $container = NULL) {
...
+  public function assertElementNotVisible($selector_type, $selector, ElementInterface $container = NULL) {
...
+  public function assertElementRequired($selector_type, $selector, ElementInterface $container = NULL) {
...
+  public function assertElementOptional($selector_type, $selector, ElementInterface $container = NULL) {

Should these methods be made protected for move somewhere else and remain public?

otherwise this RTBC and would be good to commit.

Reroll of #59. Manually fixed conflict on core/modules/system/tests/modules/form_test/form_test.routing.yml.

Needs a reroll after #2995570: #states breaks when OR is used.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -46,6 +48,157 @@ function isAjaxing(instance) {
+  public function assertElementVisible($selector_type, $selector, ElementInterface $container = NULL) {
...
+  public function assertElementNotVisible($selector_type, $selector, ElementInterface $container = NULL) {

Can we replace these with assertVisibleInViewport and assertVisibleInViewport now?

rerolled

#72 is NR (and the bot is green), but we've had a long string of patches without interdiffs. A careful review is needed to make sure we didn't loose anything during the 2.5+ years since the last major re-working (#48 or so by my reading of the issue).

Agreed it'd be fantastic to get this in soon. #states is still all kinds of broken around the edges. Having all these tests would definitely help.

Thanks,
-Derek

p.s. I wrote some #states JS tests for #2419131: [PP-1] #states attribute does not work on #type datetime. Doesn't impact or conflict with this patch at all, but marking related. My tests have to do a lot of this:

+    $this->assertFalse($date_field->isVisible(), 'Date is invisible');
+    $this->assertFalse($time_field->isVisible(), 'Time is invisible');

I'm not sure it would help me to use the assertElementNotVisible() added by this patch, b/c my tests read and work better to have the $date_field hold the DOM node for both manipulation and asserting various things. So I'm kind of meh on that debate (which I now see got punted to #2755617: Add assertions for javascript testing but sort of died on the vine, as it were).

Re #71 - I don't think checking the visibility with regards to the viewport is necessary in this scenario, all we care is that the elements react to the states configuration, whether or not they're inside the viewport is irrelevant.

Re #74- I've reviewed changes since #48 and I believe we haven’t lost any coverage, rather we gained some thanks to @dagmar :)

Re #66 - Not sure what you mean there, existing methods on that class are also public... could you please expand on that?

In this interdiff:

1. Cleaned it up a bit (removed an unused use statement and corrected a docblock).
2. Moved them all to a single test, which cuts the runtime of this from more than 8 minutes to 1,5 minutes on my laptop.

Imho we should look into adding this in so we can work on adding test coverage for the rest of the #states outstanding bugs.

Anything else we should be doing here?

Added the test to the patch for #1091852-80: Display Bug when using #states (Forms API) with Ajax Request. Obviously, without that patch test should fail.

Re-uploading the last successful patch, because I posted erroneously here the test for non-committed patch.

Drive-by quick review:

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -46,6 +48,157 @@ function isAjaxing(instance) {
+   * Asserts that the specific element is not required on the current page.
+   *
+   * @param string $selector_type
+   *   The element selector type (css, xpath).
+   * @param string|array $selector
+   *   The element selector.
+   * @param \Behat\Mink\Element\ElementInterface $container
+   *   The document to check against.
+   *
+   * @throws \Behat\Mink\Exception\ElementNotFoundException
+   *   When the element doesn't exist.
+   */
+  public function assertElementRequired($selector_type, $selector, ElementInterface $container = NULL) {

The comment here should say, "is required".

Rerolled, fixed conflict on common.inc due to https://www.drupal.org/node/3000069

Also addressed #85 while I was at it. Good catch @heddn by the way.

Setting $defaultTheme to stark in the test as per https://www.drupal.org/node/3083055

Mostly looks really good, thanks! Some concerns / nits / requests for even-better-ness. ;)

1. +++ b/core/lib/Drupal/Core/Form/FormHelper.php
   @@ -198,6 +198,8 @@ protected static function processStatesArray(array &$conditions, $search, $repla
   +   *
   +   * @see \Drupal\form_test\Form\JavascriptStatesForm
   

   This is cool, although I don't remember seeing this sort of thing in core much. +1 to doing it here, and trying to do it more often (where appropriate). ;)

2. +++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
   @@ -23,6 +23,161 @@ public function getFormId() {
   +    $form['item_visible_when_select_trigger_has_given_value'] = [
   ...
   +      '#title' => 'Item visible when select trigger has given value',
   

   I love how specific the other form elements are named. Can this one be:

   'item_visible when_select_trigger_has_value2' instead of just '..._given_value'?

   Ditto the #title.

3. +++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
   @@ -23,6 +23,161 @@ public function getFormId() {
   +    $form['textfield_visible_when_select_trigger_has_given_value'] = [
   ...
   +      '#title' => 'Textfield visible when select trigger has given value',
   

   s/has_given_value/has_value3/
   ?

4. +++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
   @@ -23,6 +23,161 @@ public function getFormId() {
   +    $form['item_visible_when_select_trigger_has_given_value_and_textfield_trigger_filled'] = [
   ...
   +      '#title' => 'Item visible when select trigger has given value and textfield trigger filled',
   

   'item_visible_when_select_trigger_has_value2_and_textfield_trigger_filled'

5. +++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
   @@ -23,6 +23,161 @@ public function getFormId() {
   +    $form['textfield_visible_when_select_trigger_has_given_value_or_another'] = [
   ...
   +      '#title' => 'Textfield visible when select trigger has given value or another',
   

   textfield_visible_when_select_trigger_has_value2_or_value3
   ?

6. +++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
   @@ -23,6 +23,161 @@ public function getFormId() {
   +    $form['fieldset_visible_when_radios_trigger_has_given_value'] = [
   ...
   +      '#title' => 'Fieldset visible when radio trigger has given value',
   

   ...has_value2

7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,236 @@
   +    /* Initial state: before the checkbox trigger is checked. */
   ...
   +    /* Initial state: before the textfield is filled. */
   ...
   +    /* Initial state: before the radios is checked. */
   ...
   +    /* Initial state: before any of the checkboxes is checked. */
   ...
   +    /* Initial state: before any option of the select box is selected. */
   

   Not clear why all these are using /* foo */ instead of // foo.

   If the idea is to visually break up this huge test case, it seems to make more sense to distinguish comments like these:

   /* Test states of elements triggered by a checkbox element. /*

   (At the start of each block of code + assertions).

8. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,236 @@
   +    // Test that the details element is collapsed so the textfield inside is not
   +    // visible.
   +    $this->assertFieldNotVisible('textfield_in_details');
   

   Should we xpath() to the details itself and check that it's closed, instead of relying on the visibility of something inside of? Could be other reasons the inner thing is invisible, right?

9. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,236 @@
   +    // We need to send a backspace hit to trigger JS events.
   +    $trigger->keyUp(8);
   

   Whacky. Thanks for the comment!

10. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -0,0 +1,236 @@
    +    // Test that the fieldset element is not visible so the textfield inside is
    +    // not visible either.
    +    $this->assertFieldNotVisible('textfield_in_fieldset');
    

    As above, can we also find the fieldset element itself and check its attributes?

11. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -0,0 +1,236 @@
    +    // Test states of elements triggered by a more than one element.
    

    /by a more/by more/

12. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -0,0 +1,236 @@
    +    $selectTrigger = $page->findField('select_trigger');
    +    $textfieldTrigger = $page->findField('textfield_trigger');
    

    These local vars shouldn't be camelCase:

    $select_trigger
    $textfield_trigger

13. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -0,0 +1,236 @@
    +    // Change back to the initial state to avoid issues running the next tests.
    +    $selectTrigger->setValue('_none');
    +    $textfieldTrigger->setValue('');
    +    // We need to send a backspace hit to trigger JS events.
    +    $textfieldTrigger->keyUp(8);
    

    Seems good to clean up after ourselves, even though this is the final set of assertions, to be defensive for if/when we add more to this test.

14. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -0,0 +1,236 @@
    +  public function assertFieldVisible($locator, TraversableElement $container = NULL) {
    ...
    +  public function assertFieldNotVisible($locator, TraversableElement $container = NULL) {
    ...
    +  public function assertFieldRequired($locator, TraversableElement $container = NULL) {
    ...
    +  public function assertFieldOptional($locator, TraversableElement $container = NULL) {
    

    Not clear why these lovely assertion methods live directly in this test. Why not add them to JSWebAssert.php like the other new helpers?

    If they're going to remain in the test, they should probably be protected.

    Oh, reading up in the comment thread, I see @tedbow said the same at #66. See also #2755617: Add assertions for javascript testing. Let's move all these to be public in JSWebAssert.php here and close 2755617 as a dup. Cool?

15. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -46,6 +48,157 @@ function isAjaxing(instance) {
    +   *   The element selector type (css, xpath).
    

    All the callers in this patch use 'named' as the value for this. Shouldn't we document that as an available option here?

    "The element selector type (css|xpath|named)." or something?

16. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -46,6 +48,157 @@ function isAjaxing(instance) {
    +   * @param string|array $selector
    ...
    +   * @param string|array $selector
    ...
    +   * @param string|array $selector
    ...
    +   * @param string|array $selector
    

    I know core committers tend to not like 'array' as a @param type hint. We can almost always be more specific than that. E.g. string[] or whatever. I'm not 100% clear what kind of array you can use as a selector for this, so someone who knows better should say.

17. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -46,6 +48,157 @@ function isAjaxing(instance) {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function assertElement($condition, $message, Element $element) {
    +    if ($condition) {
    +      return;
    +    }
    +
    +    throw new ElementHtmlException($message, $this->session->getDriver(), $element);
    +  }
    

    A) Not sure what this is inheriting docs from. All I can find with grep is vendor/behat/mink/src/WebAssert.php:

    private function assertElement(...)
    

    I don't think that's legit to try to inherit docs from as a private method. I could be wrong.

    B) Too bad we need to duplicate this entire function. But I guess that's the hell we're in since everything in phpunit is private and they don't make it too easy to extend like this. :(

18. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -46,6 +48,157 @@ function isAjaxing(instance) {
    +   * @param string $selector
    +   *   The selector engine name. See ElementInterface::findAll() for the
    +   *   supported selectors.
    

    A) 🤔 Everything else calls this "The selector". What's a "selector engine name"?

    B) Don't we want a formal @see for this, and shouldn't it be fully qualified?

19. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -46,6 +48,157 @@ function isAjaxing(instance) {
    +   *   The node element if exists in the current container.
    

    s/if exists/if it exists/

20. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -46,6 +48,157 @@ function isAjaxing(instance) {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function getMatchingElementRepresentation($selector_type, $selector, $plural = FALSE) {
    +    $pluralization = $plural ? 's' : '';
    +
    +    if (in_array($selector_type, ['named', 'named_exact', 'named_partial'])
    +      && is_array($selector) && 2 === count($selector)
    +    ) {
    +      return sprintf('%s%s matching locator "%s"', $selector[0], $pluralization, $selector[1]);
    +    }
    +
    +    if (is_array($selector)) {
    +      $selector = implode(' ', $selector);
    +    }
    +
    +    return sprintf('element%s matching %s "%s"', $pluralization, $selector_type, $selector);
    +  }
    +
    

    More of the same from above. All I can find with grep is vendor/behat/mink/src/WebAssert.php:

    private function getMatchingElementRepresentation(...)
    

Thanks!
-Derek

Thank you so much @dww for the thorough review!

Re #90:
1. Done
2. Done
3. Done
4. Done
5. Done
6. Done
7. I believe the intention here is to document that first we assert the initial state before each trigger is triggered, which makes sense, but it's confusing to read indeed. I believe this can be done line by line instead so it reads more clearly, so I updated all these accordingly.
8. Good point, doing so in this patch.
9. Whacky indeed :)
10. Again good point, done.
11. Good catch, fixed.
12. Fixed.
13. I agree, and I hope we expand on his when we fix existing bugs.
14. I agree as well, done.
15. Good catch, fixed.
16. This can either be a string, or an array of strings since they are selectors. It was probably documented like that because thats how it is on \Behat\Mink\Element\ElementInterface::find()
17. Yeah... I added a docblock for it (not 100% sure about it).
18. I have changed it to be consistent with the rest of the patch: The element selector type (css|xpath|named).
19. Fixed.
20. I added a docblock for it (not 100% sure about it).

Oops, mixed up the assertions on the fieldset and detail elements in the last patch. Fixing that now.

Of course since we changed the field names we must update the test.

Addressing the deprecation notices...

Self review:

Re #91:

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
@@ -28,34 +27,29 @@ class JavascriptStatesTest extends WebDriverTestBase {
-    $this->assertFieldNotVisible('textfield_invisible_when_checkbox_trigger_checked');
-    // Test that the details element is now open so the textfield inside is now
-    // visible.
-    $this->assertFieldVisible('textfield_in_details');
+    $assert_session->assertFieldNotVisible('textfield_invisible_when_checkbox_trigger_checked');
+    $this->assertTrue($this->xpath('//details[@id="edit-details-expanded-when-checkbox-trigger-checked" and @open="open"]'), 'The details element is correctly expanded when the checkbox is checked.');

I messed this up, the assertion for the details element is not correct.

Re #93:

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
@@ -40,7 +40,7 @@ public function testJavascriptStates() {
-    $this->assertTrue($this->xpath('//details[@id="edit-details-expanded-when-checkbox-trigger-checked" and @open="open"]'), 'The details element is correctly expanded when the checkbox is checked.');
+    $this->assertFalse($this->xpath('//details[@id="edit-details-expanded-when-checkbox-trigger-checked" and @open="open"]'), 'The details element is correctly expanded when the checkbox is checked.');

This is not the correct fix for the above, it should be checking the correct details element instead.

OK apparently I'm loosing my head from time to time... The details element is the correct one, and the patch on #91 was correct for this part. However the assertion isn't as I checked the markup changes of the detail element of the test form manually and the attribute "open" is just set without a value, thus why it wasn't working.

I haven't gotten the test to run locally yet for whatever reason:

1) Drupal\FunctionalJavascriptTests\Core\Form\JavascriptStatesTest::testJavascriptStates
The test wasn't able to connect to your webdriver instance. For more information read core/tests/README.md.

The original message while starting Mink: Could not open connection: Curl error thrown for http POST to http://localhost:4444/wd/hub/session with params: {"desiredCapabilities":{"browserName":"chrome","name":"Behat Test"}}

The requested URL returned error: 404 Not Found

Although I do have chromedriver running on that port, and I can actually see it in the browser window... /shrug

Anyway hopefully this comes back green :)

Fantastic! Thanks so much for addressing all that.

I'm deeply sorry to report that I missed some stuff on the last review. :( A new day with fresh eyes leads to fresh concerns. ;)

1. +++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
   @@ -23,6 +23,161 @@ public function getFormId() {
   +    $form['separator'] = [
   +      '#markup' => '<hr />',
   +    ];
   

   Probably doesn't matter, but this isn't used anywhere else in the test/patch. Should probably be removed.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,173 @@
   +   * Tests the javascript functionality of form states.
   

   Another ubernit: core seems to use 'JavaScript' in comments.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,173 @@
   +  public function testJavascriptStates() {
   

   Given "FunctionalJavascriptTests" namespace, I think in class and method names, "Javascript" is fine, so I wouldn't mess with changing any of the rest of this, just the class comment.

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,173 @@
   +    // We need to send a backspace hit to trigger JS events.
   +    $trigger->keyUp(8);
   ...
   +    // We need to send a backspace hit to trigger JS events.
   +    $textfield_trigger->keyUp(8);
   

   Reading more closely, it looks like we only need this when mucking with text fields, right? Maybe the comment should say so? Then folks won't think it's a problem that the other "Change back to the initial state..." blocks aren't doing this, too.

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,173 @@
   +    $assert_session->elementAttributeContains('css', '#edit-fieldset-visible-when-radios-trigger-has-value2', 'style', 'display: none');
   ...
   +    $assert_session->elementAttributeNotContains('css', '#edit-fieldset-visible-when-radios-trigger-has-value2', 'style', 'display: none');
   

   Any reason we can't use assertElementVisible() / assertElementNotVisible() for these?

6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,173 @@
   +    // Test that textfield dependant on checkbox 'value2' is not visible before
   ...
   +    // Test that textfield dependant on checkbox 'value3' is not visible before
   ...
   +    // Test that textfield dependant on checkbox 'value2' is still not visible.
   ...
   +    // Test that textfield dependant on checkbox 'value3' is still not visible.
   ...
   +    // Test that textfield dependant on checkbox 'value2' is now visible.
   ...
   +    // Test that textfield dependant on checkbox 'value3' is still not visible.
   ...
   +    // Test that textfield dependant on checkbox 'value2' is still visible.
   ...
   +    // Test that textfield dependant on checkbox 'value3' is now visible.
   ...
   +    // Test that textfield dependant on checkbox 'value2' is now invisible.
   ...
   +    // Test that textfield dependant on checkbox 'value3' is still visible.
   ...
   +    // Test that item element dependant on select 'Value 2' option is not
   ...
   +    // Test that textfield dependant on select 'Value 3' option is not visible
   ...
   +    // Test that textfield dependant on select 'Value 2' or 'Value 3' options is
   ...
   +    // Test that item element dependant on select 'Value 2' option is now
   ...
   +    // Test that textfield dependant on select 'Value 3' option is not visible.
   ...
   +    // Test that textfield dependant on select 'Value 2' or 'Value 3' options is
   ...
   +    // Test that item element dependant on select 'Value 2' option is not
   ...
   +    // Test that textfield dependant on select 'Value 3' option is now visible.
   ...
   +    // Test that textfield dependant on select 'Value 2' or 'Value 3' options is
   ...
   +    // Test that item element dependant on select 'Value 2' option and textfield
   ...
   +    // Test that item element dependant on select 'Value 2' option and textfield
   ...
   +    // Test that item element dependant on select 'Value 2' option and textfield
   

   Sorry, missed these, too.
   s/dependant/dependent/

7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -46,6 +49,233 @@ function isAjaxing(instance) {
   +   * Asserts that the specific element is visible on the current page.
   ...
   +   * Asserts that the specific element is not visible on the current page.
   ...
   +   * Asserts that the specific element is required on the current page.
   ...
   +   * Asserts that the specific element is not required on the current page.
   

   Not necessarily "on the current page", right? Depends on $container. The assertField* versions seem better, since they don't specify in the summary and clarify when documenting $container.

8. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -46,6 +49,233 @@ function isAjaxing(instance) {
   +   * @param \Behat\Mink\Element\ElementInterface $container
   +   *   The document to check against.
   ...
   +   * @param \Behat\Mink\Element\ElementInterface $container
   +   *   The document to check against.
   ...
   +   * @param \Behat\Mink\Element\ElementInterface $container
   +   *   The document to check against.
   ...
   +   * @param \Behat\Mink\Element\ElementInterface $container
   +   *   The document to check against.
   ...
   +   * @param \Behat\Mink\Element\ElementInterface $container
   +   *   The document to check against.
   

   Again, I think the assertField* versions handle this better. We should say it's optional, and what happens when you leave it off.

9. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -46,6 +49,233 @@ function isAjaxing(instance) {
   +      'Element "%s" is not visible.',
   

   Should be 'is visible', right?

10. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -46,6 +49,233 @@ function isAjaxing(instance) {
    +  /**
    +   * Asserts a condition on an element.
    +   *
    +   * @param bool $condition
    +   *   The condition to check.
    +   * @param string $message
    +   *   Failure message.
    +   * @param \Behat\Mink\Element\Element $element
    +   *   The element to raise the exception with if the condition is not met.
    +   *
    +   * @throws \Behat\Mink\Exception\ElementHtmlException
    +   *   When the condition is not fulfilled.
    +   */
    +  protected function assertElement($condition, $message, Element $element) {
    +    if ($condition) {
    +      return;
    +    }
    +
    +    throw new ElementHtmlException($message, $this->session->getDriver(), $element);
    +  }
    

    A) Seems worth documenting why we're duplicating this function from the parent class here, perhaps adding a @see link, too.

    Something like:

    /**
     * Asserts a condition on an element.
     *
     * Unfortunately, the parent class marks this method as 'private', so we can't use it ourselves. Blah blah blah...
     *
     * @see \Behat\Mink\WebAssert::assertElement()
     *
     * @param ...
    

    B) I'd consider moving this function to the top of all the new methods we're adding to JSWebAssert, since nearly everything else depends on it.

11. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -46,6 +49,233 @@ function isAjaxing(instance) {
    +   * Find a node in the container specified usually the current page.
    

    This doesn't really parse. You'd need more punctuation to keep this wording. But I'd simplify to just:

    * Find a node in the specified container.
    

12. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -46,6 +49,233 @@ function isAjaxing(instance) {
    +   * @param string $locator
    +   *   One of id|name|label|value for the field.
    ...
    +   * @param string $locator
    +   *   One of id|name|label|value for the field.
    ...
    +   * @param string $locator
    +   *   One of id|name|label|value for the field.
    ...
    +   * @param string $locator
    +   *   One of id|name|label|value for the field.
    

    For $selector_type above, when the @param doc has foo|bar|baz, those are the literal values expected. In this case, what we're really expecting is a selector string that matches one of those selector types (id|name|label|value). Not sure the most clean/concise way to say it, but I think we can improve these. How about:

    * @param string $locator
    *   A locator string targeting the id, name, label or value for the field.
    

    I think that's really clear that callers shouldn't try passing 'label' for this. ;) Thoughts?

13. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
    @@ -46,6 +49,233 @@ function isAjaxing(instance) {
    +  public function assertFieldVisible($locator, TraversableElement $container = NULL) {
    ...
    +  public function assertFieldNotVisible($locator, TraversableElement $container = NULL) {
    ...
    +  public function assertFieldRequired($locator, TraversableElement $container = NULL) {
    ...
    +  public function assertFieldOptional($locator, TraversableElement $container = NULL) {
    

    I'd also move all these higher up in the file, to be right under all the assertElement*() methods.

p.s. re: #100.2:

"Form State" has very special and specific meaning in Drupal. ;) Maybe this would be better as:

* Tests the JavaScript #states functionality of forms.

Thanks again for the review @dww

Re #100:

1. Removed
2. Done
3. I agree
4. I believe you're right, and clarifying that is a great idea. Done.
5. We can probably do so in this case, and the test would pass. In my opinion however if we assert that these specific fieldsets are hidden themselves, which is what the states change should do (add / remove display: none), then the test is more explicit / robust.
6. Good catch, fixed.
7. Good point. I cleaned it up and clarified that the $container parameter defaults to the current page.
8. Agreed, fixed.
9. Good catch, yes it should, fixed.
10. OK, done.
11. Yup, fixed.
12. MUCH clearer, thanks for that.
13. Makes sense, done.

Re #101: good point, done.

Question: The new methods we're adding to \Drupal\FunctionalJavascriptTests\JSWebAssert would be useful to functional tests as well, and (as far as I can tell) these are not specific to JS functionality. Would it be possible to add them to \Drupal\Tests\WebAssert instead?

Wonderful, looking great!

Re:

Question: The new methods we're adding to \Drupal\FunctionalJavascriptTests\JSWebAssert would be useful to functional tests as well, and (as far as I can tell) these are not specific to JS functionality. Would it be possible to add them to \Drupal\Tests\WebAssert instead?

I tried this locally. Turns out the visibility stuff is JS-dependent. Trying to use assertElementVisible() in a Functional test results in:

Behat\Mink\Exception\UnsupportedDriverActionException: Element visibility check is not supported by Behat\Mink\Driver\GoutteDriver

However, the required/optional stuff seems to work great in a Functional test:

Test:

// Missing element:
$assert_session->assertFieldRequired('projects[zzz_update_test]');

Result:

Behat\Mink\Exception\ElementNotFoundException: Element with named "field projects[zzz_update_test]" not found.

Test:

// Optional element, check for required:
$assert_session->assertFieldRequired('projects[aaa_update_test]');

Result:

Behat\Mink\Exception\ElementHtmlException: Element "field matching locator "projects[aaa_update_test]"" is required.

Test:

// Optional element, check for optional:
$assert_session->assertFieldOptional('projects[aaa_update_test]');

Result:

OK (1 test, 9 assertions)

So, if you wanted to split the required/optional stuff from the visibility stuff and move those parts to WebAssert, I wouldn't object. ;) But I think you have to leave the visibility-related assertions in JSWebAssert.

Thanks,
-Derek

Actually, looking more closely at these failure messages, I think I was wrong with #100.9. The failure message in #103 for trying to use assertFieldRequired() on something optional is confusing:

Behat\Mink\Exception\ElementHtmlException: Element "field matching locator "projects[aaa_update_test]"" is required.

Looking at the upstream WebAssert, we see messages like this:

    public function cookieEquals($name, $value)
    {
        $this->cookieExists($name);

        $actualValue = $this->session->getCookie($name);
        $message = sprintf('Cookie "%s" value is "%s", but should be "%s".', $name, $actualValue, $value);

        $this->assert($actualValue == $value, $message);
    }

    public function cookieExists($name)
    {
        $message = sprintf('Cookie "%s" is not set, but should be.', $name);
        $this->assert($this->session->getCookie($name) !== null, $message);
    }

    public function statusCodeEquals($code)
    {
        $actual = $this->session->getStatusCode();
        $message = sprintf('Current response status code is %d, but %d expected.', $actual, $code);

        $this->assert(intval($code) === intval($actual), $message);
    }
    ...

So I think we need to re-do the error messages in these new assertion methods to be more clear that there's a failure and why. See #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages for a larger discussion. ;)

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -46,6 +49,238 @@ function isAjaxing(instance) {
   +      'Element "%s" is visible.',
   

   'Element "%s" is invisible, but visible expected.'

   Or:

   'Element "%s" should be visible, but it is not.'

   ?

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -46,6 +49,238 @@ function isAjaxing(instance) {
   +      'Element "%s" is not visible.',
   

   Inverse of whatever we decide above.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -46,6 +49,238 @@ function isAjaxing(instance) {
   +      'Element "%s" is required.',
   

   'Element "%s' should be required, but is not.'
   ?

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -46,6 +49,238 @@ function isAjaxing(instance) {
   +      'Element "%s" is optional.',
   

   'Element "%s' should be optional, but is required.'
   ?

(or something).

Thanks/sorry!
-Derek

p.s. Since it NW anyway, you might as well split the optional/required into WebAssert while you're at it. ;)

Sorry to do this at this point, but I've been looking more closely at core's existing JS plumbing. I'm now having doubts about some of the generic helper methods.

Looking at 8.9.x branch, core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php already has these (deprecated) methods:

protected function assertElementVisible()
protected function assertElementNotVisible()

both point to:

   * @deprecated in drupal:8.1.0 and is removed from drupal:9.0.0. Use
   *   \Behat\Mink\Element\NodeElement::isVisible() instead.

So I'm not sure we want to re-add these like this at all. :(

Maybe we should refactor the tests to not need all this shared plumbing at all? :/

Sorry,
-Derek

For some inspiration of how to do this without any of the helpers, check out my patch at #3133532-5: JavaScript #states not working on 'duration' form elements. Seems pretty straight-forward:

  /**
   * Tests JavaScript #states functionality for 'duration' elements.
   */
  public function testDurationStates() {
    $this->drupalGet('/duration-field-form-test/duration-element-states');
    $page = $this->getSession()->getPage();

    // Test states of elements triggered by a checkbox element.
    $trigger = $page->findField('checkbox_trigger');
    $this->assertNotEmpty($trigger);

    // Check initial state.
    $duration_invisible_field = $page->find('css', 'form div.js-form-item-duration-invisible-when-checkbox-trigger-checked');
    $this->assertNotEmpty($duration_invisible_field);
    $this->assertTrue($duration_invisible_field->isVisible());

    // Change state: check the checkbox.
    $trigger->check();

    // Test that the duration is not visible anymore.
    $this->assertFalse($duration_invisible_field->isVisible());
  }

Note that in this case, the 'duration' field isn't really a field ... it's a div wrapping a bunch of individual number elements. Which is why I needed this:

    $duration_invisible_field = $page->find('css', 'form div.js-form-item-duration-invisible-when-checkbox-trigger-checked');

Anyway, hope that helps. I'm tempted to work on a version of this myself, but then I can no longer RTBC.

@Manuel Garcia you probably hate me by now. ;) But would you be willing to give this another push? Or would you rather I did it and we find someone else to review / RTBC it?

Thanks/sorry,
-Derek

Bit more Git archeology... apparently deprecating assertElementVisible() happened in #2711963: Modernized ToolbarIntegrationTest

@dww thank you very much for spending the time to figure things out, it's very useful, so no, I don’t hate you lol!

Assigning to myself, working on this.

Here is the patch without adding anything to \Drupal\FunctionalJavascriptTests\JSWebAssert. Smaller patches++

Yay, thanks!! Glad you're not upset. ;) Looking much better. Almost there!

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,165 @@
   +
   ...
   +    $this->assertTrue($element->isVisible(), 'The textfield is visible before the checkbox is checked');
   

   Although the discussion is still going at #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages, we seem to be converging on agreement:

   A) In the vast majority of cases, a custom assertion message is no longer needed in our tests. If any of these fail, the line # and default phpunit assertion failure message will be enough to understand what's wrong.

   B) If we keep custom messages (e.g. when we're inside a loop and need them to identify which iteration we were on that failed), the suggested form is now: {context} should {verb} {payload}.

   The messages in here aren't consistent. Sometimes they use 'should', sometimes, they're blanket 'positive' statements, etc. However, I think we can follow A for this and all the other assert* lines in the test and simply remove the custom messages so we don't have to split hairs over phrasing them consistently. Only flagging this one in the review, but there are a bunch that can be removed to further simplify and shrink the patch.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,165 @@
   +    $element = $page->findField('textfield_required_when_checkbox_trigger_checked');
   

   Per #106, you don't have to call find*() twice on the same element. Once you find it, if you toggle the trigger state, you can immediately assert on the element again to see the new state.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,165 @@
   +    $element = $page->findField('textfield_invisible_when_checkbox_trigger_checked');
   

   Instead of continuing to reuse $element for all the found elements, if we use unique + self-documenting names we don't have to keep re-finding them.

   E.g. the whole first block could become:

   $textfield_invisible_element = $page->findField('textfield_invisible_when_checkbox_trigger_checked');
   $textfield_required_element = $page->findField('textfield_required_when_checkbox_trigger_checked');
   $details = $assert_session->elementExists('css', '#edit-details-expanded-when-checkbox-trigger-checked');
   $this->assertTrue($textfield_invisible_element->isVisible());
   $this->assertFalse($textfield_required_element->hasAttribute('required'));
   $this->assertFalse($details->hasAttribute('open'));
   // Change state: check the checkbox.
   $trigger->check();
   $this->assertFalse($textfield_invisible_element->isVisible());
   $this->assertEquals('required', $textfield_required_element->getAttribute('required'));
   $this->assertTrue($details->hasAttribute('open'));
   

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,165 @@
   +    $assert_session->elementAttributeExists('css', '#edit-details-expanded-when-checkbox-trigger-checked', 'open');
   

   Can be just:

   $this->assertTrue($details->hasAttribute('open'));
   

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,165 @@
   +    $element = $assert_session->elementAttributeExists('css', '#edit-textfield-required-when-checkbox-trigger-checked', 'required');
   +    $this->assertTrue($element->getAttribute('required') == 'required', 'The text field is now required.');
   

   These two are duplicate, since the first is finding an element with the required attribute, and the 2nd confirms it's got a required attribute. ;) However, if the element variable had a unique name, both of these could become something like:

   $this->assertEquals('required', $textfield_required_element->getAttribute('required'));
   

6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,165 @@
   +    $assert_session->elementAttributeContains('css', '#edit-fieldset-visible-when-radios-trigger-has-value2', 'style', 'display: none');
   ...
   +    $assert_session->elementAttributeNotContains('css', '#edit-fieldset-visible-when-radios-trigger-has-value2', 'style', 'display: none');
   

   For consistency, maybe better to have:

   $fieldset = $page->find(...);
   $this->assertFalse($fieldset->isVisible());
   // change state here...
   $this->assertTrue($fieldset->isVisible());
   

   This all becomes so clear that I don't think we need inline comments like "Test that the fieldset element is now visible."

7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,165 @@
   +    $element = $page->findField('textfield_visible_when_checkboxes_trigger_value2_checked');
   ...
   +    $element = $page->findField('textfield_visible_when_checkboxes_trigger_value3_checked');
   

   Unique names for the different elements we care about would let us avoid a bunch of later calls to findField().

8. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,165 @@
   +    $element = $page->find('css', '#edit-item-visible-when-select-trigger-has-value2');
   ...
   +    $element = $page->findField('textfield_visible_when_select_trigger_has_value3');
   ...
   +    $element = $page->findField('textfield_visible_when_select_trigger_has_value2_or_value3');
   

   And here...

9. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,165 @@
   +    $element = $page->findField('#edit-item-visible-when-select-trigger-has-value2');
   

   This is the test failure... you need find() not findField() here. But you don't need it at all, since you already found it above. ;)

10. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -0,0 +1,165 @@
    +    $element = $page->find('css', '#edit-item-visible-when-select-trigger-has-value2-and-textfield-trigger-filled');
    ...
    +    $element = $page->find('css', '#edit-item-visible-when-select-trigger-has-value2-and-textfield-trigger-filled');
    ...
    +    $element = $page->find('css', '#edit-item-visible-when-select-trigger-has-value2-and-textfield-trigger-filled');
    

    In this block, there's only one $element we care about, so you can just drop the 2nd and 3rd calls.

Otherwise, I'm running out of things to complain about. ;)

Thanks again for driving this home and putting up with all these reviews!

Cheers,
-Derek

re: #111.3 and 9: If you wanted to be a little more explicit, and have a clearer failure if we have the wrong locator or an expected element is missing, we could adopt this form for each of the blocks of test code:

    // Test states of elements triggered by a checkbox element.
    // Find the trigger and target elements.
    $trigger = $page->findField('checkbox_trigger');
    $this->assertNotEmpty($trigger);
    $textfield_invisible_element = $page->findField('textfield_invisible_when_checkbox_trigger_checked');
    $this->assertNotEmpty($textfield_invisible_element);
    $textfield_required_element = $page->findField('textfield_required_when_checkbox_trigger_checked');
    $this->assertNotEmpty($textfield_required_element);
    $details = $assert_session->elementExists('css', '#edit-details-expanded-when-checkbox-trigger-checked');
    $this->assertNotEmpty($details);
    // Verify initial state.
    $this->assertTrue($textfield_invisible_element->isVisible());
    $this->assertFalse($textfield_required_element->hasAttribute('required'));
    $this->assertFalse($details->hasAttribute('open'));
    // Change state: check the checkbox.
    $trigger->check();
    // Verify triggered state.
    $this->assertFalse($textfield_invisible_element->isVisible());
    $this->assertEquals('required', $textfield_required_element->getAttribute('required'));
    $this->assertTrue($details->hasAttribute('open'));
    // Change back to the initial state to avoid issues running the next tests.
    $trigger->uncheck();

If I run that locally, and intentionally break something, instead of seeing:
Error: Call to a member function isVisible() on null

I get something arguably more useful:
Failed asserting that a NULL is not empty.

In both cases, we get a line number, so we can immediately find the assertion that failed.

Re #11, #12 All good feedback, thanks! I think I got all of that in this patch. Also cleaned up most of the comments since the test is much clearer due to the explicit variable and form element names.

I also took the liberty of using $assert_session->elementExists() instead of $page->find(), since it will get you the element and assert that it is there in one call.

So close!! ;)

+1 to $assert_session->elementExists().

1. +++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
   @@ -23,6 +23,161 @@ public function getFormId() {
   +
   +    $form['separator'] = [
   +      '#markup' => '<hr />',
   +    ];
   +
   

   This is still here. ;) In #102 you said you removed it, but apparently not. Sorry I missed it.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,156 @@
   +  public static $modules = ['form_test'];
   

   This can be protected.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
   @@ -0,0 +1,156 @@
   +    $textfield_visible_value3 = $page->findField('textfield_visible_when_checkboxes_trigger_value3_checked');
   +    $this->assertNotEmpty($trigger);
   

   Checking empty on the wrong variable. Should be:

   assertNotEmpty($textfield_visible_value3);

Thanks again!
-Derek

#114 are such trivial fixes, here we go. Once the bot's happy, I still feel okay RTBC'ing here.

I thought it'd be good to add some assertions on the textfields inside the details and fieldset. Good thing I did! ;) Found a bug in the form.

+++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
@@ -23,6 +23,157 @@ public function getFormId() {
+    $form['fieldset_visible_when_radios_trigger_has_value2'] = [
...
+    $form['fieldset_visible_when_radios_trigger_has_given_value']['textfield_in_fieldset'] = [

'textfield_in_fieldset' wasn't actually in the fieldset, since s/given_value/value2/ didn't happen for its parent item. ;)

Also added a few more clarifying comments.

Finally, I don't think we need to repeat all of "// Change back to the initial state to avoid issues running the next tests." each time. After the first one, I shortened the others to just: "// Change back to the initial state."

Sorry, 1 more. ;) If we're going to @see link to test plumbing in the docblock for processStates() seems like we should do this, too:

@see \Drupal\FunctionalJavascriptTests\Core\Form\JavascriptStatesTest

That seems more useful than @see \Drupal\form_test\Form\JavascriptStatesForm, although it doesn't seem to hurt to leave both.

Oh wow good catch on #116 @dww! Also, thanks for #115.

The changes since my last patch look good to me, the comments clean up make sense, and +1 for the extra @see.

Wow! Thanks so much for pushing this along! Remember spending a good chuck of time debating the setup for this at DevDays Milan, good times :)

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
@@ -0,0 +1,171 @@
+    // Change back to the initial state.
+    $select_trigger->setValue('_none');
+    $textfield_trigger->setValue('');
+    // When dealing with textfields we need to send a backspace hit to trigger
+    // JS events.
+    $textfield_trigger->keyUp(8);

This might not be necessary at the end of the test, but I think it is safe to assume we will need to add more coverage to this at some point, so no harm in preparing for that.

Added relevant tag

Re: #119: Thanks for the review, and RTBC, @Lendude! Re: cleaning up the final case - agreed. See #90.13. ;)

Big +1 to RTBC. This will be a huge step in the right direction. Let's update the IS with which states are being tested here to make it easier for the committers to understand the patch.

Thanks! Updated the summary and title. Hope that's sufficient, and we don't need the exact 5x5 table of which combos of trigger and #state are tested here. That seems more appropriate at the parent: #2892872: [META] Add Javascript tests for all Form API's #states

Cheers,
-Derek

To (hopefully) lay a more solid foundation here to expand this coverage in subsequent issues, and based on the experience of fleshing out the summary, here's an updated approach that splits up the huge testJavascriptStates() method into a series of protected helper methods:

A) Easier to see what coverage we have for each type of triggering element.
B) No method is longer than ~30 lines, easier to understand everything at a glance.
C) Since each helper reloads the form, we don't have to do all the resetting state stuff... each protected method starts with a clean slate.
D) Easy to expand coverage for an existing trigger element without worrying about impacting other things.
E) Easy to add new coverage for another kind of triggering element.
F) Once split like this, none of the protected methods needed $assert_session more than once, so I removed that local variable entirely and just call $this->assertSession() directly when needed.

Interdiff is a bit larger than it needed to be, since I also moved the checkboxes helper to right below the checkbox one.

Do y'all agree this is an improvement? Anyone willing to re-RTBC?

Thanks!
-Derek

p.s. Leaving #117 visible in the summary files table, in case folks prefer that over this.

Noting in the summary that this includes coverage for "Multiple triggers (checkbox AND select)".

Curious for myself, I started a spreadsheet to track the coverage:

https://docs.google.com/spreadsheets/d/1jaROESn25lz1L4C0v0vHgFYdtrR6n9TL...

Adding link to that in the summary.

Not quite as much of a "wide swath" as I initially thought, although we do have at least something from each column and row. ;) Not sure if it's worth continuing to expand here or move to followups.

To further prep for expansion, here's a trivial reorganization of the new form elements added to core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php to group form elements by what kind of triggering element(s) they respond to.

This completes the test coverage row for checkbox triggers. Shows how easy it is to expand this given the current layout/structure of the form and test.

Updated remaining tasks:

1. Decide how much of the coverage matrix we need to fill out in this issue vs. follow-ups from the parent META.
2. Re-RTBC.
3. Commit.

Complete coverage for the entire 'Visible' column.