1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-22 14:52:41 +01:00

Remarkup: make less internal links open in new tabs

Summary:
This is an attempt to improve the default behavior in Remarkup about
links. It does not change any behaviors manually specified in the engine
and it does not change any behaviors related to external domains.

As default, now these kind of links will open in the same tab:

- anchors
- relative URLs
- absolute URLs pointing to the base-URI domain

All the other cases are kept as before - so they open in another tab.

In short, assuming you are we.phorge.it, here the changes:

|      |https://gnu.org|[[changelog/]]|[[#anchor|#anchor]]|https://we.phorge.it/|[[/config/|/config/]]|
|Before|external       |internal      |internal           |external             |external             |
|After |external       |internal      |internal           |**internal**         |**internal**         |

This situation can further improve but it already covers most of the
cases where most users do not expect to break their navigation into
several tabs. Moreover, if an user wants to open a link in another
window, no one prevents from using the middle mouse button,
or CTRL+click or any other nice really basic feature from their browser.

Also, this change introduces a new CSS class, allowing web designers
to style these external resources.

Example CSS rule to try:

```css
.remarkup-link-ext::before {
    content: "[external] ";
}
```

Closes T15161
Closes T15182

Test Plan:
- Copy the example text from this Task: https://we.phorge.it/T15161
- Verify that "internal resources" are internal links as default now
- Verify that "external resources" are still external links as before

Reviewers: O1 Blessed Committers, Cigaryno, avivey, speck

Reviewed By: O1 Blessed Committers, Cigaryno, speck

Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno

Maniphest Tasks: T15182, T15161

Differential Revision: https://we.phorge.it/D25118
This commit is contained in:
Valerio Bozzolan 2024-07-29 12:39:05 +02:00
parent 59678094fb
commit 349f006904
6 changed files with 178 additions and 9 deletions

View file

