Problem/Motivation

On the node preview screen, linked images redirect to an undefined page.

As we can see in the attached screenshot, the direct link with "Click here" is working but the link with image tag get redirecting to the node preview undefined page.
/node/preview/86c2fbcf-11e4-4a56-a4c8-93ab8f382baa/undefined

Steps to reproduce

For demonstration, I will be using a custom block set to full html format. The block has nothing to do with the issue, it is just used to quickly place the testing code onto a page.

  1. Add the following code to a custom block, configured to display on node pages:
    <h1>Normal Link</h1>
<a href="/">Test</a>
<h1>Image Link</h1>
<a href="/"><img src="/path/to/an/actual/image.png"></a>
<h1>Child Element Link</h1>
<a href="/" style="padding:20px"><span>Test 2</span></a>
  2. Edit a node (that is not the home page)
  3. Click preview button
  4. Click the "normal link", when the modal pops up, click "leave". Observe you are correctly redirected.
  5. Use your browser back button to return to preview mode.
  6. Click the "image link", when the modal pops up, click "leave". Observe you are NOT correctly redirected.
  7. Use your browser back button to return to preview mode.
  8. Click the "child element link", specifically the padded area around the text. When the modal pops up, click "leave". Observe you are correctly redirected.
  9. Use your browser back button to return to preview mode.
  10. Click the "child element link", specifically text itself. When the modal pops up, click "leave". Observe you are NOT correctly redirected.

Proposed resolution

The issue is that the click can come from the link element or a child of it. The JS behavior is attached only to the link. When child element is clicked, event.target.href is undefined. Instead refer to event bubbling and switch to event.currentTarget.href which will always refer to the element that the behavior is attached to.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#42 3308432-42.patch504 byteskeshavv
#29 3308432-10.3.x-node-preview-image-link-undefined.patch2.76 KBdevkinetic
#15 3308432-tests-only.patch2.33 KBbramdriesen
#14 3308432-tests-only.txt2.33 KBbramdriesen
Screenshot 2022-09-07 at 10.35.53 PM.png241.48 KBshifali baghel
Screenshot 2022-09-07 at 10.31.14 PM.png434.99 KBshifali baghel

Issue fork drupal-3308432

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Shifali Baghel created an issue. See original summary.

cilefen’s picture

cilefen commented
Category: Bug report » Support request
Priority: Major » Normal
Issue tags: -Drupal 9

This would require steps to reproduce to be taken on as a bug. Can you please add to the issue summary, the steps to set up this bug, starting with a standard Drupal installation?

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

cilefen’s picture

cilefen commented
Status: Active » Postponed (maintainer needs more info)
cilefen’s picture

cilefen commented
Status: Postponed (maintainer needs more info) » Closed (outdated)
devkinetic’s picture

devkinetic commented
Status: Closed (outdated) » Active

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"><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 tested with an img tag link as well to verify the fix for the initially described issue.

devkinetic opened merge request !4034

devkinetic’s picture

devkinetic commented

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.

devkinetic’s picture

devkinetic commented
Status: Active » Needs review
bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Needs review » Needs work
Issue tags: +Needs steps to reproduce

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.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

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()
devkinetic’s picture

devkinetic commented
Category: Support request » Bug report
Issue summary: View changes
devkinetic’s picture

devkinetic commented
Status: Needs work » Needs review
Issue tags: -Needs steps to reproduce
bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
StatusFileSize
new 3308432-tests-only.txt2.33 KB

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

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
StatusFileSize
new 3308432-tests-only.patch2.33 KB

It being an actual patch will probably help 😅

Status: Needs review » Needs work

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

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

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

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Needs work

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.

Ignore that. I missed the not in pageTextNotContains 🙈

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

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

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

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

quietone’s picture

quietone commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

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.

devkinetic’s picture

devkinetic commented

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.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

devkinetic opened merge request !8578

devkinetic closed merge request !4034

devkinetic’s picture

devkinetic commented
Status: Needs work » Needs review

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

devkinetic’s picture

devkinetic commented
StatusFileSize
new 3308432-10.3.x-node-preview-image-link-undefined.patch2.76 KB

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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

devkinetic’s picture

devkinetic commented
Issue summary: View changes
devkinetic’s picture

devkinetic commented
Status: Needs work » Needs review

IS updated and combined into single test.

devkinetic’s picture

devkinetic commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Feedback appears to be addressed.

quietone’s picture

quietone commented

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

  • nod_ committed d8909624 on 10.4.x 
    Issue #3308432 by devkinetic, BramDriesen, Shifali Baghel, cilefen,...

  • nod_ committed bd6abf7c on 11.x 
    Issue #3308432 by devkinetic, BramDriesen, Shifali Baghel, cilefen,...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Fixed

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

nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: The link on the Image tag is redirecting to an undefined page from the node preview screen. » The link on the Image tag is redirecting to an undefined page from the node preview screen

(edit) update credits

Status: Fixed » Closed (fixed)

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

keshavv’s picture

keshavv commented
StatusFileSize
new 3308432-42.patch504 bytes

After implementing this commit, clicking on simple links during preview mode results in an undefined redirect.
Steps to Reproduce:
1. Edit any node.
2. Click the Preview button.
3. Within the preview, click on any basic link or admin links.
4. You are redirected to: /node/preview/UUID/undefined

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented

@keshavv this ticket is closed for almost a year. It's recommended to create a new ticket and reference to this issue. Also note that the patch workflow is deprecated, you should use merge requests.

norman.lol’s picture

norman.lol
he/him
Primary language German
Location Berlin
commented

Follow-up: #3541343: "Leave preview" points to undefined instead of node's edit page