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.

Sign up for GitHub

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

Jump to bottom

Should FragmentDirective know about TextDirective? #220

Closed
annevk opened this issue Mar 27, 2023 · 1 comment · Fixed by #247
Closed

Should FragmentDirective know about TextDirective? #220

annevk opened this issue Mar 27, 2023 · 1 comment · Fixed by #247
Labels
spec issue

Comments

@annevk
Copy link
Collaborator

annevk commented Mar 27, 2023

I guess currently UnknownDirective is a superset of TextDirective but the current setup allows for them to get out of sync which doesn't seem desirable.
bokand added a commit to bokand/ScrollToTextFragment that referenced this issue Nov 30, 2023
@bokand
Fix and make grammar non-normative
b6381a1 
The grammar is now provided solely as a convenience so this makes the
section non-normative. Also fixes it so that UnknownDirective doesn't
subsume TextDirective.

Fixes WICG#220
@bokand
Copy link
Collaborator

bokand commented Nov 30, 2023

@annevk I'm not sure what you mean by "allows them to get out of sync"? But I do see it's wrong that UnknownDirective covers TextDirective. b6381a1 addresses it (as well as making the grammar non-normative) by making the two disjoint:

TextDirective ::= "text="CharacterString
UnknownDirective ::= CharacterString - TextDirective
CharacterString ::= (ExplicitChar | PercentEncodedByte)*

will that change resolve this issue?

bokand added a commit to bokand/ScrollToTextFragment that referenced this issue Dec 13, 2023
@bokand
Fix and make grammar non-normative
b406c89 
The grammar is now provided solely as a convenience so this makes the
section non-normative. Also fixes it so that UnknownDirective doesn't
subsume TextDirective.

Fixes WICG#220
@bokand bokand mentioned this issue Dec 13, 2023
Merged
bokand added a commit that referenced this issue Dec 13, 2023
@bokand
[Spec] Overhaul directive parsing (#247)
d88512f 
* Specify parsing imperatively

This commit overhauls the parsing steps to avoid using the EBNF grammar
for validity, instead specifying that imperatively. It also moves
parsing to happen earlier in the process so that we pass around parsed
Text Directive objects.

Also makes the steps more precise, referring to infra types and
correctly decoding the strings.

Fixes #221
Fixes #230

* Fix and make grammar non-normative

The grammar is now provided solely as a convenience so this makes the
section non-normative. Also fixes it so that UnknownDirective doesn't
subsume TextDirective.

Fixes #220
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
spec issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Spec] Overhaul directive parsing bokand/ScrollToTextFragment
2 participants
@annevk @bokand