-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Make reflect work for ElementInternals #8496
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.
This seems like a good start, but it has some issues... it's unclear how far we can get with a patch that just makes things a little better, versus doing the full rewrite of all reflection that I think both of us have had kicking around in our head for a while. If you do want to shave some yaks, I'm very supportive!
f0363a4
to
02193b2
Compare
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 promising. Sorry for the long delay, and I'll try to be more responsive this time around!
25e5809
to
ab11d66
Compare
If this generally looks good I'll make a more concerted effort to apply this throughout the whole section and tidy up some of the language as well as start working on the corresponding PRs that will be needed. |
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.
Looks good! Please proceed :)
Now that all the algorithms are converted:
Meanwhile I'd still appreciate drive-by review of course of what I managed to get done today. |
While looking at ARIA I hit a bit of a snag, ARIA has a lot of spec-UB: w3c/aria#1110 (comment).
My current thinking is to leave some of the spec-UB in ARIA, but at least make it adopt this new framework. Thoughts? One other thing I wanted to raise, should we rename "native accessibility semantics map" to something more neutral such as "internal content attribute map" so it can be used for other internal state as well, as needed? We could also do that later as there's not going to be many call sites. |
This does not address most of the issues described in w3c#1110, but does provide a better base for ElementInternals support and tackling the issues in w3c#1110. Corresponding HTML change: whatwg/html#8496.
Before #7956 it said
which doesn't seem better. So yeah I guess this bug has been around for a long time.
Yes, I'd be in favor of that. Even if we never plan on expanding it to other attributes, it's a bit weird to have it be accessibility-named now that there's a general framework.
Seems reasonable, but we should probably make addressing this the next priority in this area. |
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.
Looks great, with some clarity-improvement suggestions. Happy to do another round of review though if needed.
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 except maybe that intro bullets thing. I'll also check out the ARIA PR.
Other changes:
Fixes #8442.
Follow-up:
Corresponding ARIA PR: w3c/aria#1876.
/common-dom-interfaces.html ( diff )
/custom-elements.html ( diff )
/dom.html ( diff )
/infrastructure.html ( diff )