Add support for inert attribute
#1964
Conversation
🦋 Changeset detectedLatest commit: adfaea1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 80 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
rcj-siteimprove
force-pushed
the
inert
branch
from
91dce76 to
4cf1a2d
Compare
|
!pr extract |
|
!pr extract |
Jym77
left a comment
Jym77
left a comment
There was a problem hiding this comment.
Looks good. I think there is still a problem with open dialog deeply nested in inert containers.
| const button = <button style={{ display: "none" }}>Hidden</button>; | ||
| h.document([button]); | ||
|
|
||
| t.equal(isProgrammaticallyHidden(button), true); |
There was a problem hiding this comment.
Note (non-blocking): t.equal(foo, true) is actually equivalent to t(foo); and t.equal(foo, false) to !t(foo), although I'm still not really sure which style I prefer… (explicitness vs compactness).
| if (this.attribute("inert").isSome()) { | ||
| this._isInert = true; | ||
| } else if (this.name === "dialog" && this.attribute("open").isSome()) { | ||
| this._isInert = false; | ||
| } else { | ||
| this._isInert = this.ancestors(Node.flatTree) | ||
| .find(Element.isElement) | ||
| .map((parent) => parent.isInert()) | ||
| .getOr(false); |
There was a problem hiding this comment.
| if (this.attribute("inert").isSome()) { | |
| this._isInert = true; | |
| } else if (this.name === "dialog" && this.attribute("open").isSome()) { | |
| this._isInert = false; | |
| } else { | |
| this._isInert = this.ancestors(Node.flatTree) | |
| .find(Element.isElement) | |
| .map((parent) => parent.isInert()) | |
| .getOr(false); | |
| this._isInert = test( | |
| or( | |
| // Explicitly inert; | |
| Element.hasAttribute("inert"), | |
| and( | |
| // or not an open dialog, | |
| not(and(Element.hasName("dialog"), Element.hasAttribute("open"))), | |
| // and with an inert ancestor. | |
| element => element | |
| .parent(Node.flatTree) | |
| .filter(Element.isElement) | |
| .some(parent =>parent.isInert()) | |
| ) | |
| ), | |
| this) |
I'm not fully sure the predicate style is best here, though… I did find the this.attribute("inert").isSome() (and the likes) a bit tricky to read, and we do have access to the usual element predicates here, so I tried it that way… But it might be better to keep it low-level (not use the predicates) since we are in the package defining them 🤔
| if (state.isInert) { | ||
| return Inert.of(node); | ||
| } |
There was a problem hiding this comment.
Issue: Won't that break for deeply nested dialogs?
<div inert>
<div>
<dialog open>Hello</dialog>
</div>
</div>
The outer <div> is a Container due to its inert attribute. The inner <div> is a Inert due to the state being set to inert by the parent, this means we never reach the dialog 🤔
I agree that we should set inert elements as Inert when possible, since we work on static snapshot, this will reduce the accessibility tree size and probably make some request faster.
Given that we do now have a Element#isInert predicate (and this seems to redo part of it), how about something like:
- if
inertis set, and no descendant isan open dialognot inert =>Inert; - if
inertis set and some descendant is not inert =>Container; - if
State.inert, then we have aninertContainerancestor:- if
open dialognot inert =>Elementand resetState.inert; - if no descendant is not inert =>
Inert - otherwise,
Container.
- if
- otherwise, as usual.
This probably requires some caching of "no descendant is not inert". Or maybe we can start by gathering all open dialogs (should be at most 1, iirc) and the set of their ancestors 🤔
Co-authored-by: Jean-Yves Moyen <jym@siteimprove.com>
Resolves #1140