Problem/Motivation

There's an current bug with GastonJS that makes clicking elements with children impossible: https://github.com/jcalderonzumba/gastonjs/issues/19

As we continue to write Javascript tests, we're going to run into this more and more, so instead of waiting for the GastonJS fix we should implement a temporary fix in JavascriptTestBase which protects test-writers from running into this problem. When testing Quick Edit, for example, we will run into this bug when trying to click the "Save" button, as it has nested HTML. I had to implement a similar fix to the one proposed here when writing tests for #2421427: Improve the UX of Quick Editing single-valued image fields.

I've attached a test-only patch which outlines this issue, and a patch with a fix.

Proposed resolution

Catch the GastonJS error and use Javascript to click the target element.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

samuel.mortenson created an issue. See original summary.

The last submitted patch, drupal-click-nested-element-js-test.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, drupal-click-nested-element-js-test-test-only.patch, failed testing.

samuel.mortenson’s picture

Title: Clicking nested elements in Javascript tests throws a GastonJS exception » Clicking elements with children in Javascript tests throws a GastonJS exception
samuel.mortenson’s picture

Status: Needs review » Needs work

The last submitted patch, 5: drupal-2773791-5-test-only.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
dawehner’s picture

As we continue to write Javascript tests, we're going to run into this more and more, so instead of waiting for the GastonJS fix we should implement a temporary fix in JavascriptTestBase which protects test-writers from running into this problem.

Well, did someone tried to create a PR for gastonJS actually? When you have PRs on upstream projects they are usually orders of magnitude faster fixed than anything in Drupal.

samuel.mortenson’s picture

I agree. I've raised this issue to the maintainer, who in April said he would look into it. If nothing else this documents the bug, but if I get time to investigate further I will.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

@samuel.mortenson
I didn't tried to sound harsh, sorry for that!

