Bug 230256 - [GTK][a11y] Add initial implementation of accessible interface when building with ATSPI
Summary: [GTK][a11y] Add initial implementation of accessible interface when building ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 230255
Blocks: ATSPI 230259
  Show dependency treegraph
 
Reported: 2021-09-14 05:36 PDT by Carlos Garcia Campos
Modified: 2021-10-01 03:03 PDT (History)
15 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2021-09-14 05:36:27 PDT
Basic implementation of accessible interface.
Comment 1 Carlos Garcia Campos 2021-09-30 06:20:53 PDT
Created attachment 439730 [details]
Patch
Comment 2 Andres Gonzalez 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 :-)
Comment 3 Adrian Perez 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 :])
Comment 4 Carlos Garcia Campos 2021-10-01 01:40:30 PDT
Created attachment 439831 [details]
Patch
Comment 5 Adrian Perez 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.
Comment 6 Carlos Garcia Campos 2021-10-01 03:03:35 PDT
Committed r283361 (242370@main): <https://commits.webkit.org/242370@main>