| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 10
Clarifications to the table of contents algorithm #180
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
Conversation
add rel tokenization
index.html
Outdated
| white space, split the trimmed value on whitespace and set | ||
| <var>current_toc_branch["rel"]</var> to the resulting <a | ||
| href="https://infra.spec.whatwg.org/#list">list</a> of tokens. | ||
| Otherwise, set <var>current_toc_branch["rel"]</var> to <a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Github does not let me comment on unchanged lines; but it is a consequence of this change: In step (4), where current_toc_branch is initialized, I would expect the rel value to be set to either an empty array or, most probably, to null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this has already been fixed, as I see rel -> null in step 4.
I'm fine with that, but then type should probably be set to null as well for consistency. And in both cases, the "otherwise" statement at the end becomes redundant so can be removed (both values will already be null if their respective conditions to add a value aren't met).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've actually switched all the properties to null by default.
The only one that affects processing is name, but it took me a while to figure out what it was doing. Instead of starting it off as empty string and then setting it to null if a name is an empty string, we should start it off as null and just leave an empty string if that's the result.
Setting it to null conflicts with 5.2, which is where a placeholder gets inserted when the name is an empty string.
| exited immediately after it is processed. An event approach could be applied, but would require | ||
| modifying the algorithm to process/ignore the omitted nodes.</p> | ||
|
|
||
| <ol> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(1) I think this note should come after the first paragraph. Otherwise, I believe the text is o.k.
(2) The usual terminology in the algorithm is
exit the element and continue processing with the next element.
Maybe a slightly better way is to emphasize the situation, something like
exit the element without descending the sub-tree at this element and continue processing with the next element.
A bit more verbose but may help making things clearer...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it's good now, too. The only thing I did was s/omitted/skipped/ in the note to match the terminology from the other PR.
|
Forgot in the formal github review part: maybe adding a reference to this PR in the change log is also appropriate. |
|
A purely stylistic/editorial comment. I found the name |
|
Actually... I found a bug in my code, and I realized that it was due to my misunderstanding of the text: the use of the word 'exit'. (1) On the one hand, we use the term 'exiting an XXX element' to denote the (pseudo) event of, well, exiting the element. So... does (2) mean that: (i) the element also goes through the 'exit event', ie, executes the relevant 'event handler' If you think indeed in terms of events, the answer is (i). But I believe what we really mean in the algorithm (ii). My interpretation of the spec code was (i). Specifically, in step 4.2.2. there is an 'exit' of the sort described in (2), which (erroneously) led to the execution of 4.3 and, in the test Maybe we can avoid this misunderstanding if we use the term 'leaving', instead of 'exiting' in section 4 situation like (1). For example, 4.3. would say "When leaving a list element" instead of "When exiting a list element". And/or we can say "forget the element and continue processing with the next element" for the situation in (2). WDYT? |
- took care of #180 (comment) and #180 (comment) - to handle the issue #180 (comment) I exchanged the word 'exit' in the relevant sentences to 'omit', with a further clarification in the initial note
|
@mattgarrish I have a separate PR (#182) against this PR to handle these issues. |
Minor updates on #180
Ya, I like |
|
I think between the merged PRs and the changes I've just made everything has been captured now, but can you take another look @iherman ? |
|
@mattgarrish yep, it is a go as far as I am concerned! |
This PR addresses the issues noted in #179 as well as the rel tokenizing issues in #177.
@iherman I'm not sure if the note about the event model captures what you wanted or not, so feel free to modify if not.
Preview | Diff