Bug 230256

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:

Keywords:

Gtk

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

230255

Bug Blocks:

230253, 230259

Attachments:

Description	Flags
Patch	aperez: review-
Patch	aperez: review+

Carlos Garcia Campos

Reported 2021-09-14 05:36:27 PDT

Basic implementation of accessible interface.

Attachments
Patch (127.56 KB, patch) 2021-09-30 06:20 PDT, Carlos Garcia Campos	aperez: review-	Details Formatted Diff Diff
Patch (128.26 KB, patch) 2021-10-01 01:40 PDT, Carlos Garcia Campos	aperez: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Carlos Garcia Campos

Comment 1 2021-09-30 06:20:53 PDT

Created attachment 439730 [details] Patch

Andres Gonzalez

Comment 2 2021-09-30 07:55:59 PDT

(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 :-)

Adrian Perez

Comment 3 2021-09-30 13:10:23 PDT

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 :])

Carlos Garcia Campos

Comment 4 2021-10-01 01:40:30 PDT

Created attachment 439831 [details] Patch

Adrian Perez

Comment 5 2021-10-01 02:53:48 PDT

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.

Carlos Garcia Campos

Comment 6 2021-10-01 03:03:35 PDT

Committed r283361 (242370@main): <https://commits.webkit.org/242370@main>

Note You need to log in before you can comment on or make changes to this bug.