From e4ceff2a242bf9cc96598b1f71de17d6f96753dd Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Thu, 16 Sep 2021 16:13:27 +0800 Subject: [PATCH 1/2] MDL-72593 behat: Load the Field node content locally for processing The standard NodeElement functions for getAttribute, getTagName, getParent, and friends go back to WebDriver and parse the DOM for each request. This is insanely slow per request, and in the case of forms we do a lot of checking to determine the field type. This change modifies the form field detection to copy the entire node content into a DOMDocument and parse the document locally. This is significantly faster - in some cases where there are large documents minutes faster. I believe that this should be a safe change as the document fetched from the browser is normalised to match the doctype specified. --- lib/behat/behat_field_manager.php | 114 ++++++++++++++++++++++-------- 1 file changed, 84 insertions(+), 30 deletions(-) diff --git a/lib/behat/behat_field_manager.php b/lib/behat/behat_field_manager.php index d2d0da7a6c0..0f386b71514 100644 --- a/lib/behat/behat_field_manager.php +++ b/lib/behat/behat_field_manager.php @@ -54,7 +54,8 @@ class behat_field_manager { $fieldnode = $context->find_field($label); // The behat field manager. - return self::get_form_field($fieldnode, $context->getSession()); + $field = self::get_form_field($fieldnode, $context->getSession()); + return $field; } /** @@ -74,12 +75,7 @@ class behat_field_manager { // Get the field type if is part of a moodleform. if (self::is_moodleform_field($fieldnode)) { - // This might go out of scope, finding element beyond the dom and fail. So fallback to guessing type. - try { - $type = self::get_field_node_type($fieldnode, $session); - } catch (WebDriver\Exception\InvalidSelector $e) { - $type = 'field'; - } + $type = self::get_field_node_type($fieldnode, $session); } // If is not a moodleforms field use the base field type. @@ -105,8 +101,7 @@ class behat_field_manager { // If the field is not part of a moodleform, we should still try to find out // which field type are we dealing with. - if ($type == 'field' && - $guessedtype = self::guess_field_type($fieldnode, $session)) { + if ($type == 'field' && $guessedtype = self::guess_field_type($fieldnode, $session)) { $type = $guessedtype; } @@ -135,26 +130,31 @@ class behat_field_manager { * @return string|bool The field type or false. */ public static function guess_field_type(NodeElement $fieldnode, Session $session) { + [ + 'document' => $document, + 'node' => $node, + ] = self::get_dom_elements_for_node($fieldnode, $session); // If the type is explicitly set on the element pointed to by the label - use it. - if ($fieldtype = $fieldnode->getAttribute('data-fieldtype')) { + if ($fieldtype = $node->getAttribute('data-fieldtype')) { return self::normalise_fieldtype($fieldtype); } // Textareas are considered text based elements. - $tagname = strtolower($fieldnode->getTagName()); + $tagname = strtolower($node->nodeName); if ($tagname == 'textarea') { + $xpath = new \DOMXPath($document); // If there is an iframe with $id + _ifr there a TinyMCE editor loaded. - $xpath = '//div[@id="' . $fieldnode->getAttribute('id') . 'editable"]'; - if ($session->getPage()->find('xpath', $xpath)) { + if ($xpath->query('//div[@id="' . $node->getAttribute('id') . 'editable"]')->count() !== 0) { return 'editor'; } return 'textarea'; - } else if ($tagname == 'input') { - $type = $fieldnode->getAttribute('type'); - switch ($type) { + } + + if ($tagname == 'input') { + switch ($node->getAttribute('type')) { case 'text': case 'password': case 'email': @@ -172,11 +172,15 @@ class behat_field_manager { return false; } - } else if ($tagname == 'select') { + } + + if ($tagname == 'select') { // Select tag. return 'select'; - } else if ($tagname == 'span') { - if ($fieldnode->hasAttribute('data-inplaceeditable') && $fieldnode->getAttribute('data-inplaceeditable')) { + } + + if ($tagname == 'span') { + if ($node->hasAttribute('data-inplaceeditable') && $node->getAttribute('data-inplaceeditable')) { return 'inplaceeditable'; } } @@ -206,6 +210,32 @@ class behat_field_manager { return ($parentformfound != false); } + /** + * Get the DOMDocument and DOMElement for a NodeElement. + * + * @param NodeElement $fieldnode + * @param Session $session + * @return array + */ + protected static function get_dom_elements_for_node(NodeElement $fieldnode, Session $session): array { + $html = $session->getPage()->getContent(); + + $document = new \DOMDocument(); + + $previousinternalerrors = libxml_use_internal_errors(true); + $document->loadHTML($html, LIBXML_HTML_NODEFDTD | LIBXML_BIGLINES); + libxml_clear_errors(); + libxml_use_internal_errors($previousinternalerrors); + + $xpath = new \DOMXPath($document); + $node = $xpath->query($fieldnode->getXpath())->item(0); + + return [ + 'document' => $document, + 'node' => $node, + ]; + } + /** * Recursive method to find the field type. * @@ -214,31 +244,54 @@ class behat_field_manager { * * @param NodeElement $fieldnode The current node. * @param Session $session The behat browser session - * @return mixed A NodeElement if we continue looking for the element type and String or false when we are done. + * @return null|string A text description of the node type, or null if one could not be accurately determined */ - protected static function get_field_node_type(NodeElement $fieldnode, Session $session) { + protected static function get_field_node_type(NodeElement $fieldnode, Session $session): ?string { + [ + 'document' => $document, + 'node' => $node, + ] = self::get_dom_elements_for_node($fieldnode, $session); - // Special handling for availability field which requires custom JavaScript. - if ($fieldnode->getAttribute('name') === 'availabilityconditionsjson') { + return self::get_field_type($document, $node, $session); + } + + /** + * Get the field type from the specified DOMElement. + * + * @param \DOMDocument $document + * @param \DOMElement $node + * @param Session $session + * @return null|string + */ + protected static function get_field_type(\DOMDocument $document, \DOMElement $node, Session $session): ?string { + $xpath = new \DOMXPath($document); + + if ($node->getAttribute('name') === 'availabilityconditionsjson') { + // Special handling for availability field which requires custom JavaScript. return 'availability'; } - if ($fieldnode->getTagName() == 'html') { - return false; + if ($node->nodeName == 'html') { + // The top of the document has been reached. + return null; } // If the type is explictly set on the element pointed to by the label - use it. - $fieldtype = $fieldnode->getAttribute('data-fieldtype'); + $fieldtype = $node->getAttribute('data-fieldtype'); if ($fieldtype) { return self::normalise_fieldtype($fieldtype); } - if (!empty($fieldnode->find('xpath', '/ancestor::*[@data-passwordunmaskid]'))) { + if ($xpath->query('/ancestor::*[@data-passwordunmaskid]', $node)->count() !== 0) { + // This element has a passwordunmaskid as a parent. return 'passwordunmask'; } // Fetch the parentnode only once. - $parentnode = $fieldnode->getParent(); + $parentnode = $node->parentNode; + if ($parentnode instanceof \DOMDocument) { + return null; + } // Check the parent fieldtype before we check classes. $fieldtype = $parentnode->getAttribute('data-fieldtype'); @@ -255,11 +308,12 @@ class behat_field_manager { // Stop propagation through the DOM, if it does not have a felement is not part of a moodle form. if (strstr($class, 'fcontainer') != false) { - return false; + return null; } } - return self::get_field_node_type($parentnode, $session); + // Move up the tree. + return self::get_field_type($document, $parentnode, $session); } /** From 0a0823672c2103731a35dac9ae1b541f529270be Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 17 Sep 2021 15:27:58 +0800 Subject: [PATCH 2/2] MDL-72593 behat: Improve behat handling of file manager buttons When using a try/catch for a find if the first match is not met then we have an implicit 6 second delay waiting for the search to time out. We can avoid this by combining the searches in a comma-separated selection. I have also taken the opportunity to convert the use of ->click() to the i_click_on function which has the benefit of supporting wait for pendingJS. --- lib/behat/core_behat_file_helper.php | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/lib/behat/core_behat_file_helper.php b/lib/behat/core_behat_file_helper.php index b4064ef4bfb..17285ed2c93 100644 --- a/lib/behat/core_behat_file_helper.php +++ b/lib/behat/core_behat_file_helper.php @@ -177,20 +177,14 @@ trait core_behat_file_helper { * @return void */ protected function open_add_file_window($filemanagernode, $repositoryname) { - $exception = new ExpectationException('No files can be added to the specified filemanager', $this->getSession()); // We should deal with single-file and multiple-file filemanagers, // catching the exception thrown by behat_base::find() in case is not multiple - try { - // Looking for the add button inside the specified filemanager. - $add = $this->find('css', 'div.fp-btn-add a', $exception, $filemanagernode); - } catch (Exception $e) { - // Otherwise should be a single-file filepicker form element. - $add = $this->find('css', 'input.fp-btn-choose', $exception, $filemanagernode); - } - $this->ensure_node_is_visible($add); - $add->click(); + $this->execute('behat_general::i_click_on_in_the', [ + 'div.fp-btn-add a, input.fp-btn-choose', 'css_element', + $filemanagernode, 'NodeElement' + ]); // Wait for the default repository (if any) to load. This checks that // the relevant div exists and that it does not include the loading image. @@ -220,7 +214,7 @@ trait core_behat_file_helper { if (!$repositorylink->getParent()->getParent()->hasClass('active')) { // If the repository link is active, then the repository is already loaded. // Clicking it while it's active causes issues, so only click it when it isn't (see MDL-51014). - $repositorylink->click(); + $this->execute('behat_general::i_click_on', [$repositorylink, 'NodeElement']); } }