-
Notifications
You must be signed in to change notification settings - Fork 43
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
Editorial: use infra for lists and sets #79
Conversation
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.
Lots of small nits. Looks good overall.
A couple things around documents here will change though right if we change the setup in HTML? But maybe you're making this cleanup change first so that goes more smoothly.
@@ -102,7 +105,7 @@ it from its <a>node document</a>'s <a>top layer</a>. | |||
that have their <a>fullscreen flag</a> set, in <a>shadow-including tree order</a>. | |||
|
|||
<li> | |||
<p>For each <var>node</var> in <var>nodes</var>, run these substeps: | |||
<p><a>For each</a> <var>node</var> in <var>nodes</var>, run these substeps: |
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 needs a for attribute I think.
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.
Since only "for each" is used now, I've gone with "type:dfn; for:set; text:for each" to make the source a bit more readable.
fullscreen.bs
Outdated
<var>fullscreenElements</var>. | ||
<!-- cross-process --> | ||
|
||
<li><p>Let <var>eventDocs</var> be an empty list. | ||
<li><p>Let <var>eventDocs</var> be an empty <a>list</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.
This can be a set as well?
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.
Sure. I'm going to suggest removing it next, but let's not assume that's the final shape of the spec, more compat problems might revert it.
fullscreen.bs
Outdated
|
||
<li> | ||
<p>For each <var>element</var> in <var>fullscreenElements</var>, in order, run these | ||
<p><a>For each</a> <var>element</var> in <var>fullscreenElements</var>, run these |
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.
Needs a for attribute I think.
fullscreen.bs
Outdated
@@ -252,16 +255,16 @@ these steps: | |||
|
|||
<p class=note>No need to notify observers when nothing has changed. | |||
|
|||
<li><p>Otherwise, append <var>doc</var> to <var>eventDocs</var>. | |||
<li><p>Otherwise, <a for=list>append</a> <var>doc</var> to <var>eventDocs</var>. |
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.
If we change eventDocs to be a set, this needs to change.
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.
Doing that too.
|
||
<li><p>If <var>element</var> is <var>pending</var> and <var>pending</var> is an <{iframe}> | ||
<a>element</a>, then set <var>element</var>'s <a>iframe fullscreen flag</a>. | ||
|
||
<li><p><a lt="Fullscreen an element">Fullscreen <var>element</var></a> within <var>doc</var>. | ||
</ol> | ||
|
||
<li><p>For each <var>doc</var> in <var>eventDocs</var>, in order, <a>fire an event</a> named | ||
<code>fullscreenchange</code> on <var>doc</var>. | ||
<li><p><a>For each</a> <var>doc</var> in <var>eventDocs</var>, <a>fire an event</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.
Needs a for attribute.
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.
And a couple more of these below.
fullscreen.bs
Outdated
|
||
<p>To <dfn export for="top layer">remove</dfn> an <var>element</var> from a <var>top layer</var>, | ||
remove <var>element</var> from <var>top layer</var>. | ||
<a for=set>remove</a> <var>element</var> from <var>top layer</var>. |
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.
Given that these are identical, perhaps we should no longer have a dedicated remove entry point?
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.
Sure, let's say that the top layer is an ordered set instead of that it consists of it, that'll make it natural to remove the extra remove layer.
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.
Ugh, but HTML uses #top-layer-remove. Now I'm going to pretend that it's deliberate that top layer is an abstraction layer you have to go through in case we want to change what kind of data structure it is :)
Yeah, some of this will go away, some for loops will move, so I figured it's best to avoid those discussions in the real upcoming changes. |
With whatwg/fullscreen#79, the top layer is just an ordered set, and the remove operation does need any indirection.
OK, #79 and whatwg/html#2650 good to go I think, but don't assume I didn't make mistakes :) |
This also clarifies the description of top layer order somewhat. In whatwg/html#2650 HTML is adjusted to match.
With whatwg/fullscreen#79, the top layer is just an ordered set, and the remove operation does not need any indirection.
With whatwg/fullscreen#79, the top layer is just an ordered set, and the remove operation does not need any indirection.
This also clarifies the description of top layer order somewhat.
Preview | Diff