akeeba / panopticon

Self-hosted site monitoring and management
GNU Affero General Public License v3.0
39 stars 20 forks source link

RFC Links open in new window #449

Closed brianteeman closed 9 months ago

brianteeman commented 9 months ago

Several links have an icon to visually indicate that the link opens in a new tab. But not all of them. See the links in the footer and the links in the list of extensions.

My first thought was to add the hardcoded icon to those links as well but its not reall necessary and its not great in some cases where the markup would have to be in the language string.

Instead we can use css to add the icons for all of the links (its what I wrote for Joomla). It's DRY and you don't have to remember to add the icons.

a[target="_blank"]::after{
    content: "\f08e";
    padding-inline-start: 3px;
    font-family: "Font Awesome 6 Free";
    font-size: 14px;
    font-weight: 900;
}

Accessibility enhancement

To satisfy a11y it would be good to add an announcement that the link opens in a new window. We can do this with simple javascript


function addNewTabMessage(link) {
  if (!link.querySelector('.visually-hidden')) {
    link.insertAdjacentHTML('beforeend', '<span class="visually-hidden">(opens in a new tab)</span>');
  }
}

document.querySelectorAll('a[target="_blank"]').forEach(link => {
  addNewTabMessage(link);
});

image

Security enhancement

Finally we can add an additional function to the js to add the rel=noopener to the link


function addNoOpener(link) {
  let linkTypes = (link.getAttribute('rel') || '').split(' ');
  if (!linkTypes.includes('noopener')) {
    linkTypes.push('noopener');
  }
  link.setAttribute('rel', linkTypes.join(' ').trim());
}

function addNewTabMessage(link) {
  if (!link.querySelector('.visually-hidden')) {
    link.insertAdjacentHTML('beforeend', '<span class="visually-hidden">(opens in a new tab)</span>');
  }
}

document.querySelectorAll('a[target="_blank"]').forEach(link => {
  addNewTabMessage(link);
});
brianteeman commented 9 months ago

my js and css can i am sure be improved upon

nikosdion commented 9 months ago

I don't like using JS for all that. It's finicky. It cannot run until the entire DOM is loaded, and when we have dynamic content (e.g. through Petite Vue) we need to re-run all of this which has a considerable performance impact.

I would much rather do that manually. I plan on adding more interactivity with Petite Vue in the future. Let me see if I can start with the CSS and continue to normalising the link attributes.

nikosdion commented 9 months ago

BTW, there is no need for rel="noopener". See https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/rel/noopener

Setting target="_blank" on <a> elements now implicitly provides the same rel behavior as setting rel="noopener" which does not set window.opener.

This is the behaviour of all browsers since 2017, so we're talking about more than 6 years ago. Arguably, if someone is using a 6+ years old browser they can't use Panopticon (we're targeting the browser released the last 5 years), and they have far worse security concerns than the opener window object being available.

Yes, I am aware that Edge only started working like that in 2020, namely when it started using Chromium. Classic Edge is obsolete, dead, buried, and its rotting corpse removed by Microsoft from all Windows 10 computers. Again, this means that the only way someone would use an affected Edge version is if they stuck on Windows 8.1 or a very, very early version of Windows 10 which means that their computer is compromised six ways to the underworld.

So, no need for rel="noopener".

nikosdion commented 9 months ago

I am also very weary about using CSS to add content after the link. This is now in the DOM, visible to the screen reader. It will announce an icon that is not there.

I think I actually prefer what we're doing now. Mark external links as long as they are not inline in messages (they detract from the text flow), or in the footer (where you can reasonably expect to find external links).

brianteeman commented 9 months ago

Interestingly I chose to use sql and not css to add the "opens in new window" as everywhere says css generated content should not be used as screen readers do not see anything added with :before or :after.

But I just checked the test results and interestingly that problem only exists in IE https://www.powermapper.com/tests/screen-readers/content/css-generated-content/ so it would be perfectly safe to use.

I didnt realise that tabnapping was no longer a thing so thats good to know

nikosdion commented 9 months ago

Error! Logical fallacy detected! You have given me two mutually exclusive conditions which you claim are simultaneously true. Does not compute.

If :before and :after content is invisible to screen readers then your recommendation, also recommended by FontAwesome, to use aria-hidden="true" on icon SPANs is nonsense since the icon content is rendered with :after which you're telling me is invisible to screen readers. If it's invisible anyway, using aria-hidden is superfluous, therefore a bug. In this case what you did in your CSS for external links makes sense from an accessibility point of view.

If, however, the recommendation is sensible – which I strongly suspect it is – it is only so because :before and :after are visible to screen readers. However, in this case, the CSS you have for external links is wrong as it creates :after content which is visible to the screen reader (notably, it is identical to how FontAwesome icons are rendered, merely substituting :after for :before).

When confronted with mutually exclusive advice I resort to experimentation. Since you've pretty much forbade me from using a screen reader as I am sighted, would you please find a person with visual impairment who is a frequent screen reader user to try both markups and tell us which one actually works?

nikosdion commented 9 months ago

Sorry, I didn't mean to annoy you, I am just wondering what is the right way to go about accessibility of icons. I thought I understood the reason we use aria-hidden="true", but not I am not sure what is true :/

nikosdion commented 9 months ago

So, according to some very quick research, at least since 2017 the pseudo-element content is being announced by screen readers, unless you're using Firefox:

The recommendation is to only use pseudo-content for decorative purposes with the note that some screen readers announce it. I would posit that this recommendation made sense ten years ago, but now that CSS is almost a Turing-complete language (we already have counters and arithmetic) this is not realistic.

So, given the fact that all sources – including Accessible Web – say that at the very least some screen readers will announce pseudo-content, we cannot use the CSS approach. We would either have to use a JS approach which has a performance penalty, exponentially increasing in long content / many elements (since every DOM manipulation results in reflowing the page content…), or we have to stick to the current approach which may be inconsistent but does not have accessibility or performance issues.

Basically it comes down to this: Accessibility. Performance. Consistency. Pick two.

I will go with accessibility and performance. These are far more important to me than consistency at any cost. At least until someone comes up with a better solution. I am always open to solutions 😉