Issue #589: Click fieldset summary to reveal

[xurizaemon]

OK, this uses xpath to locate the element summary text which needs clicking to expand. So on a create node form, you might Then I expand details labelled "Menu settings", Then I expand details labelled "Metatag", Then I expand details labelled "URL alias". Case does matter here (and admin themes may display the summary in uppercase).

I hoped to use $this->getSession()->wait(); instead of usleep() here but that didn't work (the details to be expanded weren't visibly expanded in a screenshot in the next step). Input welcome, happy to improve on this PR!

This new step should be a no-op for drivers such as Goutte that don't support click on non-link/submit/button elements; they are likely to have the collapsed details "visible" anyway.

Summary by CodeRabbit

• Tests
  • Added a new step definition for expanding collapsible detail sections in Behat test automation, enabling more comprehensive testing of interactive UI components.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[xurizaemon]

Tests passed in https://travis-ci.com/github/xurizaemon/drupalextension/builds/231790255 for xurizaemon@cb3e629 - I checked but didn't see existing test coverage for tests which depend on javascript-capable drivers, so haven't added any for this PR.

[xurizaemon]

Have discovered a need for this looser XPath where videos had a Trascript summary element:

  /**
   * Expand a <details> element by <summary>.
   *
   * @Given I expand details with summary containing :summary
   */
  public function iExpandDetailsByLabelContaining($summary) {
    $page = $this->getSession()->getPage();
    $element = $page->find('xpath', "//details/summary[@aria-expanded='false'][contains(text(), '$summary')]");
    if (empty($element)) {
      throw new \Exception("Unable to find details element containing text $summary");
    }
    try {
      $element->click();
      usleep(100000);
    }
    catch (UnsupportedDriverActionException $exception) {
      // Goutte etc only supports clicking link, submit, button;
      // for non-JS drivers this won't impact test.
    }
  }

Can't get both approaches to work using a common XPath selector - the video transcript summary is different to Drupal's more common structure. May refactor to try exact match, then "contains" match if the first fails to find an element to expand.

[phuang07]

How about using "id" to identify the element. Here is my modification that works in my case:

/**
   * Expand a <details> element by <id>.
   *
   *
   * @Given I expand details with id :id
   */
  public function iExpandDetailsWithId($id) {
    $page = $this->getSession()->getPage();
    $element = $page->find('xpath', "//details[@id='$id']");
    if (empty($element)) {
      throw new \Exception("Unable to find details element with $id");
    }
    try {
      $element->click();
      usleep(100000);
    }
    catch (UnsupportedDriverActionException $exception) {
      // Goutte etc only supports clicking link, submit, button;
      // for non-JS drivers this won't impact test.
    }
  }

[xurizaemon]

@phuang07 a DOM ID is an internal detail of the implementation. Behat steps should be written to describe in user language to refer to business details, not implementation details. For example:

• A user reading the test should need to know only "I expand the details labelled 'URL Alias'" without being exposed to details about the underlying DOM structure.
• When are testing a search input in the main content region and a new search input is introduced to the header, the test should not fail when Drupal's autogenerated search input ID changes. (This example assumes use of "in the 'content' region" suffix!)

I'd avoid specifying by ID for those reasons. In tricky cases where an ID feels necessary, I'll do it in the step definition's function, and avoid exposing it in the .feature.

[phuang07]

@xurizaemon I totally get it. The behat step should strive for the higher level business details. But sometimes using id to target an element is unavoidable. In my case, there are two details tag with the same labeled, which they should really make a different. However, instead of closing the door for targeting an element by ID, maybe it should give the developer the option for that? Many of the behat definitions functions seem follow this patten. For example,

 /**
   * Presses button with specified id|name|title|alt|value.
   *
   * @When I press the :button button
   */


  /**
   * Fills in a form field with id|name|title|alt|value in the specified region.
   *
   * @Given I fill in :value for :field in the :region( region)
   * @Given I fill in :field with :value in the :region( region)
   *
   * @throws \Exception
   *   If region cannot be found.
   */

[xurizaemon]

I understand, but I'd recommend to deal with this using labels and regions.

If there are identically labeled sections in the same region then that sounds like it might be a bug at the UX/business layer?!

