Add activation behavior for <summary> #2256
Conversation
Fixes #2246. This also fixes the value that the open attribute is set to to be the empty string, instead of "open", matching existing implementations.
domenic
added
normative change
needs tests
|
tests have passed |
|
|
||
| <li><p>If <var>parent</var> is not a <code>details</code> element, abort these steps.</p> | ||
|
|
||
| <li><p>If <var>parent</var>'s first element child is not the <code>summary</code> element, abort |
There was a problem hiding this comment.
I tend to reserve "this" for method steps, but not for any super-great reason.
|
|
||
| <li> | ||
| <p>If the <code data-x="attr-details-open">open</code> attribute is present on | ||
| <var>parent</var>, remove it. Otherwise, set <var>parent</var>'s <code |
There was a problem hiding this comment.
Should remove and set here call the DOM hooks for removing and setting attributes?
There was a problem hiding this comment.
At first I did, but I noticed that nowhere else in the spec do we do that so far, so probably leave that for a later cleanup.
There was a problem hiding this comment.
It's used in some places, search for concept-element-attributes-
It's OK to have previous element siblings, as long as they are not summary elements. That matches the rendering section better (and also Chrome).
|
OK, ready for round two, with tests: web-platform-tests/wpt#4539 (although apparently they are flaky tests...) |
domenic
removed
the
needs tests
|
It looks like Firefox does not implement the "being rendered" check. I guess the precendent from other activation behaviors in the spec is not to have such a check, so I should probably remove it. |
|
Activation behavior aspect LGTM, but #2246 also talks about focus and accessibility mapping. |
|
the accessibility mappings are already defined and implemented: In all implementations |
| @@ -72501,6 +72535,9 @@ END:VCARD</pre> | |||
|
|
|||
| <li><code>menuitem</code> elements</li> | |||
|
|
|||
| <li><code>summary</code> elements that are the first element child of a <code>details</code> | |||
There was a problem hiding this comment.
This should be fixed to match the others; the first summary child, not the first element child
|
Unsure if my latest commit makes sense; see #2272 (comment) |
zcorpan
added
the
do not merge yet
* Add tests for <summary>'s activation behavior Follows whatwg/html#2256. * Flip tests for being-rendered per whatwg/html#2256 (comment)
This reverts commit 28590b2.
domenic
removed
the
do not merge yet
Fixes #2246. This also fixes the value that the open attribute is set to
to be the empty string, instead of "open", matching existing
implementations.
Tests coming tomorrow. By code inspection at least, Chrome seems to enforce the first-element-childness, which makes sense since that is kind of enforced by the rendering section already.