The link on the Image tag is redirecting to an undefined page from the node preview screen

I just ran into this, and the fix is pretty simple. In my case the issue was the event was being triggered by clicking on a <span> tag within an <a> tag. For example:

<a href="/blog" class="link usa-nav__link" data-drupal-link-system-path="node/32"><span>Blog</span></a>

Imagine that the link has some padding around the span with css. If you click on the padded area around the span, node preview redirects work just fine, but if you clicked exactly on the area of the span, the span itself would end up being the event target. Currently, the JS redirects to event.target.href which in the case of the span click, would be undefined.

The fix for me was to instead use event.currentTarget.href which will always be the link.

I looked into updating the tests to cover this and while I can add the new link type, it seems that the UIHelperTrait::clickLink() method would also need to be extended to provide the option to click the element within the link vs the link itself.

I tried to understand how and why this is happening but could not figure it out. I think steps to reproduce would help and would also allow this to be marked as a bug report.

Re #8

I looked into updating the tests to cover this and while I can add the new link type, it seems that the UIHelperTrait::clickLink() method would also need to be extended to provide the option to click the element within the link vs the link itself.

No need to make clickLink() click non-links, you can target the element directly and click it.

$this->getSession()->getPage()->find('css', 'SELECTOR OF THE ELEMENT INSIDE LINK')->click()

Uploading a tests only patch. The MR already demonstrates to pass all tests, but without the change the patch should fail.

It being an actual patch will probably help 😅

The last submitted patch, 15: 3308432-tests-only.patch, failed testing. View results →

LGTM in that case 😉 not a PHPUnit expert, so there might be some nits/best practices that I'm missing.

Actually, I have one question.

Why would an anchor link open the pop-up that you are leaving the page? There is no reason for that to be happening.
The second tests passes, which is default behaviour so that's good.
And the last test with the span failed as expected.

The last submitted patch, 15: 3308432-tests-only.patch, failed testing. View results →

Will hide the patch so it won't re-run

Triaging the RTBC issue queue.

I read the issue summary and the comments. I have not tried to reproduce the problem.

I look at the patch and see that two new tests, testPreviewAnchorLinks and testPreviewChildElementLinks are added. Looking at the test results only one test is failing, testPreviewChildElementLinks. That make me wonder why the other test was added. That could use an explanation.

Also, since these are functional tests can it be done in a single test?

Setting to NW for the above questions.

The main issue is that the javascript didn't target the bubbled event data correctly, so some types of links would fail. That was fixed, and the question came to should there be a test to cover this situation?

When I looked at the existing tests, there was one test with two asserts for normal and anchor links. Adding in this new type of link would then make that single test have 3 asserts now, but also involve navigating backwards to the previous page after the second assert.

It was more straightforward to just split each link into it's own test.

devkinetic → changed the visibility of the branch 3308432-the-link-on to hidden.

devkinetic → closed merge request !4034

Updated the issue fork and rebuilt MR based on 11.x.

Here is also a patch against 10.3.x for anyone who needs it.

Hiding patches for clarity.

#22 still seems to be relevant

Instead of breaking out could we just add to the existing test, especially since they are javascript tests and require a full bootstrap.

Issue summary appears to be missing proposed solution from the standard issue template can that be added please

IS updated and combined into single test.

Feedback appears to be addressed.

I read the IS, comments and MR. There are no unanswered questions. My question about the tests in #22 was answered but then later the tests were combined. That seems OK in this case.
Leaving at RTBC.

nod_ → closed merge request !8578

Committed and pushed bd6abf7c4d to 11.x and d890962445 to 10.4.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.