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

Add API to create text fragment URLs from selection/range #160

Closed
BoCupp-Microsoft opened this issue Dec 14, 2020 · 18 comments
Closed

Add API to create text fragment URLs from selection/range #160

BoCupp-Microsoft opened this issue Dec 14, 2020 · 18 comments

Comments

@BoCupp-Microsoft
Copy link

I imagine that many features that create text fragment URLs will use the browser's selection as an input. Two gotchas in crafting the URLs from a selection are ensuring the prefix or suffix to match don't cross a block boundary and the other (more difficult restriction IMO) is that the match doesn't cross a word boundary.

One solution might be to lift these restrictions. I would be in favor of that so that authors of sites, extensions or browser features can recreate highlights equivalent to selection.

Another possibility would be to extend URL with new APIs that could create the fragment portion of the URL by accepting a range (or for scroll to image an element) to take the burden from the author.

@bokand
Copy link
Collaborator

bokand commented Jan 15, 2021

Yup, I agree it's a big lift for authors/users to generate these themselves. We're currently working on a generator like this in Chromium and are finding it difficult to cover all the cases.

It'd be nice to remove the word boundary restriction (there's other reasons to do so as mentioned in #137) but I'm not sure that would solve the entire problem and I'm not sure whether it's feasible since this is a security mitigation. There's other snags that make this difficult though. You mentioned block boundaries; I don't think it'd be easy to remove that as a restriction as that'd have compatibility consequences. There's also various nuances related to visibility state of nodes, which nodes are considered "empty", etc. so I think this would remain a difficult problem for users.

I'd be in favor of adding a simple API for this. This has some additional upsides too. It would help ensure such links are as stable/friendly as possible by following the best practices/guidelines. For example, preferring to use the full text rather than a brittle range-based match. As the browser engine, we can also easily run the generated fragment through the finder to confirm whether the URL works, which isn't easy for external tooling due to user gesture and other requirements.

Given we're already working on an implementation in Chromium, I think it'd be fairly low-effort for us to just expose that. I'd propose something like:

  let range = window.getSelection().getRangeAt(0);
  let url = window.fragmentDirective.generateSelectorURL(range);

Where generateSelectorURL takes an arbitrary Range object and returns a URL object that uses :~:text= or :~:selector= or some future selector, based on context (or maybe an additional parameter to specify more details).

WDYT?

@bokand bokand changed the title Is it too hard to create text fragment URLs? Add API to create text fragment URLs from selection/range Jan 25, 2021
@tomayac
Copy link
Contributor

tomayac commented May 18, 2021

While we wait for a decision on adding this to the browser directly, the polyfill can do this already, documented here.

@bokand
Copy link
Collaborator

bokand commented Jul 12, 2021

I've been toying with an API that solves both the "generate a URL for the given range" as well as exposes the text fragments from a navigated URL. What do folks think of this?

// Print out the start/end of a text fragment specified in the URL.
let existingTextDirective = document.fragmentDirective.text[0];
console.log(existingTextDirective.exactOrRangeStart);
console.log(existingTextDirective.rangeEnd);
console.log("Full matched text: " + existingTextDirective.getMatchingRange().toString());

// Generate a text fragment for the current selection.
let newTextDirective = document.fragmentDirective.createTextDirective(window.getSelection().getRangeAt(0));
newTextDirective.then( selector => {
  window.location = 'https://example.com#:~:text=' + selector.toString();
});

To the FragmentDirective interface (i.e. window.fragmentDirective) we add:

interface FragmentDirective {
  // Array of parsed TextDirective objects, one for each `text=` term in the fragment directive
  readonly attribute FrozenArray<TextDirective> text;

  // Creates a TextDirective object that can be used to select the given range.
  Promise<TextDirective> createTextDirective(Range range);
 };

And add a new TextDirective interface:

interface TextDirective {
  readonly attribute DOMString? prefix;
  readonly attribute DOMString exactOrRangeStart;
  readonly attribute DOMString? rangeEnd;
  readonly attribute DOMString? suffix;
  Range? getMatchingRange();
  DOMString toString();
};

