commit 217e8f2bb4d02b9267aa533f1397b3861870748c
parent 7b3bb279a64a46a2740f1f648738021d8899f001
Author: Jan-Niklas Jaeschke <jjaschke@mozilla.com>
Date: Thu, 9 Oct 2025 13:03:59 +0000
Bug 1986373 - Text Fragments: Show context menu entries when either a selection or an existing highlight is present. r=Gijs
This patch mostly reverts the logic introduced in bug 1974126
and introduces a simpler logic as described in comment 12.
With these changes, the copy link to highlight context menu items are visible
- when there's a selection on the page or
- when there is an existing highlight.
If both are present, the selection takes precedence.
This makes the `browser_copy_link_to_existing_highlight.js` test file obsolete,
the simpler logic is tested in `browser_copy_link_to_highlight.js`.
Differential Revision: https://phabricator.services.mozilla.com/D268109
Diffstat:
6 files changed, 72 insertions(+), 185 deletions(-)
diff --git a/browser/actors/ContextMenuChild.sys.mjs b/browser/actors/ContextMenuChild.sys.mjs
@@ -321,26 +321,22 @@ export class ContextMenuChild extends JSWindowActorChild {
) {
return null;
}
- if (!this.textDirectiveTarget) {
- return null;
- }
const sel = this.contentWindow.getSelection();
- const ranges =
- this.textDirectiveTarget === "selection"
- ? Array.from({ length: sel.rangeCount }, (_, i) =>
- sel.getRangeAt(i)
- )
- : this.document.fragmentDirective.getTextDirectiveRanges();
- return this.document.fragmentDirective
- .createTextDirectiveForRanges(ranges)
- .then(textFragment => {
- if (textFragment) {
- let url = URL.fromURI(this.document?.documentURIObject);
- url.hash += `:~:${textFragment}`;
- return url.href;
- }
- return null;
- });
+ const ranges = !sel.isCollapsed
+ ? Array.from({ length: sel.rangeCount }, (_, i) => sel.getRangeAt(i))
+ : this.document.fragmentDirective.getTextDirectiveRanges();
+ return ranges
+ ? this.document.fragmentDirective
+ .createTextDirectiveForRanges(ranges)
+ .then(textFragment => {
+ if (textFragment) {
+ let url = URL.fromURI(this.document?.documentURIObject);
+ url.hash += `:~:${textFragment}`;
+ return url.href;
+ }
+ return null;
+ })
+ : null;
}
case "ContextMenu:RemoveAllTextFragments": {
this.document.fragmentDirective.removeAllTextDirectives();
@@ -936,28 +932,6 @@ export class ContextMenuChild extends JSWindowActorChild {
this.document.fragmentDirective?.getTextDirectiveRanges?.() || [];
// .hasTextFragments indicates whether the page will show highlights.
context.hasTextFragments = !!textDirectiveRanges.length;
- const { offsetNode, offset } =
- node.ownerDocument.caretPositionFromPoint(
- aEvent.clientX,
- aEvent.clientY
- ) || {};
- const sel = this.contentWindow.getSelection();
- context.textDirectiveTarget = null;
- if (offsetNode && offset != null) {
- if (
- !sel.isCollapsed &&
- Array.from({ length: sel.rangeCount }, (_, i) =>
- sel.getRangeAt(i)
- ).some(r => r.isPointInRange(offsetNode, offset))
- ) {
- context.textDirectiveTarget = "selection";
- } else if (
- textDirectiveRanges.some(r => r.isPointInRange(offsetNode, offset))
- ) {
- context.textDirectiveTarget = "textDirective";
- }
- }
- this.textDirectiveTarget = context.textDirectiveTarget;
// Remember the node and its owner document that was clicked
// This may be modifed before sending to nsContextMenu
diff --git a/browser/base/content/browser-context.js b/browser/base/content/browser-context.js
@@ -315,7 +315,7 @@ document.addEventListener(
// attempts to generate the text fragment directive of selected text
// Note: This is kicking off an async operation that might update
// the context menu while it's open (enables an entry).
- if (gContextMenu.textDirectiveTarget) {
+ if (gContextMenu.isContentSelected || gContextMenu.hasTextFragments) {
gContextMenu.getTextDirective();
}
break;
diff --git a/browser/base/content/nsContextMenu.sys.mjs b/browser/base/content/nsContextMenu.sys.mjs
@@ -345,7 +345,6 @@ export class nsContextMenu {
}
this.hasTextFragments = context.hasTextFragments;
- this.textDirectiveTarget = context.textDirectiveTarget;
this.textFragmentURL = null;
} // setContext
@@ -480,7 +479,7 @@ export class nsContextMenu {
this.onEditable ||
this.browser.currentURI.schemeIs("view-source")
) &&
- this.textDirectiveTarget;
+ (this.hasTextFragments || this.isContentSelected);
this.showItem("context-copy-link-to-highlight", shouldShow);
this.showItem("context-copy-clean-link-to-highlight", shouldShow);
diff --git a/browser/base/content/test/contextMenu/browser.toml b/browser/base/content/test/contextMenu/browser.toml
@@ -89,8 +89,6 @@ support-files = [
]
skip-if = ["os == 'linux' && socketprocess_networking"]
-["browser_copy_link_to_existing_highlight.js"]
-
["browser_copy_link_to_highlight.js"]
["browser_copy_link_to_highlight_viewsource.js"]
diff --git a/browser/base/content/test/contextMenu/browser_copy_link_to_existing_highlight.js b/browser/base/content/test/contextMenu/browser_copy_link_to_existing_highlight.js
@@ -1,136 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
- * http://creativecommons.org/publicdomain/zero/1.0/ */
-
-"use strict";
-
-async function openContextMenuAtSelector(browser, selector) {
- const menu = document.getElementById("contentAreaContextMenu");
- const shown = BrowserTestUtils.waitForEvent(menu, "popupshown");
- await BrowserTestUtils.synthesizeMouseAtCenter(
- selector,
- { type: "contextmenu", button: 2 },
- browser
- );
- await shown;
- return menu;
-}
-
-function makeTestURI() {
- const html =
- `<p><span id="hello">Hello</span> ` +
- `<span id="foo">foo</span> <span id="bar">bar</span> ` +
- `<span id="baz">baz</span></p>`;
- const encoded = encodeURIComponent(html);
- return `data:text/html,${encoded}`;
-}
-
-async function waitForHighlight(browser) {
- await SpecialPowers.spawn(browser, [], async () => {
- await ContentTaskUtils.waitForCondition(
- () => content.document.fragmentDirective.getTextDirectiveRanges().length,
- "Fragment highlight range present"
- );
- });
-}
-
-/**
- * Opens the context menu at |selector|, asserts that “Copy Link to Highlight”
- * is visible + enabled, and verifies the clipboard content after clicking it.
- *
- * @param {Browser} browser Remote browser for synth clicks
- * @param {String} selector CSS selector to right-click
- * @param {String} testCase Description used in ok() messages
- * @param {String} textFragmentURL Expected clipboard text
- */
-async function assertHighlightMenu(
- browser,
- selector,
- testCase,
- textFragmentURL = null
-) {
- const menu = await openContextMenuAtSelector(browser, selector);
- const awaitPopupHidden = BrowserTestUtils.waitForEvent(menu, "popuphidden");
- const copy = menu.querySelector("#context-copy-link-to-highlight");
- try {
- ok(!copy.hidden, `"Copy Link to Highlight" visible when ${testCase}`);
- ok(
- !copy.hasAttribute("disabled") ||
- copy.getAttribute("disabled") === "false",
- `"Copy Link to Highlight" enabled when ${testCase}`
- );
-
- if (textFragmentURL !== null) {
- // Click the item and verify clipboard; menu auto-closes.
- await SimpleTest.promiseClipboardChange(textFragmentURL, async () =>
- copy.closest("menupopup").activateItem(copy)
- );
- } else {
- menu.hidePopup();
- }
- } finally {
- // Ensure the menu is closed, even if the test failed.
- await awaitPopupHidden;
- }
-}
-
-add_task(async function test_existing_highlight_paths() {
- await SpecialPowers.pushPrefEnv({
- set: [
- ["dom.text_fragments.enabled", true],
- ["dom.text_fragments.create_text_fragment.enabled", true],
- ],
- });
- const TEST_URI = makeTestURI();
- const tab = await BrowserTestUtils.openNewForegroundTab(
- gBrowser,
- `${TEST_URI}#:~:text=foo%20bar` // Load the page with a highlight
- );
- try {
- const browser = tab.linkedBrowser;
- await waitForHighlight(browser);
-
- // Case 1: click on selection outside highlight
- await SpecialPowers.spawn(browser, [], () => {
- const sel = content.getSelection();
- sel.removeAllRanges();
- const node = content.document.querySelector("#baz").firstChild; // “baz”
- const range = content.document.createRange();
- range.selectNodeContents(node);
- sel.addRange(range);
- });
- await assertHighlightMenu(
- browser,
- "#baz",
- "selection outside highlight",
- `${TEST_URI}#:~:text=baz`
- );
-
- // Case 2: click inside highlight, selection is outside of highlight, highlight takes precedence
- await assertHighlightMenu(
- browser,
- "#foo",
- "inside highlight (no selection)",
- `${TEST_URI}#:~:text=foo%20bar`
- );
-
- // Case 3: selection overlaps highlight, selection takes precedence
- await SpecialPowers.spawn(browser, [], () => {
- const sel = content.getSelection();
- sel.removeAllRanges();
- const startNode = content.document.querySelector("#hello").firstChild;
- const endNode = content.document.querySelector("#foo").firstChild;
- const range = content.document.createRange();
- range.setStart(startNode, 0);
- range.setEnd(endNode, 3);
- sel.addRange(range);
- });
- await assertHighlightMenu(
- browser,
- "#foo", // click on a section where there are both selection and highlight
- "selection overlaps highlight",
- `${TEST_URI}#:~:text=Hello%20foo`
- );
- } finally {
- BrowserTestUtils.removeTab(tab);
- }
-});
diff --git a/browser/base/content/test/contextMenu/browser_copy_link_to_highlight.js b/browser/base/content/test/contextMenu/browser_copy_link_to_highlight.js
@@ -132,6 +132,52 @@ add_task(async function copiesToClipboard() {
});
});
+// If there is already a highlight on the page, "Copy Link to Highlight" should work even if no text is selected.
+add_task(async function copiesToClipWithExistingHighlightAndNoSelection() {
+ await testCopyLinkToHighlight({
+ testPage: loremIpsumTestPage(false, true),
+ runTests: async ({ copyLinkToHighlight }) => {
+ await SimpleTest.promiseClipboardChange(
+ "https://www.example.com/?stripParam=1234#:~:text=Lorem",
+ async () => {
+ await BrowserTestUtils.waitForCondition(
+ () =>
+ !copyLinkToHighlight.hasAttribute("disabled") ||
+ copyLinkToHighlight.getAttribute("disabled") === "false",
+ "Waiting for copyLinkToHighlight to become enabled"
+ );
+ copyLinkToHighlight
+ .closest("menupopup")
+ .activateItem(copyLinkToHighlight);
+ }
+ );
+ },
+ });
+});
+
+// If there is already a highlight on the page and text is selected,
+// "Copy Link to Highlight" should use the selection over the existing highlight.
+add_task(async function copiesToClipWithExistingHighlightAndSelection() {
+ await testCopyLinkToHighlight({
+ testPage: loremIpsumTestPage(true, true),
+ runTests: async ({ copyLinkToHighlight }) => {
+ await SimpleTest.promiseClipboardChange(
+ "https://www.example.com/?stripParam=1234#:~:text=eiusmod%20tempor%20incididunt&text=labore",
+ async () => {
+ await BrowserTestUtils.waitForCondition(
+ () =>
+ !copyLinkToHighlight.hasAttribute("disabled") ||
+ copyLinkToHighlight.getAttribute("disabled") === "false",
+ "Waiting for copyLinkToHighlight to become enabled"
+ );
+ copyLinkToHighlight
+ .closest("menupopup")
+ .activateItem(copyLinkToHighlight);
+ }
+ );
+ },
+ });
+});
/**
* Tests the "Remove Highlight" context menu item.
*
@@ -290,14 +336,16 @@ function editableTestPage() {
*
* @param {boolean} isTextSelected If true, two ranges are created in the
* document and added to the selection.
+ * @param {boolean} loadWithExistingHighlight If true, the page is loaded
+ * with a text fragment in the URL.
* @returns Async function which creates the content.
*/
-function loremIpsumTestPage(isTextSelected) {
+function loremIpsumTestPage(isTextSelected, loadWithExistingHighlight = false) {
return async function (browser) {
await SpecialPowers.spawn(
browser,
- [isTextSelected],
- async function (selectText) {
+ [isTextSelected, loadWithExistingHighlight],
+ async function (selectText, existingHighlight) {
const textBegin = content.document.createTextNode(
"Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do "
);
@@ -320,6 +368,10 @@ function loremIpsumTestPage(isTextSelected) {
paragraph.id = "text";
content.document.body.prepend(paragraph);
+ if (existingHighlight) {
+ content.location.hash = ":~:text=Lorem";
+ }
+
if (selectText) {
const selection = content.getSelection();
const range = content.document.createRange();
