Summary: | [GTK][a11y] Add initial implementation of accessible interface when building with ATSPI | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aboxhall, andresg_22, annulen, aperez, apinheiro, bugs-noreply, cfleizach, dmazzoni, ews-watchlist, gyuyoung.kim, jcraig, jdiggs, ryuan.choi, samuel_white, sergio | ||||||
Priority: | P2 | Keywords: | Gtk | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 230255 | ||||||||
Bug Blocks: | 230253, 230259 | ||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2021-09-14 05:36:27 PDT
Created attachment 439730 [details]
Patch
(In reply to Carlos Garcia Campos from comment #1) > Created attachment 439730 [details] > Patch The core changes look good. I leave the ATSPI changes to you :-) Comment on attachment 439730 [details] Patch I have one suggestion below. Nice that this adds many tests, good work with that! View in context: https://bugs.webkit.org/attachment.cgi?id=439730&action=review > Source/WebCore/accessibility/atspi/AccessibilityAtspi.cpp:175 > +static const RoleNameEntry roleNames[] = { Given that AccessibilityRole is an enum, it seems quite wasteful to do a linear lookup every time that the mapping has to be done. Ideally we would use the AccsessibilityRole::Foo values as indexes into an array, which would be trivial in but this is one of the things where C++ gets in the way The next best thing we can do without relying on ugly casts and that the compiler supports C99 designated initializers while parsing C++ is using WTF::SortedArrayMap, which does binary search over the keys: struct RoleNameEntry { const char *name; const char *localizedName; }; static constexpr std::pair<AccessibilityRole, RoleNameEntry> roleNamesList[] = { { AccessibilityRole::Unknown, { "unknown", N_("unknown") } }, // ... more entries ... }; Then for looking up items: const char* AccessibilityAtspi::localizedRoleName(AccessibilityRole role) { static constexpr SortedArrayMap roleNames { roleNamesList }; if (auto entry = roleNames.tryGet(role)) return entry->localizedName; return _("unknown"); } This at least will be O(log n) instead of O(n) for each lookup. (Or, if we really would need the speed, using macros to generate a big switch statement—JSC has some of that :]) Created attachment 439831 [details]
Patch
Comment on attachment 439831 [details]
Patch
Thanks for updating the patch, Carlos :)
The style checker definitely seems like a false positive,
so I think we are good in that regard.
Committed r283361 (242370@main): <https://commits.webkit.org/242370@main> |