@eugenegirard
Copy link

Question one: I'm concerned about the line
window.location = 'https://example.com#:~:text=' + newTextDirective.toString();
given that we're also proposing URLs that don't include 'text', could we move knowledge of that tag into the TextDirective/FragmentDirective?

Question two: Our discussion today focused on the failure cases - what should we be doing when we can't generate a unique TextDirective. I think the consensus was that the getMatchingRange promise would fail, and we would monitor over time to see how frequently this happens.

@tomayac
Copy link
Contributor

tomayac commented Jul 19, 2021

let existingTextDirective = document.fragmentDirective.text[0];
console.log(existingTextDirective.exactOrRangeStart);
console.log(existingTextDirective.rangeEnd);

Maybe making it explicit (instead of the exactOrRangeStart) and using the terms from the spec?

existingTextDirective.prefix
existingTextDirective.textStart
existingTextDirective.textEnd
existingTextDirective.suffix

(Not needed fields would just be null.)

// Generate a text fragment for the current selection.
let newTextDirective = document.fragmentDirective.createTextDirective(window.getSelection().getRangeAt(0));

Could this just work with a Selection to be a tad more developer-friendly (less code to type)?

newTextDirective.then( selector => {
window.location = 'https://example.com#:~:text=' + selector.toString();
});

+1 to the above comment of making :~:text part of the output of toString().

@bokand
Copy link
Collaborator

bokand commented Jul 23, 2021

Thanks all for the feedback! (sorry for the delayed response)

could we move knowledge of that tag into the TextDirective/FragmentDirective?

SGTM - so now the generator line would become:

window.location = 'https://example.com#:~:' + newTextDirective.toString();

I'd prefer not to include :~: since a user could, in theory, want to string multiple of these together. WDYT?

what should we be doing when we can't generate a unique TextDirective. I think the consensus was that the getMatchingRange promise would fail, and we would monitor over time to see how frequently this happens.

Right except the promise being rejected is from createTextDirective, not getMatchingRange. That is, once you've got a TextDirective object the range is already set (it could be null because the directive comes from the page load where the text directive didn't match any text on the page document.fragmentDirective.text[0]).

I called it getMatchingRange since we create a new DOM Range object each time this is called. I forgot if I had to do this for a technical reason...But I guess we could we just store a Range on TextDirective and make it a property? Would that be more idiomatic/ergonomic?

Maybe making it explicit (instead of the exactOrRangeStart) and using the terms from the spec?

Yeah, I was just worried that users won't know rangeStart can double as the "exact" match but maybe that's not a big deal. Agree the property list as you have it is nice as it's simpler.

Could this just work with a Selection to be a tad more developer-friendly (less code to type)?

Assuming you mean in addition to Range, yeah! That's a good idea. IIUC Selection is only for user generated selections so we still need a Range overload for programmatically generating these not based on selection.

@bokand
Copy link
Collaborator

bokand commented Jul 23, 2021

IIRC @flackr also suggested making it less text-centric since we expect new selectors in the future, which I think is a good idea. In which case something like:

(EDIT: Also convert the array of terms in FragmentDirective to a generic Directive class so it returns an ordered list of any kind of directive)

interface FragmentDirective {
  // Array of parsed Directive objects, one for each term in the fragment directive (i.e. currently, each `text=` term)
  readonly attribute FrozenArray<Directive> items;

  // Creates a SelectorDirective object that can be used to select the given range/selection.
  Promise<SelectorDirective> createSelectorDirective(Range or Selection);
 };

enum DirectiveType { "text" };

interface Directive {
  readonly attribute DirectiveType type;
  DOMString toString();
}

interface SelectorDirective : Directive {
  Promise<Range> getMatchingRange();
}

interface TextDirective : SelectorDirective {
  constructor(TextDirectiveOptions);
  readonly attribute DOMString prefix;
  readonly attribute DOMString textStart;
  readonly attribute DOMString textEnd;
  readonly attribute DOMString suffix;
};

