Bug 265434 – AX: TextInputMarkedTextMarkerRange should be cached during initialization

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joshua Hoffman]

We do not cache TextInputMarkedTextMarkerRange when isolated objects are initialized. This becomes an issue when an object is initialized while there is already marked text.

[Radar WebKit Bug Importer]

<rdar://problem/118866640>

[Joshua Hoffman]

Created attachment 468781 [details]
Patch

[Tyler Wilcock]

Comment on attachment 468781 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=468781&action=review

> LayoutTests/accessibility/mac/text-input-marked-range.html:27
> +    })

For consistency, maybe we add a semicolon here?

> LayoutTests/accessibility/mac/text-input-marked-text-marker-range.html:25
> +        await waitFor(() => {
> +            text = element.stringForTextMarkerRange(element.textInputMarkedTextMarkerRange());
> +            return text == "test";
> +        });
> +        output += await expectAsync("text", "'test'");

Can this be shortened to:

output += await expectAsync("element.stringForTextMarkerRange(element.textInputMarkedTextMarkerRange())", "test");

> LayoutTests/accessibility/mac/text-input-marked-text-marker-range.html:32
> +        await waitFor(() => {
> +            text = element.stringForTextMarkerRange(element.textInputMarkedTextMarkerRange());
> +            return !text;
> +        });
> +        output += await expectAsync("!text", "true");

Can this be shortened to:

output += await expectAsync("!element.stringForTextMarkerRange(element.textInputMarkedTextMarkerRange())", "true");

> LayoutTests/accessibility/mac/text-input-marked-text-marker-range.html:40
> +        textInputController.setMarkedText(" message", 4, " message".length);
> +        await waitFor(() => {
> +            text = element.stringForTextMarkerRange(element.textInputMarkedTextMarkerRange());
> +            return text == " message";
> +        });
> +        output += expect("text", "' message'");

Can we use await expectAsync here too?

> LayoutTests/accessibility/mac/text-input-marked-text-marker-range.html:45
> +    })

For consistency, maybe we add a semicolon after this parentheses?

[Joshua Hoffman]

Created attachment 468785 [details]
Patch

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #4)
> Created attachment 468785 [details]
> Patch

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
index 37c619db01e2..8a278862b697 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
@@ -344,9 +344,17 @@ void AXIsolatedObject::initializeProperties(const Ref<AccessibilityObject>& axOb
     if (descriptor.length())
         setProperty(AXPropertyName::ExtendedDescription, descriptor.isolatedCopy());

-    if (object.isTextControl())
+    if (object.isTextControl()) {
         setProperty(AXPropertyName::SelectedTextRange, object.selectedTextRange());

+        std::pair<AXID, CharacterRange> value;
+        auto range = object.textInputMarkedTextMarkerRange();
+        if (auto characterRange = range.characterRange(); range && characterRange)
+            value = { range.start().objectID(), *characterRange };
+
+        setProperty(AXPropertyName::TextInputMarkedTextMarkerRange, value);

AG: do we have to store the empty value for this property or we can apply the sparse property map technique?

+    }
+
     // These properties are only needed on the AXCoreObject interface due to their use in ATSPI,
     // so only cache them for ATSPI.
 #if PLATFORM(ATSPI)

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #5)
> (In reply to Joshua Hoffman from comment #4)
> > Created attachment 468785 [details]
> > Patch
> 
> diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
> b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
> index 37c619db01e2..8a278862b697 100644
> --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
> +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
> @@ -344,9 +344,17 @@ void AXIsolatedObject::initializeProperties(const
> Ref<AccessibilityObject>& axOb
>      if (descriptor.length())
>          setProperty(AXPropertyName::ExtendedDescription,
> descriptor.isolatedCopy());
> 
> -    if (object.isTextControl())
> +    if (object.isTextControl()) {
>          setProperty(AXPropertyName::SelectedTextRange,
> object.selectedTextRange());
> 
> +        std::pair<AXID, CharacterRange> value;
> +        auto range = object.textInputMarkedTextMarkerRange();
> +        if (auto characterRange = range.characterRange(); range &&
> characterRange)
> +            value = { range.start().objectID(), *characterRange };
> +
> +        setProperty(AXPropertyName::TextInputMarkedTextMarkerRange, value);
> 
> AG: do we have to store the empty value for this property or we can apply
> the sparse property map technique?

setProperty handles filtering out default values, so this is already covered!

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #6)
> (In reply to Andres Gonzalez from comment #5)
> > (In reply to Joshua Hoffman from comment #4)
> > > Created attachment 468785 [details]
> > > Patch
> > 
> > diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
> > b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
> > index 37c619db01e2..8a278862b697 100644
> > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
> > +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
> > @@ -344,9 +344,17 @@ void AXIsolatedObject::initializeProperties(const
> > Ref<AccessibilityObject>& axOb
> >      if (descriptor.length())
> >          setProperty(AXPropertyName::ExtendedDescription,
> > descriptor.isolatedCopy());
> > 
> > -    if (object.isTextControl())
> > +    if (object.isTextControl()) {
> >          setProperty(AXPropertyName::SelectedTextRange,
> > object.selectedTextRange());
> > 
> > +        std::pair<AXID, CharacterRange> value;
> > +        auto range = object.textInputMarkedTextMarkerRange();
> > +        if (auto characterRange = range.characterRange(); range &&
> > characterRange)
> > +            value = { range.start().objectID(), *characterRange };
> > +
> > +        setProperty(AXPropertyName::TextInputMarkedTextMarkerRange, value);
> > 
> > AG: do we have to store the empty value for this property or we can apply
> > the sparse property map technique?
> 
> setProperty handles filtering out default values, so this is already covered!

I think that you can then do:

+        if (auto characterRange = range.characterRange(); range && characterRange)
+            setProperty(AXPropertyName::TextInputMarkedTextMarkerRange, { range.start().objectID(), *characterRange });
+    }

and save the copy to a temp.

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #7)
> (In reply to Joshua Hoffman from comment #6)
> > (In reply to Andres Gonzalez from comment #5)
> > > (In reply to Joshua Hoffman from comment #4)
> > > > Created attachment 468785 [details]
> > > > Patch
> > > 
> > > diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
> > > b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
> > > index 37c619db01e2..8a278862b697 100644
> > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
> > > +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
> > > @@ -344,9 +344,17 @@ void AXIsolatedObject::initializeProperties(const
> > > Ref<AccessibilityObject>& axOb
> > >      if (descriptor.length())
> > >          setProperty(AXPropertyName::ExtendedDescription,
> > > descriptor.isolatedCopy());
> > > 
> > > -    if (object.isTextControl())
> > > +    if (object.isTextControl()) {
> > >          setProperty(AXPropertyName::SelectedTextRange,
> > > object.selectedTextRange());
> > > 
> > > +        std::pair<AXID, CharacterRange> value;
> > > +        auto range = object.textInputMarkedTextMarkerRange();
> > > +        if (auto characterRange = range.characterRange(); range &&
> > > characterRange)
> > > +            value = { range.start().objectID(), *characterRange };
> > > +
> > > +        setProperty(AXPropertyName::TextInputMarkedTextMarkerRange, value);
> > > 
> > > AG: do we have to store the empty value for this property or we can apply
> > > the sparse property map technique?
> > 
> > setProperty handles filtering out default values, so this is already covered!
> 
> I think that you can then do:
> 
> +        if (auto characterRange = range.characterRange(); range &&
> characterRange)
> +            setProperty(AXPropertyName::TextInputMarkedTextMarkerRange, {
> range.start().objectID(), *characterRange });
> +    }
> 
> and save the copy to a temp.

C++ can't infer that type directly, but I can use the constructor inline and achieve the same result. I will upload with that revision included.

[Joshua Hoffman]

Created attachment 468787 [details]
Patch

[EWS]

Committed 271251@main (5965a5a92e80): <https://commits.webkit.org/271251@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 468787 [details].