serenity icon indicating copy to clipboard operation
serenity copied to clipboard

Browser: Implent the ARIA Role Model

Open Epigenetic opened this issue 2 years ago • 5 comments

This PR adds support for the WAI-ARIA role model. This allows us to determine what state and properties are supported/required/prohibited on a given element, as well as providing default values for some properties and state. You can view this information in the new ARIA tab on the widget inspector: image By doing this, we have now implemented the last major piece of the ARIA standards suite before we can start thinking about using this data outside the browser :^)

Epigenetic avatar May 21 '23 14:05 Epigenetic

iʼm suggesting merging that without further ado due to the sheer size! 😅

(those are mostly additions – itʼs hard to spoil something by that. there might be some mistakes but i believe those could be tackled one-by-one later…! 🫣 as for me itʼs mostly structural/scaffolding work, which seems okay)

All but the 3rd commit should be straight forward to review. If you were only going to look at a couple of files for the role models I would suggest RoleType (the root of the inheritance hierarchy) and then picking a role implementation.

Epigenetic avatar Jun 02 '23 01:06 Epigenetic

This has conflicts.

AtkinsSJ avatar Jun 12 '23 08:06 AtkinsSJ

This has conflicts.

Cleaned that up

Epigenetic avatar Jun 12 '23 21:06 Epigenetic

Firstly, thanks for working on this! ARIA is good to support properly, and I think the only reason this hasn't attracted reviews is just how large it is. It's quite hard (and intimidating!) to review a PR that's thousands of lines of code. There's not always a good way to split it though.

I just have a couple of general points from skimming through this. Maybe this doesn't make sense for reasons I'm unaware of since I haven't read the specs, but: a lot of this code looks like you could generate it from a much smaller JSON file, which would be easier to maintain. And yes, I do feel bad saying that after you've spent hours writing 6000 lines of code for it. 😅

Also, AriaData seems to hold the same stuff as AriaMixin, so do we need both? I'm not quite following what the purpose of that is. Maybe it's obvious and I just shouldn't be reviewing it on my phone. 😅

I think code gen is definitely a viable idea, I prototyped it yesterday and it looks like it will work for most of the ARIA roles. I had actually considered code gen when writing this, but was stopped by the spec not being machine parseable and a not just thinking "why don't I make it parseable," so I definitely appreciate the suggestion :).

With regards to AriaData, this is my attempt to maintain ARIA's promise to be language agnostic. In the case of AriaData, this is trying to solve the problem that we don't know if ARIAMixin is GC-allocated or not. This makes properly handling it on the ARIA side more difficult if/when we expand ARIA to other areas. For example, it would be nice to reuse our ARIA implementation in LibGUI instead of implementing another a11y spec or hand rolling our own. In order to not preclude this future usage, I came up with the notion of AriaData, which has a memory model defined by ARIA while ARIAMixin has a memory model defined by whatever implements it. There may be a better way to accomplish this though, so let me know what you think.

Epigenetic avatar Jun 18 '23 12:06 Epigenetic

I think code gen is definitely a viable idea, I prototyped it yesterday and it looks like it will work for most of the ARIA roles. I had actually considered code gen when writing this, but was stopped by the spec not being machine parseable and a not just thinking "why don't I make it parseable," so I definitely appreciate the suggestion :).

Very nice! Several CSS things are coge generators reading hand-written JSON files based on specs. Once you start getting into that mindset, you start seeing it as the solution to a lot of things. (Maybe too much. 😅)

With regards to AriaData, this is my attempt to maintain ARIA's promise to be language agnostic. In the case of AriaData, this is trying to solve the problem that we don't know if ARIAMixin is GC-allocated or not. This makes properly handling it on the ARIA side more difficult if/when we expand ARIA to other areas. For example, it would be nice to reuse our ARIA implementation in LibGUI instead of implementing another a11y spec or hand rolling our own. In order to not preclude this future usage, I came up with the notion of AriaData, which has a memory model defined by ARIA while ARIAMixin has a memory model defined by whatever implements it. There may be a better way to accomplish this though, so let me know what you think.

Ooh, that would be very nice! In that case you'd probably want a separate LibARIA library with that in, which LibGUI and LibWeb both link against. But no pressure to start off that way. I look forward to seeing what you come up with. :^)

AtkinsSJ avatar Jun 19 '23 18:06 AtkinsSJ

Updated to use code gen!

Epigenetic avatar Jun 28 '23 02:06 Epigenetic

I'm realising now, I commented on the use of snake_case for enum values, is that so that they match the ones in the JSON? In general we would have the "real" names in the JSON (so, "aria-foo") and then call title_casify() or snake_casify() etc to convert that to "AriaFoo" or "aria_foo" respectively, for use in the code. I can see that would be a huge pain to change all of those though. 😅

The trouble with "real" ARIA names is that word boundaries are not delineated, for example aria-activedescendant. We would end up turning that into PascalCase as AriaActivedescendant which doesn't feel great to me. If you think that is a better way to go though, I can switch over.

Epigenetic avatar Jun 28 '23 22:06 Epigenetic

Cleaned up the enum names @AtkinsSJ

Epigenetic avatar Jul 04 '23 23:07 Epigenetic