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

Potential fragment directive parsing bugs #2

Closed
eligrey opened this issue Jan 13, 2022 · 5 comments
Closed

Potential fragment directive parsing bugs #2

eligrey opened this issue Jan 13, 2022 · 5 comments

Comments

@eligrey
Copy link

eligrey commented Jan 13, 2022

This fragment directive parsing code looks flimsy and doesn't correctly handle multiple directives. The selector validation logic is fine but the code which extracts the fragment directives seems problematic.

Feel free to copy this getFragmentDirectives() function. I am dedicating that entire code snippet block to the public domain.

@tomayac
Copy link
Owner

tomayac commented Jan 13, 2022

Do you want to PR this in? Asking since you were interested in contributing on Twitter.

@eligrey
Copy link
Author

eligrey commented Jan 13, 2022

Yeah I can do that later today this weekend 👍

@eligrey
Copy link
Author

eligrey commented Jan 14, 2022

I ended up quite busy today. I'll definitely try to contribute this weekend though

@eligrey
Copy link
Author

eligrey commented Jan 16, 2022

Oh I hadn't realized that this syntax was not compatible with the URLSearchParams parser. Has there been any discussion about aiming to make fragment directives follow a similar syntax to search params?

To re-use the getFragmentDirectives() utility I wrote, this spec would have to use a URL-encoded key-value pairing. I can look into improving the existing code where possible later, as I'm definitely interested in helping get this project running.

@tomayac
Copy link
Owner

tomayac commented Jan 16, 2022

In the case of media, the aim was to reuse the existing syntax of WebAnnotations. See https://github.com/WICG/scroll-to-text-fragment/blob/main/EXTENSIONS.md#proposed-solution and https://www.w3.org/TR/2017/NOTE-selectors-states-20170223/#FragmentSelector_frag.

@eligrey eligrey changed the title Fragment directive parsing bugs Potential fragment directive parsing bugs Jan 25, 2022
@eligrey eligrey closed this as completed Jan 25, 2022
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

2 participants