Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(atomic): using _blank target on custom recommendation link template opens two tabs on click #4953

Merged
merged 7 commits into from
Feb 13, 2025

Conversation

lbergeron
Copy link
Contributor

@lbergeron lbergeron commented Feb 10, 2025

SVCC-4611

When a recommendation list uses a custom link template to open recommendations in a new tab, clicking the recommendation opens two tabs instead of one.

How to reproduce the issue

  1. Override the atomic-recs-result-template as per documentation.
<atomic-recs-result-template>
  <template slot="link">
    <atomic-result-link>
      <a slot="attributes" target="_blank"></a>
    </atomic-result-link>
  </template>
  <template>... recommendation content is there ...</template>
</atomic-recs-result-template>
  1. If you click the content of the recommendation, it open two tabs. If you click the margin (between the border and the content), then it opens a single tab.

Problem

The recommendation list components rely on DisplayGrid for their rendering. The problem is related to this line where DisplayGrid forces a click on the recommendation content. When the recommendation content overrides the link slot, it already has an event handler for the click. It means that, when clicking the recommendation content, its click handler is invoked (opening the first tab). Then, the event propagates to the parent (DisplayGrid) which forces a click event on the content, causing another tab to open.

Proposed solution

The proposed solution is to modify the recommendation lists to detect whether the link slot is modified. If it is, then the stopPropagation property is set on the child atomic-recs-result component. It prevents the click event from being propagated to the parent when clicking a recommendation content. However, when outside the recommendation content (i.e., the margin), DisplayGrid still forces the click on the content, opening a single tab.

Copy link

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Live demo links

Bundle Size

File Old (kb) New (kb) Change (%)
case-assist 244 244 0
commerce 355.3 355.3 0
search 415.2 415.2 0
insight 406.4 406.4 0
recommendation 256.2 256.2 0
ssr 409 409 0
ssr-commerce 373 373 0

SSR Progress

Use case SSR (#) CSR (#) Progress (%)
search 39 44 89
recommendation 0 4 0
case-assist 0 6 0
insight 0 27 0
commerce 0 15 0
Detailed logs search : buildInteractiveResult
search : buildInteractiveInstantResult
search : buildInteractiveRecentResult
search : buildInteractiveCitation
search : buildGeneratedAnswer
recommendation : missing SSR support
case-assist : missing SSR support
insight : missing SSR support
commerce : missing SSR support

Copy link
Contributor

@dmgauthier dmgauthier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I certainly wouldn't find anything more clever myself.
But i'm always sad about adding obscure stopPropagation with sometime very obscure purpose.

Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While doing some checks, I think about 2w ago; I noticed some behaviour was not OK overall with the link slot.

Overall, the feature is supposed to work as such:
When an atomic-[result|product]-link is provided into a slot=link template, it should be used for any click occurring anywhere on the rendered item, notwithstanding the anchor tag and other clickable that do handle the click first (and include a stopPropagation).

I do not think we narrowed down the root cause of this issue, and I think the feature might be broken. If that's the case, I think it'd be best to send back the ticket to @coveo/search team so that we can do a holistic and comprehensive fix on this one.

Copy link
Contributor

@fbeaudoincoveo fbeaudoincoveo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested and the issue is also reproducible in the atomic-commerce-recommendation-list component.

Would you mind fixing it as well (and adjusting the tests), @lbergeron ?

@lbergeron
Copy link
Contributor Author

I tested and the issue is also reproducible in the atomic-commerce-recommendation-list component.

Would you mind fixing it as well (and adjusting the tests), @lbergeron ?

Thanks for the pointer, will look into it.

@louis-bompart
Copy link
Collaborator

Overall, the feature is supposed to work as such:
When an atomic-[result|product]-link is provided into a slot=link template, it should be used for any click occurring anywhere on the rendered item, notwithstanding the anchor tag and other clickable that do handle the click first (and include a stopPropagation).

I do not think we narrowed down the root cause of this issue, and I think the feature might be broken. If that's the case, I think it'd be best to send back the ticket to @coveo/search team so that we can do a holistic and comprehensive fix on this one.

On that one, I was off the mark. forget about it 😅

@lbergeron lbergeron added this pull request to the merge queue Feb 13, 2025
Merged via the queue into master with commit de34ef6 Feb 13, 2025
97 checks passed
@lbergeron lbergeron deleted the fix/SVCC-4611 branch February 13, 2025 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants