We are not correctly handling embedded objects (replaced elements) when converting text offsets to/from visible position.
Created attachment 443086 [details] Patch
Created attachment 443088 [details] Patch
(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?
(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.
(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)
(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
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.
Created attachment 443302 [details] Patch
(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.
Committed r285339 (243900@main): <https://commits.webkit.org/243900@main>