[Spec] Re-write text matching steps #90
Conversation
This commit is a re-write of the text matching portions of the spec. It avoids using TreeWalker and is more specific and detailed in how the various algorithms work. Fixes WICG#73
|
@nickburris: It's a big change. I'll see if I can find a spec expert to review later but could you take a first pass? Put up on https://bokand.github.io/spec.html for ease of review. I've pretty much rewritten all of section "3.5 Navigating to a Text Fragment" so you might just want the published copy here rather than a diff of the markdown. |
There was a problem hiding this comment.
LGTM! This is a huge improvement to the spec, I wish I had comments but after looking through this thoroughly twice I've got nothing. :)
One suggestion, you could add a non-normative note that each parameter individually needs to be within a block, but parameters don't have to all be within one block, e.g. a prefix can be in the block preceding the startText if startText is at the beginning of a block. This does follow from the logic, I just think this would be a useful non-normative note for people working on generating text directives. This note would probably make sense in [=find range from a text directive=] or after the related note in [=find a string in range=].
|
Good idea, added some non-normative notes and an example |
|
Thanks for the review. Note: there are still issues even with this update, two that are top of mind for me:
Will address this and others in follow-ups Edit: Also, the prefix/suffix as specified aren't compatible with RTL bug |
There was a problem hiding this comment.
This looks like a really solid improvement in detail and implementability!
I gave a mix of formatting feedback and some more important feedback on how to avoid using public APIs. The biggest issue is you want to use the https://dom.spec.whatwg.org/#concept-range concept instead of the Range live-range class.
There was a problem hiding this comment.
This looks like a really solid improvement in detail and implementability!
I gave a mix of formatting feedback and some more important feedback on how to avoid using public APIs. The biggest issue is you want to use the https://dom.spec.whatwg.org/#concept-range concept instead of the Range live-range class.
index.bs
Outdated
|
|
||
| See [[#syntax]] for a description of how these parameters are interpreted. | ||
|
|
||
| |textEnd| is optional. If omitted, this is an "exact" search and the returned |
There was a problem hiding this comment.
sgtm, I've defined ParsedTextDirective as a kind of struct so that I say later e.g. "Returns a [=ParsedTextDirective=] struct". JS doesn't really have a notion of struct "types" but is this a valid thing to do in the spec?
Another QQ: If I have a ParsedTextDirective |parsedValues|, do I always have to say |parsedValues|'s [=ParsedTextDirective/textStart=] or can I just say [=ParsedTextDirective/textStart=] where it's clear from context?
|
@domenic: friendly ping to take a look at the changes/replies (PS, I'm still trying to adjust to the GitHub review system - I'm not sure I'm holding it right - so if there's anything I should/shouldn't be doing for easy reviews please lmk). |
|
Thanks for the ping! I'll do my best to get to this tomorrow. |
|
Latest commit has all the latest feedback incorporated. |
|
Thanks for the review and informative tips! |
This commit is a re-write of the text matching portions of the spec.
It avoids using TreeWalker and is more specific and detailed in how the
various algorithms work.
Fixes #73
💥 Error: write EPROTO 139705468127104:error:1407742E:SSL routines:SSL23_GET_SERVER_HELLO:tlsv1 alert protocol version:../deps/openssl/openssl/ssl/s23_clnt.c:772:
💥 ###
PR Preview failed to build. (Last tried on Mar 12, 2020, 8:46 PM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 HTML Diff Service - The HTML Diff Service is used to create HTML diffs of the spec changes suggested in a pull request.
🔗 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.