Bug 232622 – [GTK][a11y] Embedded objects are not correctly handled by text interface with ATSPI

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

We are not correctly handling embedded objects (replaced elements) when converting text offsets to/from visible position.

[Carlos Garcia Campos]

Created attachment 443086 [details]
Patch

[Carlos Garcia Campos]

Created attachment 443088 [details]
Patch

[Andres Gonzalez]

(In reply to Carlos Garcia Campos from comment #2)
> Created attachment 443088 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
@@ -2145,7 +2145,12 @@ VisiblePosition AccessibilityRenderObject::visiblePositionForIndex(int index) co
     if (!node)
         return VisiblePosition();

+#if USE(ATSPI)
+    // We need to consider replaced elements for GTK, as they will be presented with the 'object replacement character' (0xFFFC).
+    return { makeDeprecatedLegacyPosition(resolveCharacterLocation(makeRangeSelectingNodeContents(*node), index, TextIteratorBehavior::EmitsObjectReplacementCharacters)) };

Can we use 

VisiblePosition visiblePositionForIndex(int index, ContainerNode* scope)

in editing.cpp adding a TextIteratorBehaviors parameter to it?

[Carlos Garcia Campos]

(In reply to Andres Gonzalez from comment #3)
> (In reply to Carlos Garcia Campos from comment #2)
> > Created attachment 443088 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
> +++ a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
> @@ -2145,7 +2145,12 @@ VisiblePosition
> AccessibilityRenderObject::visiblePositionForIndex(int index) co
>      if (!node)
>          return VisiblePosition();
> 
> +#if USE(ATSPI)
> +    // We need to consider replaced elements for GTK, as they will be
> presented with the 'object replacement character' (0xFFFC).
> +    return {
> makeDeprecatedLegacyPosition(resolveCharacterLocation(makeRangeSelectingNodeC
> ontents(*node), index,
> TextIteratorBehavior::EmitsObjectReplacementCharacters)) };
> 
> Can we use 
> 
> VisiblePosition visiblePositionForIndex(int index, ContainerNode* scope)
> 
> in editing.cpp adding a TextIteratorBehaviors parameter to it?

That was my initial idea, but it expects ContainerNode while we have just a Node and I'm not sure it's ok to change visiblePositionForIndex to receive a Node.

[Andres Gonzalez]

(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 443088 [details]
> > > Patch
> > 
> > --- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
> > +++ a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
> > @@ -2145,7 +2145,12 @@ VisiblePosition
> > AccessibilityRenderObject::visiblePositionForIndex(int index) co
> >      if (!node)
> >          return VisiblePosition();
> > 
> > +#if USE(ATSPI)
> > +    // We need to consider replaced elements for GTK, as they will be
> > presented with the 'object replacement character' (0xFFFC).
> > +    return {
> > makeDeprecatedLegacyPosition(resolveCharacterLocation(makeRangeSelectingNodeC
> > ontents(*node), index,
> > TextIteratorBehavior::EmitsObjectReplacementCharacters)) };
> > 
> > Can we use 
> > 
> > VisiblePosition visiblePositionForIndex(int index, ContainerNode* scope)
> > 
> > in editing.cpp adding a TextIteratorBehaviors parameter to it?
> 
> That was my initial idea, but it expects ContainerNode while we have just a
> Node and I'm not sure it's ok to change visiblePositionForIndex to receive a
> Node.

It should be ok since it is only used for

SimpleRange makeRangeSelectingNodeContents(Node& node)

[Carlos Garcia Campos]

(In reply to Andres Gonzalez from comment #5)
> (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 443088 [details]
> > > > Patch
> > > 
> > > --- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
> > > +++ a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
> > > @@ -2145,7 +2145,12 @@ VisiblePosition
> > > AccessibilityRenderObject::visiblePositionForIndex(int index) co
> > >      if (!node)
> > >          return VisiblePosition();
> > > 
> > > +#if USE(ATSPI)
> > > +    // We need to consider replaced elements for GTK, as they will be
> > > presented with the 'object replacement character' (0xFFFC).
> > > +    return {
> > > makeDeprecatedLegacyPosition(resolveCharacterLocation(makeRangeSelectingNodeC
> > > ontents(*node), index,
> > > TextIteratorBehavior::EmitsObjectReplacementCharacters)) };
> > > 
> > > Can we use 
> > > 
> > > VisiblePosition visiblePositionForIndex(int index, ContainerNode* scope)
> > > 
> > > in editing.cpp adding a TextIteratorBehaviors parameter to it?
> > 
> > That was my initial idea, but it expects ContainerNode while we have just a
> > Node and I'm not sure it's ok to change visiblePositionForIndex to receive a
> > Node.
> 
> It should be ok since it is only used for
> 
> SimpleRange makeRangeSelectingNodeContents(Node& node)

Ok! I'll do it

[Andres Gonzalez]

Why the TextIteratorBehaviors need to be different for ATSPI vx. ATK or Mac? This should all be platform independent. Not saying that you should make it platform-independent, but just want to understand the differences.

[Carlos Garcia Campos]

Created attachment 443302 [details]
Patch

[Carlos Garcia Campos]

(In reply to Andres Gonzalez from comment #7)
> Why the TextIteratorBehaviors need to be different for ATSPI vx. ATK or Mac?
> This should all be platform independent. Not saying that you should make it
> platform-independent, but just want to understand the differences.

I don't know how it's expected to behave in mac, it should be the same for ATK and ATSPI, but I don't want to change the ATK behavior in this patch. The handling of replaced objects is already broken in ATK and fixing it would require more changes not included in this patch. The new ATSPI implementation will replace the ATK one, so I don't plan to fix ATK at this point.

[Carlos Garcia Campos]

Committed r285339 (243900@main): <https://commits.webkit.org/243900@main>