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

Implement External Link Icon #504

Closed
iksaku opened this issue Jun 18, 2020 · 6 comments
Closed

Implement External Link Icon #504

iksaku opened this issue Jun 18, 2020 · 6 comments
Assignees
Labels
documentation Documentation issues or updates implemented Change has been implemented

Comments

@iksaku
Copy link
Contributor

iksaku commented Jun 18, 2020

Description
External Links are a pretty useful feature to prevent security issues when linking other websites, however, users could be informed about certain links being external to the site they're currently reading.

The proposal is to implement an extra configuration parameter with a similar behaviour to Heading Permalink Extension, so if the developer opt-in, they can generate an icon next to their external links when rendering markdown content.

Example
I made a custom link rendering interceptor (using some little hacks) which has a high priority registration. This custom renderer fires before the traditional Link renderer, but returns null so the later executes and does the heavy link rendering work:

class ExternalLinkInterceptor implements InlineRendererInterface
{
    /**
     * {@inheritdoc}
     */
    public function render(AbstractInline $inline, ElementRendererInterface $htmlRenderer)
    {
        if (!($inline instanceof Link)) {
            throw new \InvalidArgumentException('Incompatible inline type: '.get_class($inline));
        }

        if ($inline->getData('external')) {
            $inline->appendChild(new ExternalLinkIconElement());
        }

        return null; // Little hack to intercept rendering right before running base LinkRenderer
    }
}

If the Link element has the external data set, we append a dummy ExternalLinkIcon element:

class ExternalLinkIconElement extends AbstractInline
{
    public function isContainer(): bool
    {
        return true;
    }
}

Finally, a ExternalLinkIconRenderer renders the custom icon I want:

class ExternalLinkIconRenderer implements InlineRendererInterface
{
    public function render(AbstractInline $inline, ElementRendererInterface $htmlRenderer)
    {
        if (!($inline instanceof ExternalLinkIconElement)) {
            throw new \InvalidArgumentException('Incompatible inline type: '.get_class($inline));
        }

        return new HtmlElement('span', [
            'class' => 'fas fa-external-link-alt fa-sm ml-1',
        ]);
    }
}

The result can be seen in one of my blog posts:
image

Final Note
The process can be simplified a lot if a custom icon node is appended while processing an external link, and rendering will be pretty straight forward.

Not sure if this can still land for v1.5, but I can open a PR during the weekend as an initial proposal for this feature if the request is approved.

@iksaku iksaku added the enhancement New functionality or behavior label Jun 18, 2020
@markhalliwell
Copy link
Contributor

markhalliwell commented Jun 18, 2020

I'm not entirely sure this is needed. I do the same thing, but with CSS:

a[target="_blank"]::after,
a.external::after {
  @extend .glyphicon;
  content: "\e164";
  margin-left: .5em;
  margin-right: .25em;
}

@markhalliwell
Copy link
Contributor

It can even be done with Font Awesome (just switch ::before for ::after in the examples):

FA4: https://stackoverflow.com/a/50559557
FA5: https://stackoverflow.com/a/47723704

@colinodell
Copy link
Member

If that solution works for you @iksaku perhaps we just need to include this example in our docs?

@colinodell
Copy link
Member

I've added an example of using CSS to our docs (via commit bd34d0e)

@colinodell colinodell added documentation Documentation issues or updates implemented Change has been implemented and removed enhancement New functionality or behavior labels Jun 20, 2020
@colinodell colinodell self-assigned this Jun 20, 2020
@markhalliwell
Copy link
Contributor

@iksaku, we do want to thank you for creating this issue and example implementation; you clearly put a lot of thought behind this. I feel we should probably clarify the above decision a bit as @colinodell and I discussed this and agreed that this kind of cosmetic effect should be left to the front-end to implement as it is much easier to target and style links that open new windows or a specific class using CSS pseudo-elements.

This allows the visual aspect of this enhancement to live in the front-end, where assets like Font Awesome may or may not change over time. Forcing the creation of markup at this level can potentially cause rendered markdown to be inherently linked with a specific front-end library or version. In CMS where rendered content can be archived or cached, this can cause a disconnect if the site were ever to be re-themed and/or the front-end library used for icons is replaced with something else. Thus, the rendered icon markup becomes useless.

There are also concerns about potential future security issues that may arise due to the planned allowance of runtime configuration overrides via front matter (#497)

We're also aware that Heading Permalink currently has this ability via the inner_contents config. We're in the middle of discussing whether this should be deprecated in favor of a pure CSS approach as well due to the fact that it suffers from all the same issues mentioned above as well.

@iksaku
Copy link
Contributor Author

iksaku commented Jun 20, 2020

It’s okay, thanks for the clarification thought!

It makes sense as it’s a potential XSS target, so better walk far from it.

Keep up the good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation issues or updates implemented Change has been implemented
Projects
None yet
Development

No branches or pull requests

3 participants