-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Clarify usage of "insertion steps" in the HTML Standard #7712
Conversation
- Introduced the concept HTML element insertion steps - Changed the wording from the issue comment so that the term "insertion steps" come earlier in the sentence
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 setup looks good!
The insertion steps in the HTML std -> The insertion steps for the HTML std
Now that the setups are done I'm going to fix the definitions in each element next |
This would be needed in 4.8.12.5, 4.10.7, 4.10.17.3
It seems like we also need to define "HTML element removing steps" and do the same for insertion steps, because section 4.8.12.5, 4.10.7, 4.10.17.3 are written with both "node is inserted" and "node is removed" symetrically. I added the definition @ 53cf966. As far as I can check from cross-references, all usages of "node is removed" is used symetrically along "node is inserted", so I believe it is fine to fix them altogether in this PR. The ones in 4.10.7, 4.10.17.3 are trivial so I'm going to fix them next. I might need help/opinions on 4.8.4.3.2 (Reacting to DOM mutations, under |
Hmm, 4.10.7 (The
|
Rewrote 4.10.17.3 on my machine but GitHub doesn't allow me to push... (seems like there's issues on the server side) I would like comments/suggestions on the following matters: 4.8.4.3.2 Reacting to DOM mutationsAs this section is written as a list of relevant mutations, taking these two bullets that mention insertion/removing then rewriting as insertion steps doesn't work. I suggest rewriting these two bullets as following:
then either after the list or under section 4.8.3, add the definition of insertion steps/removing steps:
4.10.7 The select element... is as commented above. 4.12.1.1 Processing modelMy understanding is that events listed here which triggers the user agent preparing the If so, I believe that the usage of [inserted] here can be left as is, and they will refer to that specific node's insertion steps if need be. |
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.
Here are a few nits, will comment on the substantive questions next...
Very encouraging progress!
It's definitely fine to introduce definitions that are only used locally :). The confusing thing about the select section though, is it appears to actually be about option elements, not select elements. I.e., it has the precondition "causing the list of options to gain or lose one or more option elements". So I think it would actually end up as option HTML element insertion/removal steps, which say something like
I like this a lot.
This is trickier, because it isn't necessarily a HTML element that is inserted, and we are so far only making the HTML element insertion rigorous. We don't have a plan for DocumentFragments or text nodes or similar. I can think of ways to make this rigorous, although they all have some tradeoffs. (E.g., maybe we should add specific steps for handling nodes that are inserted into connected IMO we should leave this for later. If it becomes the only remaining reference to |
- "is" -> "are" - remove whitespace before <span>
- Added an insertion step under `source` HTML element insertion steps - Wrote the `img` HTML element insertion steps under 4.8.3 to be consistent with the `source` element
- Introduce "selectedness setting algorithm" - Apply that to insertion steps/removing steps and when asked for a reset
Thanks for the suggestions! I added the following:
Now I will try to fix the build error I got from the commit that didn't get in yesterday... |
- text under `li` needs a `p` - nest `ol` under the `li`
True, I was missing the case where the thing inserted is not an HTML element. If we are leaving this part as is as of now then I believe this PR is ready for review. One thing I am worried about is in 4.8.4.3.2 (Reacting to DOM mutations) I had to split the definition of "relevant mutations" across multiple sections (the If there's anything else that needs fixing I am ready to fix so please feel free to point them out too. Thanks in advance! |
source
Outdated
<ol> | ||
<li><p>If <var>oldParent</var> is a <code>picture</code> element, then, count this as a | ||
<span data-x="relevant mutations">relevant mutation</span> for the <code>img</code> element | ||
(<var>removedNode</var>).</p></li> |
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.
Similarly, let's just say "for removedNode".
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.
Fixed @ 4d87aba
source
Outdated
data-x="nodes are inserted">element is inserted</span> as a previous sibling.</p></li> | ||
<li><p>The element's <span>HTML element insertion steps</span> or <span>HTML element removing | ||
steps</span> counts the mutation as a <span data-x="relevant mutations">relevant mutation</span>. | ||
</p></li> |
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.
No whitespace/newline before closing tags here.
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.
Fixed @ 4d87aba
- newline between list items - rewrite img insertion steps to directly mention insertedNode/removedNode - grammar fix - remove redundant dfn data-x - remove extraneous oldParent variable
Use `concept-node-insert-ext` / `concept-node-remove-ext` (in DOM) instead of `nodes are inserted` / `nodes are removed`, as these definitions are now dependent on the DOM Standard definition
- `p` under `li` - then under the same `li` nest the `dl`
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.
Sorry for the delay in review. I found a couple more issues!
I also pushed a commit with minor fixes to this branch. Please do git pull --rebase
before doing any further work and you will get those changes locally.
source
Outdated
|
||
<li><p>The element's parent is a <code>picture</code> element and a <code>source</code> element | ||
that was a previous sibling is <span data-x="nodes are removed">removed</span>.</p></li> | ||
that was a previous sibling is <span data-x="concept-node-remove-ext">removed</span>.</p></li> |
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.
We should be able to convert this into source HTML element removal steps, similar to what you did for the previous bullet.
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.
That's true! Fixed @ 976e48b.
source
Outdated
<p>When a <span>form-associated element</span> or one of its ancestors <span data-x="nodes are | ||
inserted">is inserted</span>, then: | ||
<p>The <code>form</code> <span data-x="html element insertion steps">HTML element insertion | ||
steps</span>, given <var>insertedNode</var>, are:</p> |
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.
So, this change ends up not being correct. It is designed for when any form-associated element is inserted, not for when a form
HTML element is inserted.
I think we should move these steps into "the HTML Standard's insertion steps" and "the HTML Standard's removing steps". Then we can add something like
<p class="note">The form owner is also reset by the HTML Standard's <span>insertion steps</span> and <span>removing steps</span>.</p>
Right above "When the user agent is to reset the form owner".
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.
Fixed @ 3c41a2d. Some notes:
- I used the
concept-node-insert-ext
/concept-node-remove-ext
data-x
here as "the HTML Standard's insertion steps" did not have adata-x
associated with it. - About the ordering: I placed the resetting of the form owner to come after each HTML element's HTML element insertion steps.
- In the insertion steps, there is a step that would "return", which could be problematic when other steps are added after this happens. If there is a chance that anything could be added after this, we can maybe say "Reset the form owner of the form-associated element, unless the form-associated element's parser inserted flag is set." or something around this line.
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.
Both of those decisions look good to me!
- count as a relevant mutation of an `img` when specific conditions are met
It looks like checks didn't run on the latest commit. How do I manually run them? |
Sometimes this happens. You can push a small change to make them re-run, or just wait for us to re-run them as part of the review. |
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.
LGTM, thanks so much for your patience!
steps</span>, given <var>removedNode</var> and <var>oldParent</var>, are:</p> | ||
|
||
<ol> | ||
<li><p>If <var>removedNode</var>'s next sibling is an <code>img</code> element and |
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 realized its next sibling is no longer an img element. I'm not sure if there's a rigorous way to fix this, but I will just change it to "next sibling was an img element"
(partly) closes #2771 .
TODO:
/embedded-content.html ( diff )
/form-control-infrastructure.html ( diff )
/form-elements.html ( diff )
/images.html ( diff )
/infrastructure.html ( diff )
/media.html ( diff )
/scripting.html ( diff )