// Later on...
enum DirectiveType {"TEXT", "CSS_SELECTOR"}
interface CSSSelectorDirective : SelectorDirective {
  readonly attribute DOMString value;
}

The browser could then determine what the best selector to use for any given Range or Selection is, potentially with an argument to createSelectorDirective if an author knows they want a certain selector and new ones are introduced, e.g.:

createSelectorDirective(range, Selectors.TEXT);

Edit (2022-10-13):

  • Removed "optional" from DOMStrings in TextDirective
  • getMatchingRange returns promise
  • Range --> Range or Selection in createSelectorDirective
  • Add constructor to TextDirective

@bokand
Copy link
Collaborator

bokand commented Oct 13, 2021

Ok - I have the implementation behind a flag up for review, it mostly survived the above proposal - I made a few small edits.

I'll add an explainer and spec text after that lands.

@bokand
Copy link
Collaborator

bokand commented Oct 18, 2021

A few edge cases / ambiguities came up during review. Posting them with my thoughts here for visibility and feedback. Thoughts?

What should happen to existing items when a new directive is programmatically added? (using location.hash = ':~:text=foo'

  1. Should setting a new :~:text= replace all existing ones in fragmentDirective.items? Or should it be additive?
  2. Should setting a new :~:text= replace different kinds of directives? e.g. the potential :~:note for annotations.
  3. Should the behavior be the same for all directive types or specific to the type?

For 1 it seems to me that either could work. Granted, if it's additive it might be useful to have a way to remove existing directives.

For 2 it seems, to me at least, the answer is almost certainly no.

3 is less clear to me. It would seem nice to have consistent behavior across types so that we have a clear processing model but this may force semantics that are convenient for one type onto another.

My initial choice was that adding a new directive would replace all existing directives of the same type. Thinking about it more, if we add a way to manually remove directives (without writing a new one) maybe an additive model would be more flexible? OTOH, I can imagine future directives (e.g. :~:translate) where it doesn't make sense to have multiple active at the same time...

Should we support setting a directive using location.hash?

  1. Seems awkward and unintuitive that location.hash = ':~:foo'; assert(location.hash == ':~:foo') doesn't hold
  2. Being able to programmatically set directives seems useful
  3. Without location.hash pages could just write a whole new URL to location but that seems unergonomic

As @flackr pointed out on the review, no other property is set via another property (if we assume the fragment directive is separate from the hash fragment). i.e. setting location.path doesn't affect location.hash.

A method on the API to add directives programmatically seems like it'd resolve some of this tension. It'd also make the relationship to history better - navigating to a directive via a URL adds a history entry (i.e. clicking a link to a same-document text directive adds a history entry) but a site adding a directive (e.g. adding annotations via the proposed :~:note directive would not add a history entry unless the author explicitly did a history.pushState.

(+@grantjwang who's been thinking about :~:note, in case you have any opinions on the above.)

@flackr
Copy link
Collaborator

flackr commented Oct 20, 2021

Adding methods to programmatically update directives could also resolve some of the questions around whether directives are replaced or added to by having explicit APIs for these operations. E.g. location.fragment = '' replaces all directives where location.fragment.text = '' replaces only text directives and location.fragment.note.add(...) adds a note directive.

pull bot pushed a commit to luojiguicai/chromium that referenced this issue Oct 21, 2021
This CL implements, behind a flag, the fragment directive API described
in WICG/scroll-to-text-fragment#160, allowing
authors to read back directives specified in the :~: portion of the URL
fragment, as well as create text-directives URLs using a Range or
Selection.

Because the feature was previously specific to text directives, the
parsing of the URL fragment directive is currently spread across
Document, TextFragmentAnchor and TextFragmentSelector. The semantics of
the new API would make it more convenient to move all this logic into
FragmentDirective itself but this is a sizeable change so we do this in
a separate CL: https://crrev.com/c/3216206.

Bug: 1214791
Change-Id: I189363a5fe9843ac1e55c7078609d61aa33bc26d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3209166
Commit-Queue: David Bokan <[email protected]>
Reviewed-by: Robert Flack <[email protected]>
Cr-Commit-Position: refs/heads/main@{#933676}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This CL implements, behind a flag, the fragment directive API described
in WICG/scroll-to-text-fragment#160, allowing
authors to read back directives specified in the :~: portion of the URL
fragment, as well as create text-directives URLs using a Range or
Selection.

Because the feature was previously specific to text directives, the
parsing of the URL fragment directive is currently spread across
Document, TextFragmentAnchor and TextFragmentSelector. The semantics of
the new API would make it more convenient to move all this logic into
FragmentDirective itself but this is a sizeable change so we do this in
a separate CL: https://crrev.com/c/3216206.

Bug: 1214791
Change-Id: I189363a5fe9843ac1e55c7078609d61aa33bc26d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3209166
Commit-Queue: David Bokan <[email protected]>
Reviewed-by: Robert Flack <[email protected]>
Cr-Commit-Position: refs/heads/main@{#933676}
NOKEYCHECK=True
GitOrigin-RevId: c1d16e9a80991776f4a3f2b8a4f8c13d18a78068
@lucgagan
Copy link

Can someone comment on what's the correct way to generate the fragment based on the current text selection?

https://stackoverflow.com/q/76369701/21936721

@tomayac
Copy link
Contributor

tomayac commented May 31, 2023

Can someone comment on what's the correct way to generate the fragment based on the current text selection?
https://stackoverflow.com/q/76369701/21936721

Done. The question was self-answered, but provided more background in a comment.

@tomayac
Copy link
Contributor

tomayac commented Oct 11, 2023

This discussion seems to have stalled a bit, and yet browsers probably need to develop the code for generating text fragments anyway (at least Chrome has it built-in [right-click, "Copy Link to Highlight"], and we have a polyfill [that's less reliable than the built-in code]).

What do other vendors (informally) think when it comes to exposing such a feature as a Web API? (CC: @annevk who originally raised the Firefox bug but now works for Apple and @megangardner who seems to have been involved in the WebKit implementation.)

@tomayac
Copy link
Contributor

tomayac commented Oct 27, 2023

@jnjaeschke may have a current Mozilla opinion, as the one assigned to the implementation bug. (Sorry if not, it’s a bit of detective work.)

@jnjaeschke
Copy link

Hi @tomayac, yes I'm currently implementing Text Fragments in Gecko (should've been more active here...). Firstly, most of this discussion took place way before I joined Mozilla, so I hope that the following doesn't open any discussions that were already resolved...

Also /cc @zcorpan who knows more about the topic than me :)

Regarding the idea of adding some API for web pages to add a text directive:

In general, I'm in favor of this idea. I think though that the function call should just return the text directive for that given range (i.e. text=foo), nothing more. I also think the before mentioned items array of all (text) directives should not exist for privacy reasons. After all, the web page is only allowed to see the text directives it has created itself.
Additionally, it feels natural to also provide a removeTextDirective() API, which web pages can then use to remove text directives. But of course only the ones the web page has created itself, no third-party- or UA-created text directives. I'm not sure if there should be the additional constraint that it must not remove text directives that are not created by the web page or not (at least it shouldn't indicate if it removed a UA/third party text directive so that web pages can't use brute-force to find out about text directives).

I believe that if we add these APIs to the spec, we should also write spec for the algorithm that creates a text directive from a given range. Since I'm currently working on that, I can volunteer to do that.

Regarding the location.hash = ":~:text=foo" issue:

This is a tough one. I've given it some thought over the weekend and talked about it with a few colleagues, but for now it's just a thought: For me the most consistent solution to this would be to formally "uplift" the fragment directive to be a part of the URL itself instead of being part of the fragment. This allows to completely separate the concerns and define clear use cases and visibility restrictions:

  • the query is used for server-side stuff
  • the fragment is used for same-page nav or state handling
  • the fragment directive is "user agent territory", the web page should not know about this

If that were the case, the question of location.hash is solved: It is not possible to update the fragment directive this way because it's not part of the hash. Attempting to do so would percent-encode the :~:.

Also, to me at least, this idea doesn't sound too far fetched, in a way this is already what's happening. There is a part of the fragment encoded by a separator, which is stripped away from the web page's eyes and used to do some UA-level thing. Only that if the fragment directive is part of the fragment, it is more complicated and harder for the user to understand (why can't the web page see it? Why can't it be set? Do web pages need to take care that doing same-page navigation stuff doesn't break the text fragments or vice versa? etc.).

@tomayac
Copy link
Contributor

tomayac commented Oct 31, 2023

Thank you, @jnjaeschke! So on the concrete question from the Issue ("Add API to create text fragment URLs from selection/range"), we seem to have in-principal agreement from Google and Mozilla then, with the exact shape of the API to-be-determined . Anyone from Apple?

@bokand
Copy link
Collaborator

bokand commented Nov 2, 2023

Welcome @jnjaeschke, excited to hear your working on this in Gecko! Let me know if there's anything spec-side blocking you - happy to help work through these issues.

I've been only sporadically working in this area of late but plan to dedicate more time here towards the end of this year and beginning of next, particularly on spec issues and upstreaming this out of WICG. I also regret we never made progress on this API, I'd be happy to restart on it. The proposals above never really went anywhere so I don't have much attachment to their shape, though I do have a lot of context on how it came to be. Some notes based on your reply:

I also think the before mentioned items array of all (text) directives should not exist for privacy reasons. After all, the web page is only allowed to see the text directives it has created itself.

I agree in general, but there are cases where it may be useful for the initiator to be able to opt-in to making the fragments non-hidden, see #234 for background.

But of course only the ones the web page has created itself, no third-party- or UA-created text directives

I actually think it would be useful to be able to remove all directives. E.g. if the page wants to scroll to a highlight of its own, it'd be best to clear all existing highlights to avoid any potential confusion.

(at least it shouldn't indicate if it removed a UA/third party text directive so that web pages can't use brute-force to find out about text directives).

This is related to issues in #234 - curious, do you see a problem with a page knowing that a directive exists - even if it can't actually see the content of the directive?

I believe that if we add these APIs to the spec, we should also write spec for the algorithm that creates a text directive from a given range.

I'm a little bit worried that baking the algorithm into a spec will make it more difficult to improve over time, or make different, valid, trade offs. Concretely, different UAs could produce different links for the same range (e.g. how many context terms to use, the threshold at which it uses exact vs. range, etc). As long as the link successfully targets the range (based on the find algorithm), it's a valid link. E.g. one UA might prefer to keep links as short as possible for convenience, another may prefer to include as much text as possible in the link for posterity. Both are valid choices.

Perhaps there's aspects we could spec about correctness and everything else could be non-normative recommendations in the form of SHOULD statements?

Regarding the location.hash = ":~:text=foo" issue:

I think we're on more solid footing now than we were at the time I was writing up replies on this issue. #225 revamped how this should work in a way that's consistent. The tl;dr is that the directive is split out from the fragment before being set to the history entry; it's stored in a separate directive state which can be shared across history entries. This precisely defines when the fragment and directive are "disconnected" from each other and when we actually process the directive on history traversal.

This means that setting a directive in location.hash does work. I think this is consistent since the directive is part of the fragment by virtue of being to the right of the # - that's something we cannot easily change as it's a deeply baked in URL parsing behavior.

@bokand
Copy link
Collaborator

bokand commented Dec 13, 2023

I'm going to close this issue since we didn't ship it as part of the initial feature and the current goal is to upstream what we have to HTML. If this is an idea others agree we should pursue, I think the HTML standard is a better venue to work out the API shape.

@bokand bokand closed this as completed Dec 13, 2023
@bokand bokand closed this as not planned Won't fix, can't repro, duplicate, stale Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants