Allow WebAssert to inspect also hidden fields

+++ b/core/tests/Drupal/Tests/PartialNamedIncludingHiddenSelector.php
@@ -0,0 +1,22 @@
+class PartialNamedIncludingHiddenSelector extends PartialNamedSelector {

Can we push this change upstream?

I was kind of against this, given there is no valid use-case for editing a hidden field except in a JavaScript scenario.

But the code is low impact and helps us with writing tests for things like nefarious dom manipulation.

+1 RTBC from me.

Thank you for fixing this problem! I moved it to the phpunit component, as it kinda belongs more into that area.

1. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -313,7 +314,10 @@ protected function initMink() {
   +    $selectors_handler = new SelectorsHandler([
   +      'named_partial_including_hidden' => new PartialNamedIncludingHiddenSelector()
   +    ]);
   +    $session = new Session($driver, $selectors_handler);
   

   Could we maybe document with a one liner, why we need it? "Per default hidden fields are not available".

2. +++ b/core/tests/Drupal/Tests/PartialNamedIncludingHiddenSelector.php
   @@ -0,0 +1,22 @@
   +/**
   + * Extends PartialNamedSelector to allow retrieval of hidden fields.
   + *
   + * @see \Behat\Mink\Selector\PartialNamedSelector
   + */
   +class PartialNamedIncludingHiddenSelector extends PartialNamedSelector {
   +
   

   Just in general, did someone checked out whether this is something mink would consider to have upstream, or maybe even be fixed as default behaviour upstream?

3. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -422,4 +422,39 @@ public function fieldDisabled($field, TraversableElement $container = NULL)  {
   +   * {@inheritdoc}
   +   */
   +  public function fieldExists($field, TraversableElement $container = NULL) {
   +    try {
   +      return parent::fieldExists($field, $container);
   +    }
   +    catch (ElementNotFoundException $exception) {
   +      // Try once again with the 'named_partial_including_hidden' selector to
   +      // get potential hidden fields.
   +      $container = $container ?: $this->session->getPage();
   +      if ($node = $container->find('named_partial_including_hidden', ['field', $field])) {
   +        return $node;
   +      }
   +      throw $exception;
   +    }
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function fieldNotExists($field, TraversableElement $container = NULL) {
   +    try {
   +      parent::fieldNotExists($field, $container);
   +    }
   +    catch (ExpectationException $exception) {
   +      // Try once again with the 'named_partial_including_hidden' selector to
   +      // get potential hidden fields.
   +      $container = $container ?: $this->session->getPage();
   +      if ($container->find('named_partial_including_hidden', ['field', $field])) {
   +        throw $exception;
   +      }
   +    }
   +  }
   +
   

   I'm wondering whether we should instead change out the existing selectors, by passing along ['named_partial' => new PartialNamedIncludingHiddenSelector()] above? This would allow us potentially to not add all this custom code below.

Ideally though I agree with the points that we should not expose a feature normal browser users don't have per default to our tests? Maybe just adding this capability to javascript tests seems to be a better approach. I also left a small comment on the github issue.

@claudiu.cristea
I mean in this example its kind of obvious. Reordering books is a clear JS feature, so they should be tested in JS, rather than faked using PHP.

I'm not sure we should do this. If the use case is only faking Javascript by manipulating hidden fields, then we should not do this and convert the test to a proper javascript test instead.

Can you provide some other use cases in the issue summary where we would want to manipulate hidden fields? @larowlan mentioned nefarious DOM manipulation, but why would you do that in a functional test?

@klausi
Yeah I still believe having a WebAssert for JS, as we actually already have in \Drupal\FunctionalJavascriptTests\JSWebAssert and put it on there is conceptually better.

There's another related issue - how about you want to assert on the value of a hidden input? $this->assertFieldById() does not work on hidden fields because of this.

So $this->assertSession()->fieldValueEquals() does not work either

Strictly speaking this is no BC layer, should we try to add one or talk about that in another issue?

@alexpott, Probably we want some dedicated assert methods only for hidden fields to cover such cases. ::assertHiddenFieldById(), $this->assertSession()->hiddenFieldValueEquals()? Then we can keep the actual behaviour for existing methods

I like this approach, less magic and more explicit code!

Discussed with @xjm and @catch, and we decided this is "rc eligible". The one hesitation we have about that is that it's not only changing tests, but also making an addition to a testing API: WebAssert could in theory be subclassed, and contrib/custom code might have such a subclass with these method names and collide. But, we think the risk of this actually being the case is sufficiently low, and the disruption for such a project/site wouldn't be breaking live code, but only a CI workflow, until fixed.

+++ b/core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php
@@ -61,6 +61,17 @@ public function testForm() {
+    $this->assertSession()->hiddenFieldExists('strawberry'); // By name.
+    $this->assertSession()->hiddenFieldExists('red'); // By value.
+    $this->assertSession()->hiddenFieldExists('redstrawberryhiddenfield'); // By id.

I'm pretty sure that this types of comments are no allowed in our coding standards - also 80 chars...

Needs a reroll - doesn't apply to 8.3.x - which is where the patch needs to be committed first.

Fixed this on commit:

core/tests/Drupal/Tests/WebAssert.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 478 | ERROR | [x] Concat operator must be surrounded by a single
     |       |     space
 478 | ERROR | [x] Concat operator must be surrounded by a single
     |       |     space
 499 | ERROR | [x] Concat operator must be surrounded by a single
     |       |     space
 499 | ERROR | [x] Concat operator must be surrounded by a single
     |       |     space
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 66ms; Memory: 6Mb

We should discuss whether to cherry-pick this to 8.2.x, so leaving at 'to be ported' there.

8.2.x backport

I have a feeling that this is not going to be backported to 8.2.x anymore :)

Struggled a bit to get this working with drupalPostForm(), what worked is using hiddenFieldExists() to get the hidden form elements and then setValue() on them, and then doing a drupalPostForm() only with the remaining visible form elements.

I have no luck with drupalPostForm() so my way to test forms with hidden fields is like this:

$this->getSession()
          ->getPage()
          ->find('css', "input[name='tagging_my_tags_hidden']")
          ->setValue('TestTag1, TestTag2');

$this->getSession()
      ->getPage()
      ->find('css', "input[name='title']")
      ->setValue('TestTitle111');

$this->getSession()->getPage()->pressButton(t('Save'));