+++ b/core/modules/simpletest/tests/src/FunctionalJavascript/BrowserWithJavascriptTest.php
@@ -62,4 +62,29 @@ public function testCreateScreenshot() {
 
+  /**
+   * Tests behaviors related to clicking on elements with nested elements.
+   */
+  public function testNestedClick() {
+    $this->drupalGet('<front>');
+    $session = $this->getSession();
+
+    // Create an anchor element with a nested <div>.
+    $javascript = <<<JS
+    (function(){
+      var anchor = document.createElement('A');
+      anchor.href = '#my-anchor';
...
+    $this->click('#anchor-with-children');

It is so nice to see that someone actually cares about tests of test functionality! Thanks a lot!

samuel.mortenson’s picture

I didn't take it in a bad way! If Drupal.org had emoticon support it'd be a lot easier to communicate. :-) Thanks for the review.

Wim Leers’s picture

Awesome work, @samuel.mortenson! :)

alexpott’s picture

Version: 8.2.x-dev » 8.1.x-dev

Discussed with @catch, @xjm, @effulgentsia, and @cottser we decided this is eligible for 8.1.x.

samuel.mortenson’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.5 KB
780 bytes

Caught a small problem my IDE caused, we were passing "''" to addcslashes instead of "'".

alexpott’s picture

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -52,6 +53,22 @@ protected function tearDown() {
+      $css_selector = addcslashes($css_selector, "'");
+      $this->getSession()->executeScript("document.querySelectorAll('" . $css_selector . "')[0].click()");

Discussed with @xjm who pointed out that this means tests can inject javascript using this method. Is there anyway to use xpath and the existing browser methods to do this.

samuel.mortenson’s picture

That's true. We have to execute Javascript for this fix to work, as the Browser has no inherit method to trigger arbitrary events using Javascript. I think that converting the CSS selector to XPath would be an appropriate fix.

samuel.mortenson’s picture

Here's an alternate approach that uses XPaths, given the feedback. It's worth noting that in the previous patch, if the user passed a malformed CSS selector, $this->getSession()->executeScript() would never get called as the click method validates if the element is present before attempting a click. So the user would have to enter a malicious selector, have that selector actually match an element on the page, and have that element be something click-able with a nested element to execute Javascript unintentionally.

So, really unlikely, but I understand why looking at the patch would raise concern. The new patch is covered by all the conditions above, and in addition to that is converted to an XPath and quote-escaped so that it should not escape the document.evaluate() call. If somehow the selector makes it all the way to that point and contains nastiness, document.evaluate() will throw a JS error which in turn will cause the test to fail.

I think we're fairly safe now. :-) Thanks for the review so far, I like diving into these things.

dawehner’s picture

+++ b/core/modules/simpletest/tests/src/FunctionalJavascript/BrowserWithJavascriptTest.php
@@ -62,4 +62,29 @@ public function testCreateScreenshot() {
+      var anchor = document.createElement('A');
+      anchor.href = '#my-anchor';
+      anchor.id = 'anchor-with-children';
+      var div = document.createElement('DIV');
+      anchor.appendChild(div);
+      document.body.appendChild(anchor);

you can dramatically simplify this bit by using document.innerHTML = '<a id="my-anchor"><div></div></a>'

droplet’s picture

Anchor with the empty tag has no dimension that should be invisible and NOT clickable. We shouldn't click it via JavaScript for Browser tests IMO. We should consider to create a new API instead.

Another ideal solution is looking into Behat (Selenium2 driver) and use Behat API to select it.

Just curious, why don't Drupal using Selenium2? Shouldn't Druapl test in real Chrome/Firefox/iOS..etc ?
Selenium2 + webDriver, it can run Phantomjs also.

dawehner’s picture

Anchor with the empty tag has no dimension that should be invisible and NOT clickable. We shouldn't click it via JavaScript for Browser tests IMO. We should consider to create a new API instead.

Well, we can easily put something in those divs and call it a day. This is a detail, IMHO.

@droplet
Just getting phantomjs to run on the testbots for example was a challenge. Using phantomjs also gives us more control over things. Whether its the best situation, I don't know.
Regarding behat, I believe that you are abusing behat for something its not meant to be used for, see https://docs.google.com/document/d/17raktxD51MP0WUvqtNQFsCzevYgDNkvl7jOu...

samuel.mortenson’s picture

We shouldn't click it via JavaScript for Browser tests IMO. We should consider to create a new API instead.

I agree that I would prefer not to do this, but right now there is a bug in our Javascript testing implementation. When you say "Create a new API", what do you have in mind?

Also as an update to people following this issue but not the GastonJS bug on GitHub - the maintainer got back to me today and said they would debug the issue. I'll update again if I hear anything concrete on that end.

dawehner’s picture

Also as an update to people following this issue but not the GastonJS bug on GitHub - the maintainer got back to me today and said they would debug the issue. I'll update again if I hear anything concrete on that end.

Great to know!

droplet’s picture

Well, we can easily put something in those divs and call it a day. This is a detail, IMHO.

To test invisible (display:none) element, we will get a false result.

@droplet
Just getting phantomjs to run on the testbots for example was a challenge. Using phantomjs also gives us more control over things. Whether its the best situation, I don't know.
Regarding behat, I believe that you are abusing behat for something its not meant to be used for, see https://docs.google.com/document/d/17raktxD51MP0WUvqtNQFsCzevYgDNkvl7jOu...

Thanks for the information. I was wonder if "./vendor/jcalderonzumba/gastonjs/src/Client/main.js" provided extra report info for Drupal testbots. I will look into it when I have free time (will not have much within these one or two months. If someone interested, please help to diva into it.)

I started a quick test here: #2775653: [PP-2] JavascriptTests with webDriver and it's worked.

When you say "Create a new API", what do you have in mind?

Don't override default click? and a new API called hackedClick, JSClick? It's used for the special case only.

(By the way, I don't know if there're any architecture difference but it has no issue in webDriver.)

samuel.mortenson’s picture

To test invisible (display:none) element, we will get a false result.

Clicking an empty element and clicking an element with nested divs are different and unrelated errors, this try/catch should not change the normal errors associated with empty elements.

Don't override default click? and a new API called hackedClick, JSClick? It's used for the special case only.

If we do this, we'll need to respond to issues from contrib and core contributors telling them to use the new method. Then when GastonJS is actually fixed, we would have to deprecate that method and have everyone who wrote tests using it use the normal click method. To be clear we're not changing the default click behavior - we're only catching an exception specific to this bug and simulating a click with JavaScript to work around the current GastonJS error. Any errors normally thrown by click() will still be thrown.

droplet’s picture

Testing testbot. I expected only testBrokenImageClick will be failed.

(the uploaded patch has wrong code on testNestedEmptyClick. But it doesn't matter, it has been tested in Patch #5.)

Status: Needs review » Needs work

The last submitted patch, 25: verifyTest.patch, failed testing.

droplet’s picture

#25 failing as expected.

Nested with Empty element: Zero dimension
Nested with Broken image: Zero dimension

Both should be not clickable.

According to #25, the better workaround is mute the error on the clickable element with zero dimension.

So that for this issue: #2421427: Improve the UX of Quick Editing single-valued image fields
I bet we should drop a timer there to wait for image loading.
this article may help: http://docs.behat.org/en/v2.5/cookbook/using_spin_functions.html

samuel.mortenson’s picture

@droplet It's interesting to see the bug narrowed down - but how users run into this problem isn't what this patch is addressing, it's protecting test-writers regardless of what HTML they're clicking. If I'm testing a module, I'm not responsible for the fact that it displays a broken image, a zero-width div, or other strange HTML that causes this exception - I know that I can click it in my browser normally, and expect that Drupal's Javascript testing framework can handle that same "click()" operation.

Edit - To your point though, I like that we're getting to the bottom of the replication steps. If we can re-write the fix to target specific problems in an efficient way, I'm up for it. It's better to understand the problem than to commit a fix without doing all the research we can. Thanks for the example tests!

droplet’s picture

Cool. I fixed the Quick Editing issue.

IMO, we need not fix this issue, or say no rush for a fixing.

About the fixes, we should make it same behavior as SENDKEY to click on white space. We could report it to GastonJS to mute the error message, it seems not a bug.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Wim Leers’s picture

droplet’s picture

Issue tags: -blocker

Removing the blocker, see my comments in https://www.drupal.org/node/2789381#comment-11642125

It's very good to keep this issue alive until we run into the real limitations. We should not always try JS to fake the testing engine before real debugging.

tedbow’s picture

Status: Needs work » Needs review
Issue tags: +blocker
FileSize
2.64 KB

@droplet adding back the blocker tag
I think @Wim Leers added that in relation to #2785589: Fix js and jsdoc of outside-in module not #2789381: Submitting an ajax dialog form in Javascript test throws a GastonJS error
#2785589: Fix js and jsdoc of outside-in module has both problems. You can't write a JS test that use a click, this issue AND you can't submit a form via JS test.

The patch needed re-roll for 8.2.x.

I can tested #2785589: Fix js and jsdoc of outside-in module with the patch and it allows testing Outside In for both the clicking a link and submitting a form.

IMO, we need not fix this issue, or say no rush for a fixing.

It seems like this important because it stops people writing JS tests. We have no idea how many people hit this error trying to write test for core or contrib but never make it to this issue.

droplet’s picture

Issue tags: -blocker

That's GREEN in #2785589: Fix js and jsdoc of outside-in module without patching. Remove `blocker` tag again :)

It seems like this important because it stops people writing JS tests. We have no idea how many people hit this error trying to write test for core or contrib but never make it to this issue.

I still think it's not an issue and you will agree now? (It hints you writing wrong testcases)

For edge cases (we didn't find any yet?), the best workaround still is a new helper: `JSClick()`

No visual debugging is the real reason that stops people writing JS tests. Remember that the testbot running with system base theme, and we can't active it from UI right? Even no simple way from setting file?

For example this issue: #2789381: Submitting an ajax dialog form in Javascript test throws a GastonJS error
It's really not clickable from browsers. Not a test engine bug, instead of a JS lib & theme bug.

the right person should kick it into experiment module also: #2775653: [PP-2] JavascriptTests with webDriver
Even not using in d.org. It will help developers to write tests.

Also drupal missing an interactive console to write test:
http://codeception.com/docs/reference/Commands#Console

and unable to use Psysh :( We waste a lot of time on test reboot. Even I can't attach Xdebug to do it. Maybe someone able to give me some hints?

dawehner’s picture

@droplet
Before continuing please read xjmdrupal.org/blog/someone-worked-hard-on-it ... people work on stuff, try to improve stuff, be part of it instead of just providing negative thoughts.

I updated the issue summary on #2775653: [PP-2] JavascriptTests with webDriver, so let's collaborate there.

Maybe someone able to give me some hints?

This is my xdebug config, and yes I'm often quite lazy :)

[xdebug]
zend_extension="/usr/local/opt/php56-xdebug/xdebug.so"
xdebug.remote_enable=1
xdebug.max_nesting_level=256
xdebug.remote_autostart=1
droplet’s picture

@dawehner,

Sorry if my word makes you feel sad. I didn't try to make anyone unhappy. :) I'm thinking in both minds all time.

Positive thoughts are good. Sometimes negative thoughts also good. It helps gather the same feeling people to work on the same topic. OpenSource still has its own rule, not the single person able to "work on stuff, try to improve stuff". We needed supporting from those right person.

I updated the issue summary on #2775653: JavascriptTests with webDriver, so let's collaborate there.

Thanks!!! will do.

This is my xdebug config, and yes I'm often quite lazy :)

Thanks. I was trying to execute code in real time but it has limitations on PHPUnit:
https://www.jetbrains.com/help/phpstorm/2016.2/evaluating-expressions.ht...

Psysh is my new hope but it simply crashed in Drupal tests. Seems not easy to find out the problem.

dawehner’s picture

Sorry if my word makes you feel sad. I didn't try to make anyone unhappy. :) I'm thinking in both minds all time.

I'm not talking about myself. This is really just about your general behaviour. Thank about what you write :)

Psysh is my new hope but it simply crashed in Drupal tests. Seems not easy to find out the problem.

Seems unrelated. Do you think about allowing to open up a psysh instance out of testing?

droplet’s picture

FileSize
108.36 KB

Maybe I should start a new issue to discuss it. Very last unrelated comment on this issue thread :)

Do you think about allowing to open up a psysh instance out of testing?

Yup! @see GIF

** This isn't a Codeception feature. I dropped `\Psy\Shell::debug(get_defined_vars());` into my tests directly.

droplet’s picture

OK. Here're my new thoughts & summary:

Using https://github.com/jquery/jquery-simulate for special cases. It sends a MouseEvents.

In gastonjs, there's similar API (non-behat-mink standard API):

 public function clickCoordinates($coordX, $coordY) {
    return $this->command('click_coordinates', $coordX, $coordY);
  }

(however, we unable to find the element X & Y coords in PHP testcases)

My assumption on GastonJS & PhantomJS:
Success:
`click()` -> ELEMENT CLICK -> CLICKABLE -> return TRUE.

Failure:
`click()` -> ELEMENT CLICK -> NOT CLICKABLE -> return FALSE.

When will the element not clickable?
E1. HIDDEN
E2. ZERO dimension
E3. PointerEvent: none
E4. Clicking the link or buttons in nested layers (topmost DIV blocking the clicking)
E5. (UNCONFIRMED) Rendered outside the browser windows (viewports)

Why `jQuery.trigger('click')` works?
A1. It triggers a JS event on target elements instead MouseEvents.

Why don't use `jQuery.trigger('click')`
B1. `jQuery.trigger('click')` able to click elements that Human can't do it with mouse devices.

Why don't hack `click()` directly?
C1. Prevent we make mistakes on test cases. And it converted our test cases to PHP-BROWSER like behaviors

Why use `jquery-simulate`?
D1. Mute the exceptions on "E3"

When to use `jquery-simulate`?
F1. So far until this moment, I think only the E3.

Hoping you have a Happy Weekend!!

OFFTOPIC:
About `\Psy\Shell` on #39. I think we able to write a bootstrap script to init test (or empty test env) inside `\Psy\Shell`, instead of a breakpoint in tests.

mradcliffe’s picture

When will the element not clickable?
E1. HIDDEN
E2. ZERO dimension
E3. PointerEvent: none
E4. Clicking the link or buttons in nested layers (topmost DIV blocking the clicking)
E5. (UNCONFIRMED) Rendered outside the browser windows (viewports)

Also, there is a bug that does not correspond to any of these conditions within phantom, which is the original bug report that I filed. I looked at it again today (my original report was an angular.js front end and behat/mink/phantomjs running against it).

Checking in Firefox, I confirmed that E1, E2, E3 and E5 do not apply in my original case. I added a z-index to the anchor element and still ran into the issue so I think that E4 does not apply as well. There is some other case with an anchor element wrapping an image element.

droplet’s picture

@mradcliffe,

I guess you ran into E2. @see #25 & #27

From your github reports:

I think a work-around would be to change the image markup to use CSS background instead.

Used as background, it has to apply dimension value to A tag. (now with dimension)
one of above reports is clicking "an anchor element wrapping an image element." and it passed: @see: https://www.drupal.org/node/2785589

(**I think it will not, but checking if network res with slow downloading is an issue)

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

droplet’s picture