WICG / scroll-to-text-fragment

Proposal to allow specifying a text snippet in a URL fragment
Other
589 stars 42 forks source link

When both fragment and fragment directive are specified, text search should start at the fragment anchor #75

Open othermaciej opened 4 years ago

othermaciej commented 4 years ago

When both fragment and fragment directive are specified, text search should start at the fragment anchor. Currently, the spec says to completely ignore the fragment in UAs that support text fragments. But it would babe a. useful extra capability in the case of multiple matches, and could allow links to more similar locations in supporting and non-supporting browsers. (Search could wrap to ensure this never entirely misses a text match).

nickburris commented 4 years ago

We considered this, but I think this could result in more stale links. With the current design, if there are multiple instances of the text you want to link to in the page, it can be disambiguated by having the surrounding text as context terms. If we have an additional method of disambiguating, by using the closest fragment ID, that's another bit that can change on the page and change the match location.

could allow links to more similar locations in supporting and non-supporting browsers.

The fragment still serves as a fallback with the current design (we scroll to the fragment ID if we can't find the text directive), so it is beneficial for link generators to include a fragment ID as well, which as you mention also allows links to go to similar locations for supporting and non-supporting browsers (assuming by "non-supporting browser" you mean one that strips the fragment directive from the URL but doesn't implement scroll to text).

bokand commented 2 years ago

Just want to mention that I've come around on this and I think I agree with @othermaciej's suggestion. There are cases where text repeats a lot on a page (e.g. long GitHub issue with the same text quoted multiple times) where it can be difficult to disambiguate any given instance. Starting the search from an elementId would be helpful in those cases.

I've started some work on Blink's implementation but haven't had time to prioritize recently but it does seem worth doing.

Given Mozilla is currently looking at implementing, @annevk I wonder if you have thoughts on making this change?

tomayac commented 2 years ago

Funny enough, before switching to the polyfill, my custom-rolled code in the extension tried to look for the closest ID with the idea being that it could serve as a fallback should the text change. Consider the following example:

(Note the typo in the heading.)

<h1 id="example">Exmaple<h1>
<p>Lorem ipsum</p>
<p>Lorem ipsum</p>

A deep link might look like #example:~:Exmaple-,Lorem ipsum.

If the page later fixes the typo, the deep link would break.

<h1 id="example">Example<h1>
<p>Lorem ipsum</p>
<p>Lorem ipsum</p>

But thanks to the ID in the URL, I thought it would still somewhat work and go to the heading at least. I soon realized that it wouldn't work as I had imagined…

GitHub threads with repeated comments drove me mad indeed when I wrote my algorithm. Thankfully the polyfill is there now for all to profit from.

bokand commented 2 years ago

@tomayac - I think in your example, our current implementation would (also) work - the text directive will fail and so we fallback to using the elementId fragment. Or am I misunderstanding?

tomayac commented 2 years ago

Oh, sorry, I confused two things. Yes, our implementation does work indeed in this case. What I was also hoping to achieve by adding the closest ID was to provide a "next-best thing" link to browsers that don't support text fragment links, and this is what doesn't work (since these other browsers interpret everything as the hash. Sorry for the confusion.

annevk commented 2 years ago

This change sounds good to me. I also like the idea of including the nearest ID when generating these links for folks, as they might well be more stable than the text (also consider localization and such). (We haven't started our impl work thus far.)