Bug 230258 - [GTK][a11y] Add implementation of text interface when building with ATSPI
Summary: [GTK][a11y] Add implementation of text interface when building with ATSPI
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 231950
Blocks: ATSPI
  Show dependency treegraph
 
Reported: 2021-09-14 05:39 PDT by Carlos Garcia Campos
Modified: 2021-10-29 10:14 PDT (History)
17 users (show)

See Also:


Attachments
Patch (110.90 KB, patch)
2021-10-20 02:16 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (111.51 KB, patch)
2021-10-20 04:59 PDT, Carlos Garcia Campos
aperez: review+
ews-feeder: commit-queue-
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:39:08 PDT
Implement text interface
Comment 1 Carlos Garcia Campos 2021-10-20 02:16:34 PDT
Created attachment 441860 [details]
Patch
Comment 2 Carlos Garcia Campos 2021-10-20 04:59:58 PDT
Created attachment 441869 [details]
Patch
Comment 3 Andres Gonzalez 2021-10-20 06:41:17 PDT
(In reply to Carlos Garcia Campos from comment #2)
> Created attachment 441869 [details]
> Patch

--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ a/Source/WebCore/accessibility/AXObjectCache.cpp

@@ -1428,9 +1428,10 @@ void AXObjectCache::postTextStateChangeNotification(const Position& position, co

     stopCachingComputedObjectAttributes();

-#if PLATFORM(COCOA)
+#if PLATFORM(COCOA) || USE(ATSPI)
     AccessibilityObject* object = getOrCreate(node);
     if (object && object->accessibilityIsIgnored()) {
+#if PLATFORM(COCOA)
         if (position.atLastEditingPositionForNode()) {
             if (AccessibilityObject* nextSibling = object->nextSiblingUnignored(1))
                 object = nextSibling;
@@ -1438,6 +1439,11 @@ void AXObjectCache::postTextStateChangeNotification(const Position& position, co
             if (AccessibilityObject* previousSibling = object->previousSiblingUnignored(1))
                 object = previousSibling;
         }
+#elif USE(ATSPI)
+        auto* parent = object->parentObjectUnignored();
+        if (is<AccessibilityObject>(parent))
+            object = downcast<AccessibilityObject>(parent);
+#endif
     }

     postTextStateChangeNotification(object, intent, selection);

Why the target should be parentObjectUnignored and not the prev/next sibling?
Comment 4 Carlos Garcia Campos 2021-10-21 00:07:58 PDT
(In reply to Andres Gonzalez from comment #3)
> (In reply to Carlos Garcia Campos from comment #2)
> > Created attachment 441869 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AXObjectCache.cpp
> +++ a/Source/WebCore/accessibility/AXObjectCache.cpp
> 
> @@ -1428,9 +1428,10 @@ void
> AXObjectCache::postTextStateChangeNotification(const Position& position, co
> 
>      stopCachingComputedObjectAttributes();
> 
> -#if PLATFORM(COCOA)
> +#if PLATFORM(COCOA) || USE(ATSPI)
>      AccessibilityObject* object = getOrCreate(node);
>      if (object && object->accessibilityIsIgnored()) {
> +#if PLATFORM(COCOA)
>          if (position.atLastEditingPositionForNode()) {
>              if (AccessibilityObject* nextSibling =
> object->nextSiblingUnignored(1))
>                  object = nextSibling;
> @@ -1438,6 +1439,11 @@ void
> AXObjectCache::postTextStateChangeNotification(const Position& position, co
>              if (AccessibilityObject* previousSibling =
> object->previousSiblingUnignored(1))
>                  object = previousSibling;
>          }
> +#elif USE(ATSPI)
> +        auto* parent = object->parentObjectUnignored();
> +        if (is<AccessibilityObject>(parent))
> +            object = downcast<AccessibilityObject>(parent);
> +#endif
>      }
> 
>      postTextStateChangeNotification(object, intent, selection);
> 
> Why the target should be parentObjectUnignored and not the prev/next sibling?

Because in linux we don't expose text nodes in the tree, so it's the parent the one implementing the text interface.
Comment 5 Andres Gonzalez 2021-10-21 06:24:28 PDT
(In reply to Carlos Garcia Campos from comment #4)
> (In reply to Andres Gonzalez from comment #3)
> > (In reply to Carlos Garcia Campos from comment #2)
> > > Created attachment 441869 [details]
> > > Patch
> > 
> > --- a/Source/WebCore/accessibility/AXObjectCache.cpp
> > +++ a/Source/WebCore/accessibility/AXObjectCache.cpp
> > 
> > @@ -1428,9 +1428,10 @@ void
> > AXObjectCache::postTextStateChangeNotification(const Position& position, co
> > 
> >      stopCachingComputedObjectAttributes();
> > 
> > -#if PLATFORM(COCOA)
> > +#if PLATFORM(COCOA) || USE(ATSPI)
> >      AccessibilityObject* object = getOrCreate(node);
> >      if (object && object->accessibilityIsIgnored()) {
> > +#if PLATFORM(COCOA)
> >          if (position.atLastEditingPositionForNode()) {
> >              if (AccessibilityObject* nextSibling =
> > object->nextSiblingUnignored(1))
> >                  object = nextSibling;
> > @@ -1438,6 +1439,11 @@ void
> > AXObjectCache::postTextStateChangeNotification(const Position& position, co
> >              if (AccessibilityObject* previousSibling =
> > object->previousSiblingUnignored(1))
> >                  object = previousSibling;
> >          }
> > +#elif USE(ATSPI)
> > +        auto* parent = object->parentObjectUnignored();
> > +        if (is<AccessibilityObject>(parent))
> > +            object = downcast<AccessibilityObject>(parent);
> > +#endif
> >      }
> > 
> >      postTextStateChangeNotification(object, intent, selection);
> > 
> > Why the target should be parentObjectUnignored and not the prev/next sibling?
> 
> Because in linux we don't expose text nodes in the tree, so it's the parent
> the one implementing the text interface.

Thanks, makes sense. A comment in the code stating this would be helpful.
Comment 6 Carlos Garcia Campos 2021-10-22 01:51:14 PDT
Committed r284675 (243395@main): <https://commits.webkit.org/243395@main>
Comment 7 Michael Catanzaro 2021-10-29 10:14:32 PDT
It introduced this warning:

[824/1963] Building CXX object Source/...sources/UnifiedSource-aba958d6-1.cpp.o
In file included from /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/unified-sources/UnifiedSource-aba958d6-1.cpp:3:
/home/mcatanzaro/Projects/WebKit/Source/WebCore/accessibility/AXObjectCache.cpp:1404:13: warning: ‘bool WebCore::isPasswordFieldOrContainedByPasswordField(WebCore::AccessibilityObject*)’ defined but not used [-Wunused-function]
 1404 | static bool isPasswordFieldOrContainedByPasswordField(AccessibilityObject* object)
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Can you check the #if guards, please? It's easy enough to guard the newly-unused function, but I'm not sure whether your change in AXObjectCache::enqueuePasswordValueChangeNotification was intentional.