From 349f006904fabf1d4df31ff4840502af3ab379a7 Mon Sep 17 00:00:00 2001 From: Valerio Bozzolan Date: Mon, 29 Jul 2024 12:39:05 +0200 Subject: [PATCH] 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 --- src/__phutil_library_map__.php | 4 + .../PhutilRemarkupDocumentLinkRule.php | 20 +++-- .../PhutilRemarkupHyperlinkRule.php | 6 +- .../markup/markuprule/PhutilRemarkupRule.php | 16 ++++ src/infrastructure/parser/PhutilURIHelper.php | 78 +++++++++++++++++++ .../__tests__/PhutilURIHelperTestCase.php | 63 +++++++++++++++ 6 files changed, 178 insertions(+), 9 deletions(-) create mode 100644 src/infrastructure/parser/PhutilURIHelper.php create mode 100644 src/infrastructure/parser/__tests__/PhutilURIHelperTestCase.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 257c69d1a4..396bb1a308 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -5799,6 +5799,8 @@ phutil_register_library_map(array( 'PhutilTranslatedHTMLTestCase' => 'infrastructure/markup/__tests__/PhutilTranslatedHTMLTestCase.php', 'PhutilTwitchAuthAdapter' => 'applications/auth/adapter/PhutilTwitchAuthAdapter.php', 'PhutilTwitterAuthAdapter' => 'applications/auth/adapter/PhutilTwitterAuthAdapter.php', + 'PhutilURIHelper' => 'infrastructure/parser/PhutilURIHelper.php', + 'PhutilURIHelperTestCase' => 'infrastructure/parser/__tests__/PhutilURIHelperTestCase.php', 'PhutilWordPressAuthAdapter' => 'applications/auth/adapter/PhutilWordPressAuthAdapter.php', 'PhutilXHPASTSyntaxHighlighter' => 'infrastructure/markup/syntax/highlighter/PhutilXHPASTSyntaxHighlighter.php', 'PhutilXHPASTSyntaxHighlighterFuture' => 'infrastructure/markup/syntax/highlighter/xhpast/PhutilXHPASTSyntaxHighlighterFuture.php', @@ -12705,6 +12707,8 @@ phutil_register_library_map(array( 'PhutilTranslatedHTMLTestCase' => 'PhutilTestCase', 'PhutilTwitchAuthAdapter' => 'PhutilOAuthAuthAdapter', 'PhutilTwitterAuthAdapter' => 'PhutilOAuth1AuthAdapter', + 'PhutilURIHelper' => 'Phobject', + 'PhutilURIHelperTestCase' => 'PhabricatorTestCase', 'PhutilWordPressAuthAdapter' => 'PhutilOAuthAuthAdapter', 'PhutilXHPASTSyntaxHighlighter' => 'Phobject', 'PhutilXHPASTSyntaxHighlighterFuture' => 'FutureProxy', diff --git a/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php b/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php index ded57d4c77..e9156b7799 100644 --- a/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php +++ b/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php @@ -44,16 +44,16 @@ final class PhutilRemarkupDocumentLinkRule extends PhutilRemarkupRule { protected function renderHyperlink($link, $name) { $engine = $this->getEngine(); - $is_anchor = false; - if (strncmp($link, '/', 1) == 0) { + $uri = new PhutilURIHelper($link); + $is_anchor = $uri->isAnchor(); + $starts_with_slash = $uri->isStartingWithSlash(); + if ($starts_with_slash) { $base = phutil_string_cast($engine->getConfig('uri.base')); $base = rtrim($base, '/'); $link = $base.$link; - } else if (strncmp($link, '#', 1) == 0) { + } else if ($is_anchor) { $here = $engine->getConfig('uri.here'); $link = $here.$link; - - $is_anchor = true; } if ($engine->isTextMode()) { @@ -76,7 +76,13 @@ final class PhutilRemarkupDocumentLinkRule extends PhutilRemarkupRule { 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) { $target = null; } else { @@ -92,7 +98,7 @@ final class PhutilRemarkupDocumentLinkRule extends PhutilRemarkupRule { 'a', array( 'href' => $link, - 'class' => 'remarkup-link', + 'class' => $this->getRemarkupLinkClass($is_self), 'target' => $target, 'rel' => 'noreferrer', ), diff --git a/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php b/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php index 560aa180c3..dbfca5e5a9 100644 --- a/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php +++ b/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php @@ -116,7 +116,9 @@ final class PhutilRemarkupHyperlinkRule extends PhutilRemarkupRule { $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) { $target = null; } else { @@ -127,7 +129,7 @@ final class PhutilRemarkupHyperlinkRule extends PhutilRemarkupRule { 'a', array( 'href' => $link, - 'class' => 'remarkup-link', + 'class' => $this->getRemarkupLinkClass($is_self), 'target' => $target, 'rel' => 'noreferrer', ), diff --git a/src/infrastructure/markup/markuprule/PhutilRemarkupRule.php b/src/infrastructure/markup/markuprule/PhutilRemarkupRule.php index 4a3c1460e9..a5ddaa4ab6 100644 --- a/src/infrastructure/markup/markuprule/PhutilRemarkupRule.php +++ b/src/infrastructure/markup/markuprule/PhutilRemarkupRule.php @@ -112,4 +112,20 @@ abstract class PhutilRemarkupRule extends Phobject { 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); + } + } diff --git a/src/infrastructure/parser/PhutilURIHelper.php b/src/infrastructure/parser/PhutilURIHelper.php new file mode 100644 index 0000000000..4341585e39 --- /dev/null +++ b/src/infrastructure/parser/PhutilURIHelper.php @@ -0,0 +1,78 @@ +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; + } + +} diff --git a/src/infrastructure/parser/__tests__/PhutilURIHelperTestCase.php b/src/infrastructure/parser/__tests__/PhutilURIHelperTestCase.php new file mode 100644 index 0000000000..4c3a43323c --- /dev/null +++ b/src/infrastructure/parser/__tests__/PhutilURIHelperTestCase.php @@ -0,0 +1,63 @@ +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)); + } + } + } +} -- 2.51.2