GoogleChromeLabs / text-fragments-polyfill

Apache License 2.0
116 stars 27 forks source link

Is the polyfill safe to use when feature detection fails? #157

Open foolip opened 7 months ago

foolip commented 7 months ago

The README recommend this usage:

// Only load the polyfill in browsers that need it.
if (
  !('fragmentDirective' in document)
) {
  import('text-fragments.js');
}

Due to https://webkit.org/b/273466 the polyfill will be loaded in Safari even though scroll to text fragment is supported, and the text is highlighted. Is this a problem, or is it harmless?

tomayac commented 7 months ago

(Just clarifying that the polyfill is never "safe" to use, as it modifies the DOM structure by inserting <mark>: https://github.com/GoogleChromeLabs/text-fragments-polyfill/blob/53375fea08665bac009bb0aa01a030e065c3933d/src/text-fragment-utils.js#L451-L512 The actual question is: does having undetectable native support paired with this polyfill make it even less safe.)

danielweck commented 7 months ago

Instead of injecting mark elements in the DOM in order to perform the styled highlighting of targeted document ranges, the new "CSS Custom Highlight API" could be used (or more precisely, a polyfill for web browsers that do not support it yet):

https://developer.mozilla.org/en-US/docs/Web/API/CSS_Custom_Highlight_API

tomayac commented 7 months ago

Yes, I was about to say, the Venn diagram between "supports CSS Custom Highlight" and "does not support Text Fragments" needs to be studied carefully. And if a CSS Custom Highlight has the same challenges as the current <mark> wrapping. I did not look into that.

foolip commented 7 months ago

I wanted to test the demo in Safari but it looks like the deployment is old and doesn't include https://github.com/GoogleChromeLabs/text-fragments-polyfill/commit/2635dffc84fd8d666f27d580cd2af0fd3193434c.

tomayac commented 7 months ago

The demo pulls in https://unpkg.com/text-fragments-polyfill@5.7.0/dist/text-fragments.js.

tomayac commented 7 months ago

Screenshot 2024-04-30 at 11 46 38

foolip commented 7 months ago

Yes, but it's guarded by the wrong feature detection:

      if (!("fragmentDirective" in Location.prototype)) {
        import("https://unpkg.com/text-fragments-polyfill");
      }
tomayac commented 7 months ago

Oh, sorry. I fixed the glitch on Glitch. I thought you said it was wrong in the library. PTAL.

foolip commented 7 months ago

I see that the script loads in Safari still, but it doesn't seem to modify the DOM. Should I see <mark> somewhere?

tomayac commented 7 months ago

So we get "lucky" here, as Safari correctly reports location.hash as "" when a Text Fragment URL like #:~:text=foo is present (whereas Firefox, as a non-supporting browser, reports the full #:~:text=foo). So the polyfill code just loads for nothing, but then doesn't further interfere with the page.

foolip commented 7 months ago

Is that "luck" something the polyfill could depend on, to do away with the need for feature detection by other means?

tomayac commented 7 months ago

To be clear, the "luck" is spec-mentioned behavior (this is why I put it in scare quotes).

If we're fine with the fact that a polyfill will be loaded for nothing, that is, it will run, but determine that there's no work to do and terminate, then we can say we don't need feature detection. The purist in me is not fine with this, but others may have differing opinions of course.

foolip commented 7 months ago

I was thinking about something like location.hash.includes(':~:') as the condition for loading the polyfill. Or wouldn't that work due to the timing of the stripping?

tomayac commented 7 months ago

My hunch is that this wouldn't work (as the stripping might happen before script has had a chance to run) or at best could be racy, but @bokand can definitely answer this better.

bokand commented 7 months ago

My hunch is that this wouldn't work (as the stripping might happen before script has had a chance to run)

I think you want the stripping to happen for this? Assuming the polyfill reads the :~: directive and performs highlighting where the UA doesn't support it - if the directive is stripped it means the UA performs highlighting (polyfill unneeded). If it isn't stripped - the polyfill is needed. Per-spec, the stripping should happen before any scripts on the document are running. Do I understand correctly?

IMHO, feature detection seems more useful for a page deciding whether to append a :~: directive to anchors since a non-supporting UA won't strip the directive and it could potentially interact badly with script on the destination page.

However, it just came to me that pages can do a feature detection using:

location.hash = ':~:text=test'; 
return location.hash = '' ? return 'supported' : 'unsupported';

This works in both Chrome and Safari, but assumes the page isn't listening to or using the fragment for anything else.

foolip commented 7 months ago

Per-spec, the stripping should happen before any scripts on the document are running.

Thanks, that's the key part I wasn't sure about, and that's great. It means that location.hash.includes(':~:') as the condition for loading the polyfill should be OK.

tomayac commented 7 months ago

The feature detection needs to be made more robust in case the hash is set. I think this could work:

(() => {
  const oldHash = location.hash;
  return (location.hash = location.hash
    ? `${location.hash}:~:text=a`
    : "#:~:text=a`") && location.hash === oldHash
    ? true
    : false;
})();

Not sure if this would break really old #! hashbang Ajax sites, but I fail to find one to test.

bokand commented 7 months ago

https://groups.google.com :)