search

tabvengers / spicy-sections

Creative Commons Zero v1.0 Universal
128 stars 10 forks source link

review of element.ts and current demo #63

Open scottaohara opened 2 years ago

scottaohara commented 2 years ago

Took a look through element.ts per @jonathantneal's mention of working on it in the discord tabvengers channel.

The following are things that jumped out at me.

Line 240 - Shouldn't be necessary to provide a button element with a button role. Unlikely to cause any issues, but curious if this was done for a reason?

Line 247 - Giving a generic div a tabindex=0 is not a good idea. This should only be done if it has a role of tabpanel. The containing element should not be in the focus order otherwise (or likely even have a tabindex=-1).

Line 323 - You can't have a role=none and a tabindex=-1. This would cause browsers to repair for author error and the implicit role of the element would be used instead of none. Comparing with the demonstration, seems this is being applied to the button element, which definitely doesn't support the role=none. This appears to be somehow making the headings get skipped over when trying to navigate with VO+Firefox. VO+Chrome only expose the button and not the heading, unless explicitly navigating by headings.

Line 339 - Same as line 247, only a tabpanel should be in the focus order. Not a region. I'm wary of the use of roe=region on the accordion content areas as it is. That's something an author should knowingly opt into if it's deemed important to expose these areas as landmark regions.

Expanded/collapsed state - I noticed while testing the accordion version of the component that the aria-expanded state is not being consistently updated to reflect state when the button is toggled.

jonathantneal commented 2 years ago

Thank you for the review, @scottaohara!

Re: Line 240 - This is remedied. That code is removed. That code was meant to be redundant, and was only done to keep the attributes in a specific order for the devtools experience, but I have removed it.

Re: Line 247 - This is remedied. That code is removed. This was another mock to keep attributes in the preferred order for the developer tools experience.

Re: Line 323 - This will need to be remedied. That’s a very unexpected side effect. There are two intentions here. The first intention is to ignore the button altogether in the content affordance, as tho the ShadowDOM were not there, so that only the heading role is exposed. The second intention is to allow authors to style each implicit panel and its parts all the same.

Due to the role limitations of a <button>, I may need to swap out the button in the content affordance, or use a different element altogether and wire up the button keyboard behaviors manually.

Re: Line 339 - This is remedied. The region role has been removed.

Re: Expanded/collapsed state - This is remedied, and the aria-expanded state now reflects correctly. Thank you for patiently re-discovering these regressions.