(Agree, this pattern is found in some existing Behat steps. Happy to discuss further - this PR does need work anyway and I'm open to improving it, just not convinced ID is the right way but could accept it being an available fallback.)

[jhedstrom]

Even though there aren't currently JS tests (IIRC they were removed when PhantomJS stopped working), adding a test for the normal driver would still show this works on non-JS drivers (better than no test). We can then add a proper test if/when JS testing is re-enabled.

[AlexSkrypnyk]

@xurizaemon

1. This implementation relies on aria-expanded which is coupled to a specific implementation. Maybe this can be somehow made more generic.

2. This method feels a bit one-sided: we allow to expand but not to collapse. Can it have a "brother" method to collapse?

3. The description should use When rather than Given as this is an action rather than a prerequisite. Could you please update.

4. It would be nice to be able to control the the usleep() number of seconds. Maybe as a parameter or via config.

[coderabbitai]

📝 Walkthrough

Walkthrough

A new Behat step definition method iExpandDetailsByLabel() is added to MinkContext that locates and expands HTML details/summary elements with a specific text label and aria-expanded state, clicking them and pausing briefly while gracefully handling drivers that don't support JavaScript interactions.

Changes

Cohort / File(s)	Summary
New Behat Step Definition src/Drupal/DrupalExtension/Context/MinkContext.php	Added iExpandDetailsByLabel($summary) method to expand details/summary elements. Method finds elements with matching text and aria-expanded='false', clicks them, pauses execution, and silently ignores unsupported driver exceptions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~18 minutes

Poem

🐰 A details element hides its tale,
But now a click makes secrets sail—
With aria-expanded, bold and true,
Our rabbity steps expand the view! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding a Behat step to click fieldset/details summaries to expand them.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @src/Drupal/DrupalExtension/Context/MinkContext.php:
- Line 622: The XPath query interpolates $summary directly causing syntax
breakage and XPath injection; fix by using the existing xpathLiteral() helper to
quote/escape the value and update the call that assigns $element (the
$page->find('xpath', ...) invocation) to pass xpathLiteral($summary) instead of
embedding $summary raw, ensuring the XPath expression is constructed safely
(refer to the xpathLiteral() usage earlier in the class).

🧹 Nitpick comments (1)

src/Drupal/DrupalExtension/Context/MinkContext.php (1)

622-624: Consider removing aria-expanded='false' constraint or improve error message.

Two related observations:

1. The aria-expanded='false' check means this step will throw an exception if the details is already expanded. This might be intentional, but could cause confusing failures if a previous step or page state already expanded the details.

2. The error message on line 624 doesn't indicate that the step specifically looks for collapsed details elements, making debugging harder.

💡 Options to consider

Option A: Make the step idempotent by not requiring aria-expanded='false':

-        $element = $page->find('xpath', "//details/summary[@aria-expanded='false'][text()][contains(., $summaryLiteral)]");
+        $element = $page->find('xpath', "//details/summary[text()][contains(., $summaryLiteral)]");
         if (empty($element)) {
-            throw new \Exception("Unable to find details element containing text $summary");
+            throw new \Exception("Unable to find details/summary element containing text '$summary'");
         }
+        // Only click if not already expanded
+        if ($element->getAttribute('aria-expanded') !== 'true') {
+            try {
+                $element->click();
+                usleep(100000);
+            } catch (UnsupportedDriverActionException $exception) {
+                // Goutte etc only supports clicking link, submit, button
+            }
+        }

Option B: Keep current behavior but improve the error message:

         if (empty($element)) {
-            throw new \Exception("Unable to find details element containing text $summary");
+            throw new \Exception("Unable to find collapsed details/summary element containing text '$summary' (with aria-expanded='false')");
         }

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5b61482 and 5389430.

📒 Files selected for processing (1)

• src/Drupal/DrupalExtension/Context/MinkContext.php

🧰 Additional context used

🧬 Code graph analysis (1)

src/Drupal/DrupalExtension/Context/MinkContext.php (1)

src/Drupal/DrupalExtension/MinkAwareTrait.php (1)

• getSession (64-67)

🔇 Additional comments (1)

src/Drupal/DrupalExtension/Context/MinkContext.php (1)

626-632: LGTM on the exception handling pattern.

The UnsupportedDriverActionException handling follows the same pattern used elsewhere in this file (e.g., lines 66-68, 189-193, 266-270) and gracefully degrades for non-JS drivers like Goutte.