@ -5799,6 +5799,8 @@ phutil_register_library_map(array(
'PhutilTranslatedHTMLTestCase' => 'infrastructure/markup/__tests__/PhutilTranslatedHTMLTestCase.php', 'PhutilTranslatedHTMLTestCase' => 'infrastructure/markup/__tests__/PhutilTranslatedHTMLTestCase.php',
'PhutilTwitchAuthAdapter' => 'applications/auth/adapter/PhutilTwitchAuthAdapter.php', 'PhutilTwitchAuthAdapter' => 'applications/auth/adapter/PhutilTwitchAuthAdapter.php',
'PhutilTwitterAuthAdapter' => 'applications/auth/adapter/PhutilTwitterAuthAdapter.php', 'PhutilTwitterAuthAdapter' => 'applications/auth/adapter/PhutilTwitterAuthAdapter.php',
'PhutilURIHelper' => 'infrastructure/parser/PhutilURIHelper.php',
'PhutilURIHelperTestCase' => 'infrastructure/parser/__tests__/PhutilURIHelperTestCase.php',
'PhutilWordPressAuthAdapter' => 'applications/auth/adapter/PhutilWordPressAuthAdapter.php', 'PhutilWordPressAuthAdapter' => 'applications/auth/adapter/PhutilWordPressAuthAdapter.php',
'PhutilXHPASTSyntaxHighlighter' => 'infrastructure/markup/syntax/highlighter/PhutilXHPASTSyntaxHighlighter.php', 'PhutilXHPASTSyntaxHighlighter' => 'infrastructure/markup/syntax/highlighter/PhutilXHPASTSyntaxHighlighter.php',
'PhutilXHPASTSyntaxHighlighterFuture' => 'infrastructure/markup/syntax/highlighter/xhpast/PhutilXHPASTSyntaxHighlighterFuture.php', 'PhutilXHPASTSyntaxHighlighterFuture' => 'infrastructure/markup/syntax/highlighter/xhpast/PhutilXHPASTSyntaxHighlighterFuture.php',
@ -12705,6 +12707,8 @@ phutil_register_library_map(array(
'PhutilTranslatedHTMLTestCase' => 'PhutilTestCase', 'PhutilTranslatedHTMLTestCase' => 'PhutilTestCase',
'PhutilTwitchAuthAdapter' => 'PhutilOAuthAuthAdapter', 'PhutilTwitchAuthAdapter' => 'PhutilOAuthAuthAdapter',
'PhutilTwitterAuthAdapter' => 'PhutilOAuth1AuthAdapter', 'PhutilTwitterAuthAdapter' => 'PhutilOAuth1AuthAdapter',
'PhutilURIHelper' => 'Phobject',
'PhutilURIHelperTestCase' => 'PhabricatorTestCase',
'PhutilWordPressAuthAdapter' => 'PhutilOAuthAuthAdapter', 'PhutilWordPressAuthAdapter' => 'PhutilOAuthAuthAdapter',
'PhutilXHPASTSyntaxHighlighter' => 'Phobject', 'PhutilXHPASTSyntaxHighlighter' => 'Phobject',
'PhutilXHPASTSyntaxHighlighterFuture' => 'FutureProxy', 'PhutilXHPASTSyntaxHighlighterFuture' => 'FutureProxy',

View file

@ -44,16 +44,16 @@ final class PhutilRemarkupDocumentLinkRule extends PhutilRemarkupRule {
protected function renderHyperlink($link, $name) { protected function renderHyperlink($link, $name) {
$engine = $this->getEngine(); $engine = $this->getEngine();
$is_anchor = false; $uri = new PhutilURIHelper($link);
if (strncmp($link, '/', 1) == 0) { $is_anchor = $uri->isAnchor();
$starts_with_slash = $uri->isStartingWithSlash();
if ($starts_with_slash) {
$base = phutil_string_cast($engine->getConfig('uri.base')); $base = phutil_string_cast($engine->getConfig('uri.base'));
$base = rtrim($base, '/'); $base = rtrim($base, '/');
$link = $base.$link; $link = $base.$link;
} else if (strncmp($link, '#', 1) == 0) { } else if ($is_anchor) {
$here = $engine->getConfig('uri.here'); $here = $engine->getConfig('uri.here');
$link = $here.$link; $link = $here.$link;
$is_anchor = true;
} }
if ($engine->isTextMode()) { if ($engine->isTextMode()) {
@ -76,7 +76,13 @@ final class PhutilRemarkupDocumentLinkRule extends PhutilRemarkupRule {
return $name; return $name;
} }
$same_window = $engine->getConfig('uri.same-window', false); // Check if this link points to Phorge itself. Micro-optimized.
$is_self = $is_anchor || $starts_with_slash || $uri->isSelf();
// For historical reasons, links opened in a different tab
// for most links as default.
// Now internal resources keep internal link, as default.
$same_window = $engine->getConfig('uri.same-window', $is_self);
if ($same_window) { if ($same_window) {
$target = null; $target = null;
} else { } else {
@ -92,7 +98,7 @@ final class PhutilRemarkupDocumentLinkRule extends PhutilRemarkupRule {
'a', 'a',
array( array(
'href' => $link, 'href' => $link,
'class' => 'remarkup-link', 'class' => $this->getRemarkupLinkClass($is_self),
'target' => $target, 'target' => $target,
'rel' => 'noreferrer', 'rel' => 'noreferrer',
), ),

View file

@ -116,7 +116,9 @@ final class PhutilRemarkupHyperlinkRule extends PhutilRemarkupRule {
$engine = $this->getEngine(); $engine = $this->getEngine();
$same_window = $engine->getConfig('uri.same-window', false); $uri = new PhutilURIHelper($link);
$is_self = $uri->isSelf();
$same_window = $engine->getConfig('uri.same-window', $is_self);
if ($same_window) { if ($same_window) {
$target = null; $target = null;
} else { } else {
@ -127,7 +129,7 @@ final class PhutilRemarkupHyperlinkRule extends PhutilRemarkupRule {
'a', 'a',
array( array(
'href' => $link, 'href' => $link,
'class' => 'remarkup-link', 'class' => $this->getRemarkupLinkClass($is_self),
'target' => $target, 'target' => $target,
'rel' => 'noreferrer', 'rel' => 'noreferrer',
), ),

View file

@ -112,4 +112,20 @@ abstract class PhutilRemarkupRule extends Phobject {
return (strpos($text, PhutilRemarkupBlockStorage::MAGIC_BYTE) === false); return (strpos($text, PhutilRemarkupBlockStorage::MAGIC_BYTE) === false);
} }
/**
* Get the CSS class="" attribute for a Remarkup link.
* It's just "remarkup-link" for all cases, plus the possibility for
* designers to style external links differently.
* @param boolean $is_internal Whenever the link was internal or not.
* @return string
*/
protected function getRemarkupLinkClass($is_internal) {
// Allow developers to style esternal links differently
$classes = array('remarkup-link');
if (!$is_internal) {
$classes[] = 'remarkup-link-ext';
}
return implode(' ', $classes);
}
} }

View file

@ -0,0 +1,78 @@
<?php
/**
* A simple wrapper for PhutilURI, to be aware of the
* relative/absolute context, and other minor things.
*/
final class PhutilURIHelper extends Phobject {
/**
* String version of your original URI.
* @var string
*/
private $uriStr;
/**
* Structured version of your URI.
* @var PhutilURI
*/
private $phutilUri;
/**
* @param string|PhutilURI
*/
public function __construct($uri) {
// Keep the original string for basic checks.
$this->uriStr = phutil_string_cast($uri);
// A PhutilURI may be useful. If available, import that as-is.
// Note that the constructor PhutilURI(string) is a bit expensive.
if ($uri instanceof PhutilURI) {
$this->phutilUri = $uri;
}
}
/**
* Check if the URI points to Phorge itself.
* @return bool
*/
public function isSelf() {
// The backend prefers a PhutilURI object, if available.
$uri = $this->phutilUri ? $this->phutilUri : $this->uriStr;
return PhabricatorEnv::isSelfURI($uri);
}
/**
* Check whenever an URI is just a simple fragment without path and protocol.
* @return bool
*/
public function isAnchor() {
return $this->isStartingWithChar('#');
}
/**
* Check whenever an URI starts with a slash (no protocol, etc.)
* @return bool
*/
public function isStartingWithSlash() {
return $this->isStartingWithChar('/');
}
/**
* A sane default.
*/
public function __toString() {
return $this->uriStr;
}
/**
* Check whenever the URI starts with the provided character.
* @param string $char String that MUST have length of 1.
* @return boolean
*/
private function isStartingWithChar($char) {
return strncmp($this->uriStr, $char, 1) === 0;
}
}

View file

@ -0,0 +1,63 @@
<?php
final class PhutilURIHelperTestCase extends PhabricatorTestCase {
public function testPhutilURIHelper() {
// Every row is a test. Every column is:
// - 0: name of the test
// - 1: test input value
// - 2: is the URI pointing to Phorge itself?
// - 3: is the URI an anchor? (no domain, no protocol)
// - 4: is the URI starting with a slash? (no domain, no protocol)
$tests = array(
array('internal anchor', '#asd', true, true, false),
array('internal relative dir', '/foo/', true, false, true),
array('internal relative dir also', 'foo/', true, false, false),
array('internal root dir', '/', true, false, true),
array('internal root dir', './', true, false, false),
array('internal root dir', '../', true, false, false),
array('internal root dir', '/#asd', true, false, true),
array('external', 'https://gnu.org/', false, false, false),
array('external anchor', 'https://gnu.org/#asd', false, false, false),
);
// Add additional self-tests if base URI is available.
$base = PhabricatorEnv::getEnvConfigIfExists('phabricator.base-uri');
if ($base) {
$domain = id(new PhutilURI($base))->getDomain();
$tests[] = array('base uri', $base, true, false, false);
$tests[] = array('base uri anchor', "{$base}#asd", true, false, false);
}
foreach ($tests as $test) {
$name = $test[0];
$uri = $test[1];
$is_self = $test[2];
$is_anchor = $test[3];
$is_slash = $test[4];
// Test input variants for the constructor of PhutilURIHelper.
$uri_variants = array(
$uri,
new PhutilURI($uri),
);
foreach ($uri_variants as $variant_uri) {
$test_name = pht("test %s value '%s' (from '%s' type %s)",
$name, $variant_uri, $uri, phutil_describe_type($variant_uri));
$uri = new PhutilURIHelper($variant_uri);
$this->assertEqual($is_self, $uri->isSelf(),
pht('%s - points to myself', $test_name));
$this->assertEqual($is_anchor, $uri->isAnchor(),
pht('%s - is just an anchor', $test_name));
$this->assertEqual($is_slash, $uri->isStartingWithSlash(),
pht('%s - is starting with slash', $test_name));
}
}
}
}